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.
Wed, Jul 8
Tue, Jul 7
Mon, Jul 6
Fri, Jul 3
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
Thu, Jul 2
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).
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.
Wed, Jul 1
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.
Mon, Jun 29
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 :)
Fri, Jun 26
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.
Thu, Jun 25
Pinging @Tgr, since we have discussed this before.
My take on this is:
Wed, Jun 24
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).
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.
Tue, Jun 23
Mon, Jun 22
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.
Fri, Jun 19
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.
Thu, Jun 18
Wed, Jun 17
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...
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.
Tue, Jun 16
I'm confused - this ticket is about deprecating hooks, right? Skimmed the discussion, and it seems to be about deprecating construction of Revision objects.
Mon, Jun 15
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.
Fri, Jun 12
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
Thu, Jun 11
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.
Wed, Jun 10
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?
Seems like we have two conflicting routes: