Replace usage of VirtualRESTServiceClient in Flow extension
Closed, ResolvedPublic
Actions

Description

Flow creates a VRS client based on the $wgVirtualRestConfig. If the config sets $wg...['modules']['restbase'], then RestbaseVirtualRestService (disabled for Flow) could be used but if not, falls back to using ParsoidVirtualRestService (which is what is being used now) because using the /restbase/ module drops data-parsoid mappings after transforming the wikitext -> html.

The main reason why Flow contacts Parsoid via RESTBase is to perform transformations to/from different formats (wikitext -> html, html->wikitext etc):

$url = '/restbase/local/v1/transform/' . $from . '/to/' . $to . '/' .
			urlencode( $prefixedDbTitle );

Recently, we've moved this logic into MW core and based on various options ($from, $to etc) in the Flow\Conversion\Utils::convert() path, we can apply core's Parsoid logic to perform this transformation/conversion directly and cut ties with RESTBase.

NOTE: If parsoid isn't configured, Flow\Conversion\Utils::convert() uses the legacy MediaWiki parser which only transforms wikitext -> html.

Acceptance Criteria

Put tests in place to capture different outputs after transformation is applied with RB configs in place
Remove usage of VirtualRestService and replace with core logic
Ensure the outputs returned by core code after transformation matches above (tests shouldn't break) - WIP: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Flow/+/927619
Review to make sure no VRS logic is still in extension
Test extension manually to make sure it still works as expected
Once deployed, monitor sites for any spikes or failures reported for a week or two.

Details

Subject	Repo	Branch	Lines +/-
[WIP] Fix failing tests due to Parsoid being enabled & used (by default)	mediawiki/extensions/Flow	master	+148 -67
Use ParsoidParser to convert wikitext to HTML.	mediawiki/extensions/Flow	master	+20 -15
Remove usage of VirtualRestService from Flow.	mediawiki/extensions/Flow	master	+8 -301
Use ParsoidParser to convert HTML to Wikitext.	mediawiki/extensions/Flow	master	+18 -4
Skip affected tests unconditionally until gerrit->927619 lands	mediawiki/extensions/Flow	master	+15 -0
Refactor utils::convert()	mediawiki/extensions/Flow	master	+36 -12

Customize query in gerrit

Related Objects
Search...

Status	Assigned	Task
Resolved	DAlangi_WMF	T338921 Remove VirtualRESTService
Resolved	DAlangi_WMF	T179680 Hard deprecate VirtualRESTService
Resolved	• R_Rana	T337223 Replace usage of VirtualRESTServiceClient in Flow extension

Event Timeline

DAlangi_WMF created this task.May 22 2023, 10:08 AM

DAlangi_WMF updated the task description. (Show Details)May 22 2023, 10:39 AM

DAlangi_WMF updated the task description. (Show Details)

Implementation plan for "Remove usage of VirtualRestService and replace with core logic":

Step 1: In Flow\Conversion\Utls::convert, replace the if ( $from === 'wikitext' || $from === 'html' ) conditional section as follows:

		if ( $from === 'wikitext' && $to === 'html' ) {
			return self::wikitext2html( $content, $title );
		} elseif ( $from === 'html' && $to === 'wikitext' ) {
			return self::html2wikitext( $content, $title );
		}

The wikitext2html and html2wikitext should be private methods that contain the old logic, e.g.:

				if ( self::isParsoidConfigured() ) {
					return self::parsoid( 'wikitext', 'html', $content, $title );
				} else {
					return self::parser( 'wikitext', 'html', $content, $title );
				}

The two methods will for now have nearly identical code. Next, we want to replace the code in wikitext2html first, and then the code in html2wikitext later:

Step 2: Replace the code in wikitext2html :

	private static function wikitext2html( string $wikitext, PageIdentity $title ) {
		$parserOptions = ParserOptions::newFromAnon();
		$parserOptions->setRenderReason( __METHOD__ );

		$parser = MediaWikiServices::getInstance()->getParsoidParserFactory()->create();
		$output = $parser->parse( $$wikitext, $title, $parserOptions );

		return $output->getRawText();
	}

Step 3: replace the code in html2wikitext:

	private static function html2wikitext( string $html, PageIdentity $title ) {
		$transform = MediaWikiServices::getInstance()->getHtmlTransformFactory()
			->getHtmlToContentTransform( $html, $title );

		$transform->setOptions( [
			'contentmodel' => CONTENT_MODEL_WIKITEXT
		] );

		$content = $transform->htmlToContent();

		// TODO: Throw if $content isn't a WikitextContentModel instance
		return $content->getText();
	}

This should be done in three different patches.

Before we can replace the implementation, we probably need tests that cover more edge cases. We can get started working on the code already, but before we merge it, we need to be sure we are not going to break conversion for more complex content.

Change 923556 had a related patch set uploaded (by Richika Rana; author: Richika Rana):

[mediawiki/extensions/Flow@master] Refactor utils::convert()

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

gerritbot added a project: Patch-For-Review.May 26 2023, 10:47 AM

Change 923648 had a related patch set uploaded (by Richika Rana; author: Richika Rana):

[mediawiki/extensions/Flow@master] Refactor utils::wikitextToHTML

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

Change 923651 had a related patch set uploaded (by Richika Rana; author: Richika Rana):

[mediawiki/extensions/Flow@master] Refactor utils::htmlToWikitext

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

Change 924478 had a related patch set uploaded (by Richika Rana; author: Richika Rana):

[mediawiki/extensions/Flow@master] Removing the usage of VirtualRestService.

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

Change 923556 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Refactor utils::convert()

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

ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.12; 2023-06-06).May 31 2023, 6:00 PM

Quick note that this topic came up on Slack last week.

A couple of potentially relevant notes:

IIRC, the content is stored in HTML and only converted back to wikitext as needed (via parsoid) when editing.
Every individual comment is a single entry in ExternalStore (I vaguely remember it being considered to make store just 1 document for an entire thread, but fairly certain that never ended up happening)
There are a couple of types of content formats; in addition to ‘html’ and ‘wikitext’, there’s also ‘topic-title-html’ and ‘topic-title-wikitext’ (these are captures in rev_flags); fewer things were supported in titles (only stuff like bold and links, I think). Looks like these use CommentFormatter rather than full parser (see Flow\Conversion\Utils)
Looks like the <body> node is stored & parsoid attributes are present. E.g. execute below via eval.php, where the url is one of SELECT rev_content, rev_flags FROM flow_revision; (to be executed against flowdb on x1)

$content = ExternalStore::fetchFromURL('DB://cluster27/397816490')
$content = \MediaWiki\MediaWikiServices::getInstance()->getBlobStoreFactory()->newSqlBlobStore()->decompressData( $content, ['utf-8', 'gzip'] );
var_dump($content);

e.g.

string(1309) "<body id="mwAA" lang="en" dir="ltr" data-parsoid="{&quot;dsr&quot;:[0,1027,0,0]}" parsoid-version="2.0.0" base-url="//www.wikidata.org/wiki/"><section data-mw-section-id="0" id="mwAQ" data-parsoid="{}"><p id="mwAg" data-parsoid="{&quot;dsr&quot;:[0,1027,0,0]}">Já bych spíše předpokládal, že častěji je položka obecního úřadu zakládána ke kategorii fotografií obecního úřadu na Commons. Myslel bych si, že fotografií obecních úřadů máme podstatně víc než např. položek pro periodika vydávaná obcemi. A ano, vydavatelem těch periodik skutečně bude většinou obec, ať už je vydává prostřednictvím obecního úřadu, nebo prostřednictvím např. redakce podléhající přímo samosprávě, zatímco fotky obecního úřadu jakožto budovy jsou prostě fotkami obecního úřadu - a není výstižnější je označovat jako fotky obce. V naprosté většině menších obcí je obecní úřad reprezentován právě svou jednou sídelní budovou - jen v pár větších městech úřad zabírá více budov, a v takových případech bývá jen jedna z budov sídelní. Ve většině případů obecních úřadů je praktičtější mít jedinou položku zároveň pro budovu i instituci, dělení do dvou položek by data zbytečně tříštilo.</p></section></body>"

Templates & magic words are supported and parsoid receives the title of the discussion page (i.e. Talk:My-page, not Topic:Xyzaskdjnasric) for context
I can’t remember Flow actually expecting anything specific in the html or wikitext content. As long as it was able to roundtrip between all formats and showed content that made sense, Flow didn’t bother with it or rely on anything; it only really mattered to Parsoid, and it’s ability to make sense of it again for conversion in the other direction. I guess I’d probably create a couple of posts on a live copy of flow+parsoid and hit it with every parser-related thing I can think of, then roundtrip those with new parser. If that works, then I see no reason for Flow to trip over anything.
The only PITA I remember is context for parser functions and templates etc, but relevant page title is included in all calls now IIRC.
Looking at Utils.php, it looks like there is some mild manipulation of content going on, most importantly patching up relative urls (in encodeHeadInfo, probably createRelativeTitle too) - since content can be visible both in the discussion namespace, and the Topic: namespace, relative paths may not otherwise accurately resolve.

DAlangi_WMF changed the task status from Open to In Progress.Jun 5 2023, 8:14 AM

DAlangi_WMF triaged this task as Medium priority.

DAlangi_WMF updated the task description. (Show Details)Jun 5 2023, 8:23 AM

Change 923648 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Use ParsoidParser to convert wikitext to HTML.

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

Change 927619 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/extensions/Flow@master] Fix Flow failing tests due to Parsoid being enabled (by default)

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

Possibly caused T338264: Caught exception of type Flow\Exception\DataModelException when trying to submit on MediaWiki.org

Change 928474 had a related patch set uploaded (by D3r1ck01; author: Derick Alangi):

[mediawiki/extensions/Flow@master] Skip affected tests unconditionally until gerrit->927619 lands

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

Change 928474 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Skip affected tests unconditionally until gerrit->927619 lands

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

ReleaseTaggerBot edited projects, added MW-1.41-notes (1.41.0-wmf.13; 2023-06-13); removed MW-1.41-notes (1.41.0-wmf.12; 2023-06-06).Jun 8 2023, 5:00 PM

Change 923651 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Use ParsoidParser to convert HTML to Wikitext.

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

Change 924478 merged by jenkins-bot:

[mediawiki/extensions/Flow@master] Remove usage of VirtualRestService from Flow.

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

DAlangi_WMF updated the task description. (Show Details)Jun 9 2023, 7:32 PM

FJoseph-WMF subscribed.Jun 13 2023, 1:06 PM

DAlangi_WMF closed this task as Resolved.Jun 26 2023, 9:50 AM

DAlangi_WMF assigned this task to • R_Rana.

DAlangi_WMF updated the task description. (Show Details)

Peachey88 edited projects, added StructuredDiscussions; removed MediaWiki-General.Jul 2 2023, 11:23 AM

matmarex mentioned this in T245953: Exception caught: Request to parsoid for "wikitext" to "html" conversion of content connected to title ... failed: (curl error: 28) Timeout was reached.Jan 8 2024, 6:33 PM

matmarex mentioned this in T260648: Zeroconf Parsoid/PHP seems to be considered as unconfigured by Flow Utils.

matmarex mentioned this in T328714: StructuredDiscussions is not handling conversion from html to wikitext on MediaWiki 1.39 zero-config setup.

Replace usage of VirtualRESTServiceClient in Flow extensionClosed, ResolvedPublicActions