Patch is merged. Note that the implementation follows the adjustments we made to T252202, so the description on this task is obsolete. In particular, we use a single segment parameter rather than using older_than and newer_than.

Since it is already guaranteed to be unique, it's probably unnecessary to explicitly define it as unique. I once thought that doing so would improve database performance, by providing additional hints to the query planner. I have since learned that it may instead degrade write performance. So perhaps this should not be done.

While applying @stable to override annotations, I realized that we have quite a few methods that are technically stable to override, but we don't want extensions to override them. This is typically the case if the method at hand is inherited from a parent class or interface, and implemented in a canonical way for the present class. Such a method is technically stable for overriding, because the abstract definition in the parent class is. But we don't want to encourage extensions to override them. We really want to tell them not to.

might be related to T253289: Remove USE INDEX usertext_timestamp and other references from code

I think we should merge it this week and closely monitor the deprecations log. The more time we have to detect and fix the long tail of things that might have escaped the better. Also, it would be really easy to revert and worst case scenario we will have some logspam

In T256367#6276603, @DannyS712 wrote:

I can try, but there really isn't an established process for this I'm afraid.

I was following the steps laid out in https://wikitech.wikimedia.org/wiki/Volunteer_NDA

In T256367#6276244, @DannyS712 wrote:

@daniel can you ask your manager to take a look and (hopefully) approve this?

I just remembered an important distinction between @unstable and @internal: An interface or class that is marked @internal should not be used in a public interfaces, since code outside the repo should have no knowledge of them. Interfaces or classes marked as @unstable can be used in signatures of public methods, and code outside the repo may have knowledge of the type, but should not rely on members and methods to be stable (yet).

In T255803#6265943, @DannyS712 wrote:

Something that might be helpful to include is stability of public constants, since those cannot emit deprecation warnings when deprecated. Eg for the removal of the Revision class, it has public constants that. How should those be deprecated?

I got the expected behavior wrong for getSubpageText() when writing the fix for getBaseText(). Sorry about that.

Thank you for the detailed response, @dcipoletti! I'm looking forward to learning more once you have progressed in your experimentation.

It seems to me that the discussion on this RFC has been made difficult by of the wide range of use cases and possible solution covered by the umbrella of a "build step". For instance, the idea of pre-compiling vue templates as described above by @Niedzielski seems relatively uncontroversial. On the other hand, automatically downloading npm packages along with their dependencies seems to be much more problematic. It is unclear if the experiment @dcipoletti describes would involve automatic deployment of unaudited third party code.

This shows an empty page now with no error message (no server error, but no user-facing/200-OK'ed error either). Is that intentional? Anyway, LGTM.

I'm just going through all exceptions to mark them as newable, and their constructor stable to call... While it's good to have that for clarity, should we declare all extensions newable per default? Or will adding exceptions to the rules make things confusing?

By the way, we have quite a few usages of @private, apparently applied with the intent of @internal. I propose to just replace them in the code base.

Would you prefer @stable to type hint, @stable to type-hint or @stable to hint instead? If that's clearer and preferred by others, I'd be fine with that. Note that they're meant to be verbs, and I was aiming for each being a single word, but that might be hard in English :)

Stable to type

Any method marked @deprecated, @internal or @unstable.

I would a bit more consistency how this is expressed.

I support DannyS712's request. In particular, access to logstash would be useful for investigating issues.

Per the TechCom meeting on June 24, this proposal enters the Last Call period. If no concerns remain unaddressed by July 9, the draft will replace the existing policy.

Pinging @Tgr, since we have discussed this before.

My take on this is:

In T246284#6256055, @DannyS712 wrote:

Yes, my point was that doing it in the *same* release should only be done if we are sure it's totally unused.
With the new policy, after 1.35 has been branched, we could skip deprecation entirely in such cases, and just remove the code.

That works for me too (just removing in 1.36) but shouldn't we give the deprecation warnings if we are planning on removing it?

In T246284#6255859, @DannyS712 wrote:

Not sure I follow - if it has been soft deprecated for at least one release, and it isn't used by anything that is bundled or deployed, the policy already allows hard deprecation in the second release

In T246284#6255409, @DannyS712 wrote:

Hooks can be hard and soft deprecated in the same release, and the hooks added to DeprecatedHooks that use Revision objects were mostly soft and hard deprecated in 1.35.

In T249434#6254147, @Izno wrote:

(Probably the policy should instead or also say that software deployed to Wikimedia servers MUST NOT have hard deprecation warnings, since there are extensions/skins deployed to Wikimedia that are not in the tarball.)

Per today's TechCom meeting, this RFC is approved as proposed and discussed. Compatibility with IE8 can be dropped from master once 1.35 has been branched. This means that compatibility is retained for the 1.35 released, compatibility for the Wikimedia sites is dropped as soon as 1.35 has been branched (probably some time in July).

In T249434#6247392, @thiemowmde wrote:

Who "tries to get it in 1.35"? The Core Platform Team? Where is this decision documented?

In T249434#6244213, @thiemowmde wrote:

[…] only create Revision objects when they are needed, so that the class can (eventually) be hard deprecated

I know. But what is the point of that if the class is not hard-deprecated? The constructor is as cheap as it can be. The extra conditionals are possibly expensive, and add unnecessary additional sources of errors, mistakes, and confusing.

Let me again try and summarize the key points. Please tell me if I got anything wrong.

• Hard-deprecating the class means making the constructor emit a deprecation warning.
• Deprecation breaks nothing in production, but is does effectively block development until, since it makes phpunit tests fail for code that uses deprecated functionality.
• We can hard-deprecate things only if we have no callers in deployed code (except in code that is itself hard-deprecated).
• We may delay hard-deprecating things still in use by third parties, but only in exceptional cases.
• We are currently constructing Revision objects in non-deprecated code in core in about 20 places, in order to pass them to hooks.
• All hooks that take Revision objects seem to be (soft) deprecated
• IIRC, adding hooks to DeprecatedHooks means hard-deprecating them, causing a warning when extensions use them. This should currently only be the case for hooks soft-deprecated in 1.34 or earlier.

I think the solution is to have one mode that lists IDs and problems, and another to mark IDs. The latter should still only mark if there are indeed problems with the given IDs, and warn if there are none.

In T251778#6239742, @Krinkle wrote:

In T251778#6230801, @daniel wrote:

findBadBlobs already has a mode that thaks a list of revision IDs, […], and marks the inaccessible ones in that batch.

Awesome, I didn't know that :)

I tried to use it just now for e.g. testwiki rev ID 27195. But it doesn't work right now because it requires a BlobAccessException/ExternalStoreException and for the one we are facing now, it fatals due to LBFactory InvalidArgumentException

Counting revisions created by a user is simple enough, but as we discovered during our work on T200259, not all contributions are revisions, some or other things defined by extensions (e.g. discussion contributions from the Flow extension). That complicates things a lot. We would have to live with an inconsistency between the number of contributions listed by /me/contributions, and the number returned by /me/contributions/count for now. We can then introduce a new hook that allows extensions to add to that number to make things consistent again.

We hit a snag with the "stable paging" requirement, as details in T200259: While we could implement stable paging based on timestamp and revision ID for a vanilla MediaWiki install, there are extensions (most notably Flow) that inject "contributions" that are not revisions. It be additional effort to ensure we are not breaking such extensions when changing how the paging works. But more importantly, it's unclear even on a conceptual level how stable paging could work if extensions can mix in arbitrary kinds of "contributions" that do not share a set of unique keys. We will leave the resolution of this issue for later.

Tagging CPT to have a look, in the light of @matmarex analysis.

Unless I'm missing something, composer-merge-plugin is currently the only way cleanly and sanely set up extensions in a development environment. @bd808 is there an alternative to merge-plugin for that use case? Running composer for each extension only works if there are no version conflicts...

In T254271#6231943, @dbarratt wrote:

This reminds me of Symfony's concept of Public vs Private services. Effectively, "private" services can only be injected. They cannot be accessed directly from the container. They even insist that private services are the norm, rather than the exception. Perhaps it would be wise to make a pattern like this in MediaWiki?

In T254271#6231852, @dbarratt wrote:

That makes perfect sense to me. Although, @Anomie's comment in T240307#5728354 seems to imply the exact opposite. :/

I just realized that the HookRunner in the patch is CheckUser's own hook runner. I'd argue that extensions can handle their own hook runners as they please. CheckUser could add a CheckUserHookRunner service to the service container, or instantiate HookRunners on the fly, as seems convenient. CheckUser could also make a CheckUserServices class that wraps MediaWikiServices (or just has its own wiring) to provide a getHookRunner() method. The things I said above apply to core's HookRunner, and to core's MediaWikiServices and service wiring.

The "command" pattern is useful especially for operations that take a lot of operations or parameters, and that produce complex state. Creating dedicated value objects for the input and output would be possible, but I don't see the advantage in that. It seems awkward to have a PageMover and a PageMoveParameters and a PageMoveResult class. a MovePage command comming from a PageCommandFactory seems more straight forward.

Per my code review at https://gerrit.wikimedia.org/r/597283, I don't think we should tolerate undefined ES or LBFactory stores, this is an infrastructure issue, not an issue with the format of anything in the database, and hopefully a one-off. Ignoring these at the cost of a general bad-wipe in the future due to something intermittent seems like a high risk that we can avoid at little to no cost by fixing these by hand instead.

In T254175#6228683, @nnikkhoui wrote:

Just adding a quick note, that cpt has moved some mediawiki REST endpoints from /v0 namespace into /v1 (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/604441). This shouldn't be a problem but just making a note of it.

I'm confused - this ticket is about deprecating hooks, right? Skimmed the discussion, and it seems to be about deprecating construction of Revision objects.

In T238966#6192600, @Marostegui wrote:

@daniel the new columns can be replicated safely to labsdb hosts?

rev_comment_id
rev_actor

In T238966#6205906, @Marostegui wrote:

Some other exceptions seeing on testwiki right now

...

Probably unrelated, but given how complex this schema change is, better be safe than sorry so asking @daniel to confirm this and the previous errors are not related to this schema change before proceeding.

The original core test still throws errors that seem related to Wikibase

You mean this one? https://gerrit.wikimedia.org/r/c/mediawiki/core/+/475065

This is cased by getBaseTitle() when the title starts with a "/", e.g. https://de.wikivoyage.org/wiki//portugal (https://de.wikivoyage.org/wiki/portugal works fine).

Pinging @aaron, since we were just talking about this.
This is currently not a huge problem, though I think we could speed up the e2e tests by having this. We could avoid calling Special:RunJobs for nearly all cases. This will become more relevant as we get more e2e tests.

I'm not sure I understand why the HookRunner wouldn't be in the service container itself? Is there a value to instantiating a new instance of the HookRunner for each service?

In T255043#6212361, @eprodromou wrote:

Cool. Can we change "html_type" to the actual values?

Seems like we have two conflicting routes: