Page MenuHomePhabricator

Move Parsoid ServiceWorker.php and extension/src/Config into core
Closed, ResolvedPublic

Description

Core should be able to instantiate Parsoid with appropriate SiteConfig/PageConfig/DataAccess without loading Parsoid as an extension. That is, something like the following should be possible w/ just Parsoid 'library' code and core support:

$siteConfig = $services->get( 'ParsoidSiteConfig' );
$dataAccess = $services->get( 'ParsoidDataAccess' );
$pageConfigFactory = $services->get( 'ParsoidPageConfigFactory' );
$pageConfig = $pageConfigFactory->create(
			$title, $user, $revId, $test->wikitext, $langCode
);
[...]
$parsoid = new Parsoid( $siteConfig, $dataAccess );
$out = $parsoid->wikitext2html( $pageConfig, [ ... ]);

Caveats:

  • This effectively freezes the interfaces in Parsoid/src/Config/* and the Parsoid class. Making changes to these after the move will requires a somewhat elaborate dance: the change must land in Parsoid without breaking core, then Parsoid needs to be released to composer with an appropriate version bump, then a corresponding change to core can be made simultaneous with a version bump in core's composer.json, then finally any backwards-compat code in Parsoid can be removed, then mediawiki-vendor's composer.json needs to be updated for the next train.
  • Because of this elaborate dance, changes to the service configuration to make it more DI friendly will be done in the Parsoid repo before the move, so that they can be reviewed in context and released w/o a dance.
  • It's not clear exactly which services are going to be exported for external use long-term; the ParsoidSiteConfig/ParsoidDataAccess/PageConfigFactory/etc services should probably be marked @unstable for now. (eg Parsoid's PageConfig has substantial overlap with the existing ParserOptions class.)
  • Similarly, there might be a ParsoidFactory service exported temporarily, but the long term plan is not to expose this as a separate service; instead the existing ParserFactory service will start returning ParsoidParser objects at some point (T236812). So any ParsoidFactory service should again be marked unstable/temporary.
  • The code in Parsoid/extension/src/Rest/* is not included in this task: it's not clear whether we will expose the current Parsoid REST API in core, or replace it with a new API. As such, we might still do a copy-paste of Rest/* into VisualEditor for the 1.36 release, as we did in T248343 for 1.35, since the RESTBase replacement (Txxxxx) isn't likely to be complete until 1.37 at the earliest. But at least 1.36 will copy *less* code into VE than 1.35 did.

The deadline for this task is the 1.36 release, although hopefully it will be done in the next month or so, since both T254181 and I6d9999b315def616e973daca0b7d544e502c7212 will benefit from it.

Event Timeline

cscott updated the task description. (Show Details)
ssastry moved this task from Needs Triage to Tech Debt / Big changes on the Parsoid board.

We discussed this in the Expedition team meeting and think it should ideally be fixed for 1.36. Is that likely to happen, @ssastry and @cscott? If not, please untag MW-1.36-release.

Parsing team will use the same stopgap solution we used for 1.35 in 1.36 so that we're not trying to make big changes to already-branched code (since 1.36 has already been branched). We'll do this early in 1.37 so we're not in this same position again. T268776 and T268777 appear to be the last blockers of this work (as in, the last major DI refactorings that would be much more awkward to do once this code is split between two repositories), although it seems like CPT wants to refactor the basic statelessFetchRevision mechanism in T278376#6975839 as well.

although it seems like CPT wants to refactor the basic statelessFetchRevision mechanism in T278376#6975839 as well.

Oh yes we do. We so much do. But, parsoid is already using the approach we are moving towards, so it would actually be much easier for us to do it once the glue code is in core. We will not be changing the interface parsoid library is seeing.

Hey there. This task is proposed as a blocker to MediaWiki 1.37, which will be cut in less than three weeks' time. Please consider whether this will make that deadline, and if not, move it to block the MediaWiki 1.38 release (MW-1.38-release) or remove as a blocker entirely.

I think this missed 1.38 (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/764922, merged today), but has already landed on master for 1.39.

I think this missed 1.38 (https://gerrit.wikimedia.org/r/c/mediawiki/core/+/764922, merged today), but has already landed on master for 1.39.

Is that OK? If so, can we un-tag it as a blocker to releasing 1.38? We want to cut the rc.0.

MarkAHershberger subscribed.

Since no one has said this needs to remain a 1.38 blocker in the past week, I'm moving it off the blocker lists.

Yeah, it took a while to get the trains all aligned, so I decided I'm not going to try to backport this. Thanks.

ssastry assigned this task to cscott.