Maniphest T203716

Duplicate mw-parser-output elements in action=parse API output
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Jdlrobson
	Sep 6 2018, 9:00 PM

Description

Visiting https://en.wikipedia.org/w/api.php?formatversion=2&format=json&action=parse&prop=text|modules|langlinks&page=Spain&useskin=vector in an incognito window I see the content is double wrapped with two div.parser-output elements.

{"warnings":{"parse":{"warnings":"Property \"modules\" was set but not \"jsconfigvars\" or \"encodedjsconfigvars\". Configuration variables are necessary for proper module usage."}},"parse":{"title":"Spain","pageid":26667,"text":"<div class=\"mw-parser-output\"><div class=\"mw-parser-output\">

Expected: it should not be double wrapped. There should only be one of these elements.

This breaks various client expectations (including a tool reading web use for QA)

Details

Subject	Repo	Branch	Lines +/-
Revert "Unwrap HTML loaded from parser cache"	mediawiki/core	master	+0 -67
Unwrap HTML loaded from parser cache	mediawiki/core	master	+67 -0
Unwrap HTML loaded from parser cache	mediawiki/core	wmf/1.32.0-wmf.20	+67 -0

Customize query in gerrit

Related Objects

Mentioned In: T203566: ParserOutput cache incompatibility errors in 1.32.0-wmf.20
T200792: [EPIC] Run A/B test on page issues (Farsi, Japanese, Russian, English)
T203567: Warning: Invalid operand type was used: implode() expects a container as one of the arguments in /srv/mediawiki/php-1.32.0-wmf.20/includes/parser/ParserOutput.php on line 428
Mentioned Here: rMW0dc7ba02b448: Apply content wrapping in ParserOutput::getText()

Event Timeline

Jdlrobson created this task.Sep 6 2018, 9:00 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 6 2018, 9:00 PM

Jdlrobson updated the task description. (Show Details)Sep 6 2018, 9:01 PM

Jdlrobson triaged this task as High priority.Sep 6 2018, 9:11 PM

Jdlrobson updated the task description. (Show Details)

Tgr mentioned this in T203567: Warning: Invalid operand type was used: implode() expects a container as one of the arguments in /srv/mediawiki/php-1.32.0-wmf.20/includes/parser/ParserOutput.php on line 428.Sep 6 2018, 9:23 PM

Got cached slightly before the deploy (20180906110544). Pre rMW0dc7ba02b448: Apply content wrapping in ParserOutput::getText() the wrapper div was added on render and removed on getText() if unwrap=false was specified. Now the wrapper div is added in getText() unless unwrap=false is specified. So with a ParserOutput cached before the code change there will be double-wrapping with unwrap=false and single-wrapping with unwrap=true. We should probably just restore the old unwrap logic and run it unconditionally until old parser cache records are all expired.

Tgr added a project: Multi-Content-Revisions (MCR-SDC File Caption Support - phase 2).Sep 6 2018, 9:29 PM

• Tbayer mentioned this in T200792: [EPIC] Run A/B test on page issues (Farsi, Japanese, Russian, English).Sep 6 2018, 10:57 PM

@Jdlrobson any thoughts about the impact of this bug? Does it break clients, or the HTML wrangling in the mobile APIs, or is it just a QA inconvenience?

Tgr mentioned this in T203566: ParserOutput cache incompatibility errors in 1.32.0-wmf.20.Sep 7 2018, 7:41 AM

Change 458732 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Unwrap HTML loaded from parser cache

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

gerritbot added a project: Patch-For-Review.Sep 7 2018, 8:47 AM

Thanks for the patch, @Tgr!

Perhaps the unwrap flag should be deprecated when that code is removed, and ApiMobileView should be changed to use [ 'wrapperDivClass' => '' ] instead.

Anomie moved this task from Unsorted to Non-core-API stuff on the MediaWiki-Action-API board.Sep 7 2018, 1:16 PM

In T203716#4565268, @Tgr wrote:

@Jdlrobson any thoughts about the impact of this bug? Does it break clients, or the HTML wrangling in the mobile APIs, or is it just a QA inconvenience?

It's hard to say.. I suspect it could impact TextExtracts and possibly the mobile content server (although I think they are fully using Parsoid. Does iOS use the mediawiki API? It's more likely to impact unofficial mediawiki clients.

Right now, I'm only seeing breakages in the MobileFrontend ContentProvider which we use to test production wiki content, which we can work around provided we run action=purge on the page we want to test on.

So with a ParserOutput cached before the code change there will be double-wrapping with unwrap=false and single-wrapping with unwrap=true.

What I don't understand is that for a PO cached with the old logic, getWrapperDivClass should return an empty string, disabling wrapping. Any given PO should have *either* the wrapper in the text *or* a wrapper class triggering the wrapping code, never both.

In T203716#4566829, @daniel wrote:

What I don't understand is that for a PO cached with the old logic, getWrapperDivClass should return an empty string, disabling wrapping. Any given PO should have *either* the wrapper in the text *or* a wrapper class triggering the wrapping code, never both.

Ah, I think I see. The PO was probably fine, it was a wmf.19-style PO that would have had getWrapperDivClass returning an empty string. But your change to ApiParse has it overriding that by passing 'wrapperDivClass' (where formerly it had used 'unwrap'), resulting in the double wrapping.

Gergő's patch should still fix it (and will fix the other bugs caused by 'unwrap' no longer working for wmf.19 POs), although the test being added there is bogus and it still needs the fix I mentioned in code review.

Change 458732 merged by jenkins-bot:
[mediawiki/core@master] Unwrap HTML loaded from parser cache

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

This should be fixed in master with the merge of that patch. But probably someone will want to backport it instead of waiting for wmf.22.

ReleaseTaggerBot added a project: MW-1.32-notes (WMF-deploy-2018-09-18 (1.32.0-wmf.22)).Sep 7 2018, 6:00 PM

CCicalese_WMF moved this task from Backlog to In Progress on the Multi-Content-Revisions (MCR-SDC File Caption Support - phase 2) board.Sep 7 2018, 10:55 PM

Kf8 subscribed.Sep 9 2018, 1:08 AM

Change 459547 had a related patch set uploaded (by Anomie; owner: Gergő Tisza):
[mediawiki/core@wmf/1.32.0-wmf.20] Unwrap HTML loaded from parser cache

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

daniel moved this task from In Progress to Needs Review on the Multi-Content-Revisions (MCR-SDC File Caption Support - phase 2) board.Sep 10 2018, 1:34 PM

CCicalese_WMF edited projects, added Multi-Content-Revisions; removed Multi-Content-Revisions (MCR-SDC File Caption Support - phase 2).Sep 10 2018, 2:11 PM

Change 459547 merged by jenkins-bot:
[mediawiki/core@wmf/1.32.0-wmf.20] Unwrap HTML loaded from parser cache

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

CCicalese_WMF moved this task from Inbox to In Progress on the Multi-Content-Revisions board.Sep 10 2018, 2:26 PM

Mentioned in SAL (#wikimedia-operations) [2018-09-10T14:28:09Z] <anomie@deploy1001> Synchronized php-1.32.0-wmf.20/includes/parser/ParserOutput.php: Backport for T203716 (duration: 00m 50s)

Fixed and backported.

Anomie reassigned this task from Anomie to Tgr.Sep 10 2018, 2:32 PM

ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-09-04 (1.32.0-wmf.20)); removed MW-1.32-notes (WMF-deploy-2018-09-18 (1.32.0-wmf.22)).Sep 10 2018, 3:00 PM

CCicalese_WMF moved this task from In Progress to Done on the Multi-Content-Revisions board.Sep 18 2018, 10:09 PM

Change 465465 had a related patch set uploaded (by Gergő Tisza; owner: Gergő Tisza):
[mediawiki/core@master] Revert "Unwrap HTML loaded from parser cache"

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

Change 465465 merged by jenkins-bot:
[mediawiki/core@master] Revert "Unwrap HTML loaded from parser cache"

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

ReleaseTaggerBot edited projects, added MW-1.32-notes (WMF-deploy-2018-10-16 (1.32.0-wmf.26)); removed MW-1.32-notes (WMF-deploy-2018-09-04 (1.32.0-wmf.20)).Oct 11 2018, 7:00 PM

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:40 PM

Maintenance_bot removed a project: Patch-For-Review.Oct 16 2020, 6:34 PM