First noticed on "enwiki > Tony Blinken".
I think this just started happening today.
Probably unbreak now / high priority?
May have been caused by https://gerrit.wikimedia.org/r/#/c/350634/ T37247
Mhurd | |
May 12 2017, 12:57 AM |
F8051333: Screen Shot 2017-05-12 at 10.16.29 AM.png | |
May 12 2017, 2:18 PM |
F8051192: Screen Shot 2017-05-12 at 2.59.45 PM.png | |
May 12 2017, 2:00 PM |
F8045666: Screen Shot 2017-05-11 at 5.42.43 PM.png | |
May 12 2017, 12:57 AM |
First noticed on "enwiki > Tony Blinken".
I think this just started happening today.
Probably unbreak now / high priority?
May have been caused by https://gerrit.wikimedia.org/r/#/c/350634/ T37247
Subject | Repo | Branch | Lines +/- | |
---|---|---|---|---|
ParserOptions: Include wrapping class in options hash | mediawiki/core | master | +5 -0 |
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | thcipriani | T163512 MW-1.30.0-wmf.2 deployment blockers | |||
Resolved | Mhurd | T165115 action=mobileview sections delivering a div of class "mw-parser-output" without a matching closing div | |||
Resolved | Mhurd | T165117 First paragraph transform broken | |||
Resolved | • Mholloway | T165119 Lead paragraph shifting is broken (at least in some cases) for mobileview-loaded pages | |||
Resolved | ABorbaWMF | T165161 Text extracts empty for some articles |
@Anomie @Jdlrobson
Hey guys I noticed this funny unbalanced div issue today. Any thoughts?
Looks like the mobileview section parser needs to be updated to take the new div into account. I'll tag MCS as well since this change is coming to Parsoid soon, too.
Ya since this new div isn't section html, it probably just needs to be removed from mobileview section output.
rEMFR263f10759583: Update tests for core change If4eb5bf7 was supposed to take care of it. But it looks like the call to WikiPage::getParserOutput() is using the cached version with the wrapper, which wasn't caught by the unit testing. Options:
You might also have to bump your ApiMobileView cache version constant, or otherwise clear its cache, to clear the bad data.
This is the same problem as T164733. It looks like the change wasn't reverted and rode the train. o_O
I'm still seeing this in the mobileview API output for en:Barack_Obama after a purge and an edit.
There's probably a cache somewhere that's still polluted. The API itself has a level of caching too so you may need to edit several times.
Try cache busting e.g. https://en.wikipedia.org/w/api.php?action=mobileview&format=json&page=Barack_Obama&prop=text§ions=0&cachebust=1
Unfortunately, without an expensive full site Varnish purge from ops we are likely to see these kind of issues for the next 7 days. For mobile web this means section editing is broken on various pages. This is why I was keen to revert the patch in T164733.
Action API responses are not cached unless you use the smaxage parameter. (In general you can avoid Varnish by using the X-Wikimedia-Debug header; there are browser plugins to do that.)
If the API uses the parser cache somehow, then it's cached to up to 30 days (unless someone edits), calling action=purge on the affected page will probably help.
Change 353566 had a related patch set uploaded (by Anomie; owner: Anomie):
[mediawiki/core@master] ParserOptions: Include wrapping class in options hash
Here is a minimal reproduction of the issue:
tgr@terbium:~$ mwscript eval.php --wiki=enwiki > $html = '<div class="mw-parser-output">foo</div>'; > $title = Title::newFromText( 'Barack Obama' ); > $mf = new MobileFormatter( MobileFormatter::wrapHTML( $html ), $title ); > $mf->filterContent(); > return $mf->getText(); <div class="mw-parser-output">foo</div>
filterContent should have called makeSections which should have removed the wrapper but that does not seem to be happening. At a glance there is no caching involved.
All that seems to be happening here is that, originally, the section transformation was made inside the <body> tag, then at the end MobileFormatter::getText() unwrapped it. d154fa36b5c3
changed the transformation to happen inside <div class="mw-parser-output"> instead but nothing removes that element. MobileFormatter::getText() should just be updated to do it.
d154fa36b5c3 is unrelated.
263f10759 attempted to fix this issue by instructing the parser (via ParserOptions) to not include the wrapper. However, WikiPage::getParserOutput() is returning a cached ParserOutput that does include the wrapper despite the passed ParserOptions saying to exclude the wrapper. Hence my suggested fixes in T165115#3257490.
In local testing https://gerrit.wikimedia.org/r/353566, which implements the fourth bullet from that comment, does fix this bug with no additional change to MobileFrontend.
Yeah the action=purge strategy was mentioned in another thread by Jon Robson so I'm guessing this is in the parser cache.
We may have to do something there.
Just to note, although the change was swatted and removed from production, master still has the change.
Also verified tha TTL is about 1 day for most pages, so the cache should be good to go over the weekend.
Not sure how this affects RESTBase
A corresponding change was merged into the Parsoid repo but isn't yet deployed. https://gerrit.wikimedia.org/r/#/c/352711/
ETA: This is as pertaining to Parsoid HTML content stored in mobile-sections*, which won't have been affected (yet). The summary endpoint is already receiving bad content and will likely need a purge/regeneration.
Note the Parsoid change adds a class to the existing <body> tag rather than adding a wrapper. Thus both Flow and VE didn't actually pick up the change (tested on Beta Labs), I have no idea whether MCS will similarly ignore it or not.
Not sure if I understand how ParserCache works but it seems like ApiMobileView::getData calling ParserCache::getKey will result in this change getting ignored (at least for the duration of mobileview API result caching) as that seems to only take into account the parser options which had a non-default value when the page was last saved, so updating the wrapper parser option inside the API does not break the cache.
Sorry for the late reply, but we have truncated the summary and feed tables on Friday, so all is good there.
Why is this ticket still opened? Is this still an issue?
https://gerrit.wikimedia.org/r/#/c/353566/ still has to be merged, right? And possibly the mobileview API cache needs to be purged or split as well.
Please verify whether this affects clients and other parts of the system in production. If it does not, it's not a UBN situation and the ticket's priority should be lowered.
As far as I know, the current situation is that the bugs caused by the wrapper element are fixed by a revert in 1.30.0-wmf.1. They still occur in master, meaning they can be seen in Beta Labs and will happen again when 1.30.0-wmf.2 is rolled out[1] if fixes aren't made and merged by then.
For this bug in particular, https://gerrit.wikimedia.org/r/#/c/353566/ should fix it if it's merged, although the cache maintained by ApiMobileView might continue to contain broken entries.
[1]: Next week (May 23–25), there's no train this week due to the Hackathon.
Change 353566 merged by jenkins-bot:
[mediawiki/core@master] ParserOptions: Include wrapping class in options hash
Should be, yes. For example, https://en.wikipedia.beta.wmflabs.org/w/api.php?action=mobileview&page=Asian+black+bear§ions=all&prop=text|sections looks correct now. Do the Mobile people need to do more testing?
It looks good from my side, but the apps rely on this API and web doesn't so it would be best for them to verify.
Closing for now. We talked about this a bit at the hackathon and my understanding is that the issues should be resolved in master. Will be monitoring for the wmf.2 train.