Page MenuHomePhabricator

Move MWParsoid\SiteConfig (and etc) to MediaWiki\Parser\Parsoid\SiteConfig (etc)
Open, Needs TriagePublic

Description

I'm sure there's another task for this already in phab, but we've got a better idea what's needed now.

First, we need to get T261161: Parsoid Config/* interfaces should perhaps be abstract classes done first, because that will be much more awkward after the code is moved.

Moving the code is going to be a little touchy w/r/t the train, making it hard to roll back Parsoid by itself, so we should probably get https://gerrit.wikimedia.org/r/c/mediawiki/core/+/752230 landed first, then do this task no sooner than the train after that.

We should probably play the same games that we do with VE and move the classes to core but keep a copy in Parsoid for a week and have parsoid only register its service workers if they aren't already present in core, which would make it a little more robust to rollbacks, but that does complicate things a bit more than "simply" moving the code. Sample implementation in https://gerrit.wikimedia.org/r/c/mediawiki/services/parsoid/+/763851

Also worth starting the conversation about exactly where in core's tree they'd like the code to live. Parsoid's copy is in \MWParsoid which won't fly for core.

Maybe MediaWiki\Parsoid and the code lives in mediawiki-core:includes/Parsoid? That seems to match the standard directory structure being used elsewhere in core.

Alternatively MediaWiki\Parser is the namespace used for includes/parser in core, so moving our code to MediaWiki\Parser\Parsoid and putting it in includes/parser/Parsoid might be an option, too. Then the "abstract" classes like Parser.php and ParserOutput.php live in includes/parser, while Parsoid code goes in includes/parser/Parsoid, and we eventually put the legacy parser in includes/parser/Legacy once we make Parser.php abstract (T236812).

This is how I hacked the ServiceWiring.php in VE in order to only register if Parsoid hadn't already done it:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/722439/3/includes/VEParsoid/ServiceWiring.php#32
So you'd keep the Parsoid copy of the code in the MWParsoid namespace (so you're not creating namespace conflicts) but then change Parsoid's copy of ServiceWiring.php to return early if MediaWikiServices::getInstance()->getService( 'ParsoidSiteConfig' ) was null. Or maybe you need to do a class_exists('MediaWiki\Parser\Parsoid\ParsoidServices') test instead. Might have to play with it a bit and figure out exactly what order core uses to initialize things.

I think you'd actually want to put that test in Parsoid in a train first and let that roll all the way out. Then if you needed to rollback Parsoid during the next train you could do so safely w/o worrying that core already had the migrated services code.

It occurs to me that VE also had its own copy of Parsoid settings in $wgVisualEditorParsoidSettings. I wonder how we manage that? Probably we should have the copy in core fall back to $wgVisualEditorParsoidSettings if $wgParsoidSettings isn't defined? And wfDeprecate that and we can remove the fallback in the next release.

FWIW we want to leave extension/src/Rest and extension/restRoutes.json where they are for now, at least until T302081 is resolved. When/if we move the rest interface, we can do a similar thing to ensure that we use core's version iff that's available; https://gerrit.wikimedia.org/r/c/mediawiki/extensions/VisualEditor/+/722439/3/includes/VisualEditorHooks.php#69 is how you conditionally register a routes.json (it needs to be named something other than restRoutes.json so that it isn't auto-discovered, then in the extension registration hook you check your conditions and add it if it's the right thing to do.)

And you should double-check with the enterprise API folks. I think they do something already to conditionally enable their rest API iff there is something providing the Parsoid services registered with MediaWikiServices, but they might be checking to see whether the Parsoid extension is installed. In either case they should be able to remove their conditional code once we move the service workers to core.

Event Timeline

Change 763851 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/services/parsoid@master] Only define our services if they haven't already been moved to core

https://gerrit.wikimedia.org/r/763851

When we move classes from Parsoid repo to the core, the question to answer is if we want to preserve git history. But, in this case, the classes are fairly recent and don't have a lot of useful history that might be useful in debugging, so I am okay with losing it with the move.

But, we probably need to figure out a solution to the git history question when we move extensions and other code out of Parsoid. ex: Cite ... that is probably worth preserving across the move from Parsoid to the Cite repo.

Yeah, I've got a solution I've used before, where you use git-filter-tree to remove "everything but" the code in question from a repo, and then use a special option to git-merge to make the addition of the new code appear like a merge from this second repo. But it's a bit intricate; luckily there aren't very many extensions with a significant implementation history in Parsoid.

Change 763851 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Only define our services if they haven't already been moved to core

https://gerrit.wikimedia.org/r/763851

Change 764920 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/services/parsoid@master] Only load our REST API if it hasn't already been moved to core

https://gerrit.wikimedia.org/r/764920

Change 764922 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/core@master] WIP: Copy over Parsoid's Config and ServiceWiring classes

https://gerrit.wikimedia.org/r/764922

Change 766858 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/vendor@master] Bump parsoid to 0.15.0-a22

https://gerrit.wikimedia.org/r/766858

Change 766858 merged by jenkins-bot:

[mediawiki/vendor@master] Bump parsoid to 0.15.0-a22

https://gerrit.wikimedia.org/r/766858

Krinkle added a subscriber: Krinkle.

Removing MW-1.38 blocker tag. Parent task has this already.

Change 764922 merged by jenkins-bot:

[mediawiki/core@master] Copy over Parsoid's Config and ServiceWiring classes

https://gerrit.wikimedia.org/r/764922

Change 764920 abandoned by C. Scott Ananian:

[mediawiki/services/parsoid@master] Only load our REST API if it hasn't already been moved to core

Reason:

See description in comments.

https://gerrit.wikimedia.org/r/764920