Filed T382572: Clarify relationship between PageUpdatedEvent and EditResult

I noticed that \MediaWiki\Storage\PageUpdatedEvent::getPage returns a PageIdentity instead of a ProperPageIdentity. Is there a particular reason for that?

In T382341#10417384, @Tgr wrote:

I'd generally try to avoid classes which are neither services (stateless) nor value objects (pure state). A PreviewParserInput that mixes page content with some DB access logic seems unwieldy.

Closing as resolved. If anyone wants to re-open the discussion about restricting purges or providing a better mechanism for loading gadgets only for logged in users, please file a separate ticket.

I think this is just blocked on someone (tm) approving the name of the new field in the response. This is an addition to the response schema which cannot easily be undone. It's not clear to me who "owns" that schema...

In T380013#10416967, @Michael wrote:

My concern is that these generic HookHandler classes tend to grow way too large for large features. GE's HomepageHooks is over 1600 lines long, has 100 use ...; statements, and has 22 services injected and who knows how many more statically accessed in various hook handlers via $mwServices = MediaWikiServices::getInstance();. Extracting some private methods for readability and separation of concerns makes it even worse.

In T382341#10412765, @Tgr wrote:

IIRC those exist because we used to guess the page ID and rev ID when rendering the page before save (as part of the "is this a valid content object that can be saved?" tests), and reused the parser output when the guess was correct. Do we not do that anymore?

Straight forward coding, but some conceptual subtleties.

Estimated 5 points. Most of the work is already done now. But the scope is rather large, perhaps this should be split into subtasks.

Should be fixed now. Not in production yet.

Estimated 3 points, mostoly for discussing/defining semantics. The implementation is straight forward (and already compelte).

This is implemented now, though it's still not enabled in production.

In T381982#10414903, @Nikerabbit wrote:

Using the main page as a placeholder title should be declared as an anti pattern?

Tagging WMF-Legal

As the WMF-Legal project tag was added to this task, some general information to avoid wrong expectations:
Please note that public tasks in Wikimedia Phabricator are in general not a place where to expect feedback from the Legal Team of the Wikimedia Foundation due to the scope of the team and/or nature of legal topics. See the project tag description.
Please see https://meta.wikimedia.org/wiki/Legal for when and how to contact the Legal Team. Thanks!

Fun fact: the issue with SignatureValidator wasn't caught in CI because it only happens if the main page is a special page. SignatureValidator uses the main page pas parsing context, which usually works fine... but on translatewiki.net it doesn't.

The obvious cases are done. Keeping this open to remind us to have another look at additional cases that may need covering.

Can we just include a file that has the URLs of all the licenses? Swagger depends on some 35 packages, it seems: https://github.com/swagger-api/swagger-ui/blob/master/package-lock.json#L12C1-L47C30

In T381982#10411763, @hashar wrote:

Both the revert and the patch made it to MW-1.44-notes (1.44.0-wmf.8; 2024-12-17) and the breaking change was only in that branch. That thus never made it to Wikimedia production. That was apparently caught on translatewiki (which runs out of the master branch), was fixed in time and thus is no more blocking the train ;)

@cscott I agree with all that, except for thinking of the thing being parsed as a revision. I did until recently, and I have now convinced myself that it was a mistake. Maybe you can convince me back ;)

In T382086#10409000, @Reedy wrote:

Noting that resources/lib/swagger-ui/LICENSE is there, but is a copy of the Apache 2.0.

There's no LICENSE.txt in the upstream repo either...

I agree with @Tgr that this doesn't do much about the currentRevisionRecordCallback, which is part of the process used to map a Link_target to a "revision" (really maybe a fragment or content?).

That said, I don't know that the "fake revision content wrapper" class *necessarily* has to be a MutableRevisionRecord. We could tighten up the MutableRevisionRecord type and still provide some other sort of "fake revision" interface.

In T369898#10409595, @Joe wrote:

I don't see how we can stop caching and purging the rest api urls.

@abi_ on what wiki did this happen? Or more importantly, with what version of MediaWiki? Is this from production? Or from CI?

In T380677#10409361, @Jdforrester-WMF wrote:

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1094003 isn't listed in the wmf.6 pick-list, which makes sense because it was merged on 11th December and wmf.6 was cut on 3rd December; this patch hasn't been deployed to production yet.

Turns out that forcing a ProperPageIdentity is at odds with the way we use "fake" revisions as parser input: T381982: Fatal exception of type "Wikimedia\Assert\PreconditionException" when marking a page for translation. The conceptual conflict is that revisions can only exist on editable pages, but page content (including multi-slot content) needs to be parsed in the context of a special page (or no page at all).

Looking at other errors in the requests that trigger this error indicate that this may be the consequence of something else failing... In all requests that I looked at, ParserCache is triggering a warning for "inconsistent revision ID", e.g. https://logstash.wikimedia.org/goto/386604c4622a5a49eea2b4823fe3e364 and https://logstash.wikimedia.org/goto/9f64b0643577d3708f452b3027e9ebf9. Many of the affected requests seem to be using pygmentize, but not all do: https://logstash.wikimedia.org/goto/31b977e276d74ba77b2f131b9a4e59b1.

The patch that I hoped would fix this rode the train on the week of December 9th (1.44.0-wmf.6), but apparently didn't fix the issue. I'm confused about the fact that it did nothing. I would have expected the error to at least be triggered earlier, in the constructor of RevisionRecord. If the RevisionRecord was constructed on an existing page, and it's immutable, how can the page later not exist?...

@Legoktm There may be no need for stashing or etags at all if you use the output of https://en.wikipedia.org/w/rest.php/v1/page/Earth/html?flavor=edit. flavor=edit makes it embed all the parsoid annotations in the HTML, which should allow the transform endpoint to turn HTML back into wikitext without the need for stashed state.

The safest pattern for service injection is to use a separate handler for each hook, and to inject only the services needed by that hook.

Calling a hook with the noServices option disables service injection. If a handler for such a hook specifies services, an exception will be thrown when the hook is called.

To me, that line of reasoning makes sense for events as well. Thus, wondering how to name that directory within a component that holds the various handlers.
What do you think?

This would be rather cumbersome in the .map format. GeoJSON allows for a description field that could be abused for this role, but I think it would be a pain to parse.

What would be a good convention for naming the directory that holds the event subscribers?

Thank you!

In T381299#10376789, @Ottomata wrote:

How does this task relate to T378934: Define domain events for page deletion and undeletion?

In T381299#10383200, @Michael wrote:

would/should return:

• The user that performed the purge/changed the restriction, or the user that performed the last edit?
• The user that imported the page or the user that saved the last revision at the source wiki?
• The user that undeleted a revision, or the user that created that undeleted revision, or the user that created the latest revision of the article?

In T381299#10380593, @Ottomata wrote:

purges (see ApiPurge and maybe also PurgeAction).

Hm! Are these page updates? The page (and its revisions) aren't actually changing here, right? It is a materialized view/cached version (html, etc.) of the page that is changing?

Hm, odd. This looks like fallout from https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1092772 and https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1088321, rfespectively. I can only assume that the test infrastructure was running on an inconsistent copy of the code. If this was a "real" problem, the site would be on fire.

Summary of a conversation with @santhosh on Slack:

We decided to go with the subsciber patter, which provides a natural way of wiring core components. See the ChangeTrackingEventIngress class for an example.

In T379959#10351177, @Ottomata wrote:

DomainEventSubscriber - Uses a DomainEventListenerRegistr(ar|y) instance to automate registering listeners (unchnaged)

I think you mean "Uses a DomainEventSource..." ya?

This all comes back to T20493: RFC: Unify the various deletion systems

See also T364652: Make component boundaries in MediaWiki core easier to discover

It seems to me that https://github.com/qossmic/deptrac may be a good condidate. A first step might be to ensure that code under libs/ doesn't use code outside libs/.

I'm unsure if running a deferred update from a deferred update is a problem or not.