Page MenuHomePhabricator

"parsoidfragment" visible in rendered articles
Open, HighPublicBUG REPORT

Description

Recently, visible placeholders like {{#parsoidfragment:2}} appeared in de.Wikipedia.

A minimal example is:

{{#tag:phonos|<nowiki />test|file=He-Purim.ogg}}

Parsoid creates "{{#parsoidfragment:11}}testⓘ" for this example, while the legacy parser creates the expected "testⓘ". More examples under https://de.wikipedia.org/wiki/Benutzer:Kallichore/ParsoidFragment . Unmatched nowiki tags are common in templates, eg in https://de.wikipedia.org/wiki/Vorlage:Nowrap :

<onlyinclude><span style="white-space:nowrap">{{#if:trim|<nowiki />{{{1|}}}}}</span></onlyinclude>

Discussion in de.Wikipedia: https://de.wikipedia.org/wiki/Wikipedia:Fragen_zur_Wikipedia#Darstellungsfehler:_Hebr%C3%A4isch_mit_Audiodatei

Event Timeline

The problem is also present for matched nowikis:

{{#tag:phonos|A<nowiki>B</nowiki>C|file=He-Purim.ogg}}

is rendered as

A{{#parsoidfragment:13}}Cⓘ
ssastry triaged this task as High priority.Sun, Jul 19, 3:51 PM
ssastry added a project: Content-Platform-Team.
ssastry subscribed.

Looks like this is specific to the Phonos extension.

I suspect the phonos extension is stripping the embedded \0 from the parsoid fragment strip marker, so when it returns the result to Parsoid Parsoid can't expand it. Stripping \0 is not safe, because the built-in mediawiki strip markers also use it (the nulls are added so that strip markers can't be spoofed by user-generated wikitext), but maybe phonos is doing something tricky with mediawiki strip markers that is breaking Parsoid.

ssastry@parsoidtest1001:/srv/parsoid-testing$ echo "{{#tag:phonos|<nowiki />test|file=He-Purim.ogg}}" |sudo -u www-data php /srv/mediawiki/multiversion/MWScript.php /srv/parsoid-testing/bin/parse.php --wiki=enwiki --integrated --trace peg
0-[peg]        | ---->    [{"type":"SelfclosingTagTk","name":"template3","attribs":[{"k":["#tag:phonos"],"v":"","srcOffsets":[2,13,13,13]},{"k":[""],"v":[{"type":"PreprocAngleTk","open":"nowiki","extAttrs":" ","close":null,"attribs":[{"k":"mw:contents","v":[""],"srcOffsets":[24,24,24,24]}],"dataParsoid":{"tsr":[14,24],"extTagOffsets":[14,24,10,0],"selfClose":true}},"test"],"srcOffsets":[14,14,14,28]},{"k":["file"],"v":["He","-","Purim.ogg"],"srcOffsets":[29,33,34,46]}],"dataParsoid":{"tsr":[0,48],"src":"{{#tag:phonos|<nowiki />test|file=He-Purim.ogg}}"}}]
2-[peg]        | ---->    [{"type":"SelfclosingTagTk","name":"extension","attribs":[{"k":"typeof","v":"mw:Extension"},{"k":"name","v":"phonos"},{"k":"source","v":"<phonos file=\"He-Purim.ogg\">{{#parsoid\u0000fragment:1}}test</phonos>"},{"k":"options","v":[{"k":"file","v":"He-Purim.ogg","srcOffsets":[8,12,14,26],"vsrc":"He-Purim.ogg","_type_":"Wikimedia\\Parsoid\\Tokens\\KV"}]}],"dataParsoid":{"tsr":[0,64],"stx":"html","src":"<phonos file=\"He-Purim.ogg\">{{#parsoid\u0000fragment:1}}test</phonos>","extTagOffsets":[0,64,28,9]}}]
1-[peg]        | ---->    [{"type":"TagTk","name":"span","attribs":[{"k":"class","v":"ext-phonos"},{"k":"typeof","v":"mw:DOMFragment"}],"dataParsoid":{"tsr":[0,23],"src":"<phonos file=\"He-Purim.ogg\">{{#parsoid\u0000fragment:1}}test</phonos>","html":{"_h":"<span class=\"ext-phonos\" typeof=\"mw:Extension/phonos\" about=\"#mwt2\" data-parsoid='{\"tsr\":[0,64],\"src\":\"&lt;phonos file=\\\"He-Purim.ogg\\\">{{#parsoid\\u0000fragment:1}}test&lt;/phonos>\"}' data-mw='{\"name\":\"phonos\",\"attrs\":{\"file\":\"He-Purim.ogg\"},\"body\":{\"extsrc\":\"{{#parsoid\\u0000fragment:1}}test\"}}'><span data-nosnippet=\"\" id=\"ooui-php-1\" class=\"noexcerpt ext-phonos-PhonosButton oo-ui-widget oo-ui-widget-enabled oo-ui-buttonElement oo-ui-buttonElement-frameless oo-ui-buttonElement-size-medium oo-ui-iconElement oo-ui-labelElement oo-ui-buttonWidget\" data-ooui='{\"_\":\"mw.Phonos.PhonosButton\",\"href\":\"\\/\\/upload.wikimedia.org\\/wikipedia\\/commons\\/transcoded\\/c\\/c9\\/He-Purim.ogg\\/He-Purim.ogg.mp3\",\"rel\":[\"nofollow\"],\"framed\":false,\"icon\":\"volumeUp\",\"label\":{\"html\":\"{{#parsoid\\u0000fragment:1}}test\"},\"data\":{\"ipa\":\"\",\"text\":\"\",\"lang\":\"en\",\"wikibase\":\"\",\"file\":\"He-Purim.ogg\"},\"classes\":[\"noexcerpt\",\"ext-phonos-PhonosButton\"]}'><a role=\"button\" tabindex=\"0\" href=\"//upload.wikimedia.org/wikipedia/commons/transcoded/c/c9/He-Purim.ogg/He-Purim.ogg.mp3\" rel=\"nofollow\" aria-label=\"Play audio\" title=\"Play audio\" class=\"oo-ui-buttonElement-button\"><span class=\"oo-ui-iconElement-icon oo-ui-icon-volumeUp\"></span><span class=\"oo-ui-labelElement-label\">{{#parsoidfragment:1}}test</span><span class=\"oo-ui-indicatorElement-indicator oo-ui-indicatorElement-noIndicator\"></span></a></span><sup class=\"ext-phonos-attribution noexcerpt navigation-not-searchable\"><a href=\"/wiki/File:He-Purim.ogg\" title=\"File:He-Purim.ogg\">ⓘ</a></sup></span>"}},"dataMw":{"name":"phonos","attrs":{"file":"He-Purim.ogg"},"body":{"extsrc":"{{#parsoid\u0000fragment:1}}test"}}},{"type":"EndTagTk","name":"span","attribs":[],"dataParsoid":{"tsr":[23,23]}}]
0-[peg]        | ---->    [{"type":"NlTk","dataParsoid":{"tsr":[48,49]}}]
<p data-parsoid='{"dsr":[0,48,0,0]}'><span class="ext-phonos" typeof="mw:Extension/phonos mw:Transclusion" about="#mwt1" data-parsoid='{"pi":[[{"k":"1"},{"k":"2"},{"k":"3"}]],"dsr":[0,48,null,null]}' data-mw='{"name":"phonos","attrs":{"file":"He-Purim.ogg"},"body":{"extsrc":"{{#parsoid\u0000fragment:1}}test"},"parts":[{"template":{"target":{"wt":"#tag:phonos","function":"tag"},"params":{"1":{"wt":"&lt;nowiki />test"},"2":{"wt":"file=He-Purim.ogg"}},"i":0}}]}'><span data-nosnippet="" id="ooui-php-1" class="noexcerpt ext-phonos-PhonosButton oo-ui-widget oo-ui-widget-enabled oo-ui-buttonElement oo-ui-buttonElement-frameless oo-ui-buttonElement-size-medium oo-ui-iconElement oo-ui-labelElement oo-ui-buttonWidget" data-ooui='{"_":"mw.Phonos.PhonosButton","href":"\/\/upload.wikimedia.org\/wikipedia\/commons\/transcoded\/c\/c9\/He-Purim.ogg\/He-Purim.ogg.mp3","rel":["nofollow"],"framed":false,"icon":"volumeUp","label":{"html":"{{#parsoid\u0000fragment:1}}test"},"data":{"ipa":"","text":"","lang":"en","wikibase":"","file":"He-Purim.ogg"},"classes":["noexcerpt","ext-phonos-PhonosButton"]}'><a role="button" tabindex="0" href="//upload.wikimedia.org/wikipedia/commons/transcoded/c/c9/He-Purim.ogg/He-Purim.ogg.mp3" rel="nofollow" aria-label="Play audio" title="Play audio" class="oo-ui-buttonElement-button"><span class="oo-ui-iconElement-icon oo-ui-icon-volumeUp"></span><span class="oo-ui-labelElement-label">{{#parsoidfragment:1}}test</span><span class="oo-ui-indicatorElement-indicator oo-ui-indicatorElement-noIndicator"></span></a></span><sup class="ext-phonos-attribution noexcerpt navigation-not-searchable"><a href="/wiki/File:He-Purim.ogg" title="File:He-Purim.ogg">ⓘ</a></sup></span></p>

So, what is going on is that Parsoid gets <phonos file=\"He-Purim.ogg\">{{#parsoid\u0000fragment:1}}test</phonos>" from the #tag expansion and dutifully sends that on to legacy because phonos does not have a native Parsoid implementation of the tag, and Phonos doesn't have any inkling of parsoid fragments. It just calls $content = $parser->recursiveTagParseFully( trim( $options['label'] ) ); So, the simplest might be to introduce a Parsoid native implementation of the tag which will then push the content back into Parsoid which will know about the fragment marker and handle it properly.

This bug only impacts the #tag based invocation of phonos because that is when the parsoid fragment markers are introduced in the content that phonos gets and cannot process properly. We should have a fix by end of week if not sooner and we'll backport it.

I don't think we necessarily need to implement a Parsoid-native phonos (although that's one way to address the present issue). The root cause is that DataAccess::parseWikitextWithTitle() (used to expand legacy extension tags) does not have the same PFragment/strip state handling that DataAccess::preprocessWikitext does. We should probably refactor that to (a) accept a PFragment (as opposed to wikitext with an embedded {{#pfragment...}}) and convert it to core strip markers then (b) convert the result-with-strip-markers back to a PFragment (this is already factored out as DataAccess::unstripForParsoid(). In particular, StripState::split() in the core parser is recursive, so I'd expect that the HTML returned by Parser::parseExtensionTagAsTopLevelDoc() will be a 'nowiki' strip marker (the legacy parser's way of representing embedded HTML) which itself contains within it the "parsoid opaque" strip marker corresponding to the {{#pfragment}}.

Somewhat orthogonally (ie, not required to fix this particular bug) I think we could probably do a better job in step (a) of converting <nowiki> which comes from Parsoid not into "parsoid opaque" strip markers but instead into 'nowiki' strip markers (T388819) so they can round-trip through the legacy parser without losing their identity as a <nowiki>.

Using Google with "site:wikipedia.org parsoidfragment" I found that en.Wikipedia is affected as well, eg https://en.wikipedia.org/w/index.php?title=Tonality_diamond&oldid=1365085503 . There I found this example without a nowiki

{{audio|Just perfect fifth on C.mid|{{frac|3|2}}}}

which is rendered by parsoid as "{{#parsoidfragment:4}}3⁄2ⓘ ". In this example the label (second parameter of template:audio) that calls a template is the problem.

That is because of templatestyles which gets the same strip-marker treatment as nowiki in the #tag case. If arbitrary extension tags can be embedded inside <phonos>, then we'll see this problem. We'll get this fixed this week. So, thanks for pointing out that it is not just #tag:phonos ...

I don't think we necessarily need to implement a Parsoid-native phonos (although that's one way to address the present issue).

Any extension that can use wikitext should ideally have a Parsoid-native implementation so that the extension content with wikitext gets Parsoid markup like anywhere else. We shouldn't go through the legacy parser, although that might be a good stop-gap while the native implementation isn't there. We have Parsoid-native implementations for other tags (ref, references, poem, indicator, pre, gallery, imagemap, maplink, mapframe, etc.) that accept wikitext but happened to miss Phonos.

Again, I don't think it needs a Parsoid-native implementation, we just need to make the strip markers work right. The recursive parsing should eventually use Parsoid either via T236812: Parser.php should be split into a base class and a parser implementation or by using the ContentRenderer pathways; you don't need a full rewrite for that.

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

[mediawiki/core@master] Parsoid DataAccess: convert Parsoid fragment markers to/from strip tags

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

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

[mediawiki/services/parsoid@master] Pass extension tags with embedded fragments as a PFragment to core

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

Again, I don't think it needs a Parsoid-native implementation, we just need to make the strip markers work right. The recursive parsing should eventually use Parsoid either via T236812: Parser.php should be split into a base class and a parser implementation or by using the ContentRenderer pathways; you don't need a full rewrite for that.

I am fine with you fixing it this way if that is how you wish to do it. And, there is no "full rewrite" involved. It is a fairly straightforward switch.

For the record, to repeat myself, I think it is useful for us to be consistent about our recommendation to extension authors. If some extension is handling wikitext, do we want them to use the Extension API and target the ExtensionTagHandler interface OR are we moving away from that and going back to the legacy parser code paths OR are we going to keep supporting both and they can use one or the other (which are both very different)? I am just observing that it is helpful to pick a consistent path. But, it is your call. I am just making a recommendation.