Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Page MenuHomePhabricator

Rendering diff from inclusion in a category (visual diff testing)
Closed, ResolvedPublic

Description

The wikitext in these templates all look pretty similar,

https://shn.wikivoyage.org/wiki/%E1%80%91%E1%82%85%E1%80%99%E1%80%BA%E1%80%B8%E1%80%95%E1%80%9C%E1%80%B5%E1%80%90%E1%80%BA%E1%82%89%3AWarningForPageRedirect?useparsoid=0
https://shn.wikivoyage.org/wiki/%E1%80%91%E1%82%85%E1%80%99%E1%80%BA%E1%80%B8%E1%80%95%E1%80%9C%E1%80%B5%E1%80%90%E1%80%BA%E1%82%89%3AWarningForPageRedirect?useparsoid=1

https://hi.wikivoyage.org/w/index.php?title=%E0%A4%B8%E0%A4%BE%E0%A4%81%E0%A4%9A%E0%A4%BE:%E0%A4%AA%E0%A5%83%E0%A4%B7%E0%A5%8D%E0%A4%A0_%E0%A4%AA%E0%A5%81%E0%A4%A8%E0%A4%B0%E0%A5%8D%E0%A4%A8%E0%A4%BF%E0%A4%B0%E0%A5%8D%E0%A4%A6%E0%A5%87%E0%A4%B6%E0%A4%A8_%E0%A4%B9%E0%A5%87%E0%A4%A4%E0%A5%81_%E0%A4%9A%E0%A5%87%E0%A4%A4%E0%A4%BE%E0%A4%B5%E0%A4%A8%E0%A5%80&useparsoid=0
https://hi.wikivoyage.org/w/index.php?title=%E0%A4%B8%E0%A4%BE%E0%A4%81%E0%A4%9A%E0%A4%BE:%E0%A4%AA%E0%A5%83%E0%A4%B7%E0%A5%8D%E0%A4%A0_%E0%A4%AA%E0%A5%81%E0%A4%A8%E0%A4%B0%E0%A5%8D%E0%A4%A8%E0%A4%BF%E0%A4%B0%E0%A5%8D%E0%A4%A6%E0%A5%87%E0%A4%B6%E0%A4%A8_%E0%A4%B9%E0%A5%87%E0%A4%A4%E0%A5%81_%E0%A4%9A%E0%A5%87%E0%A4%A4%E0%A4%BE%E0%A4%B5%E0%A4%A8%E0%A5%80&useparsoid=1

https://hi.wikivoyage.org/wiki/%E0%A4%B8%E0%A4%BE%E0%A4%81%E0%A4%9A%E0%A4%BE%3A%E0%A4%AA%E0%A5%83%E0%A4%B7%E0%A5%8D%E0%A4%A0_%E0%A4%AA%E0%A5%81%E0%A4%A8%E0%A4%B0%E0%A5%8D%E0%A4%A8%E0%A4%BF%E0%A4%B0%E0%A5%8D%E0%A4%A6%E0%A5%87%E0%A4%B6%E0%A4%A8_%E0%A4%B9%E0%A5%87%E0%A4%A4%E0%A5%81_%E0%A4%9A%E0%A5%87%E0%A4%A4%E0%A4%BE%E0%A4%B5%E0%A4%A8%E0%A5%80?useparsoid=0
https://hi.wikivoyage.org/wiki/%E0%A4%B8%E0%A4%BE%E0%A4%81%E0%A4%9A%E0%A4%BE%3A%E0%A4%AA%E0%A5%83%E0%A4%B7%E0%A5%8D%E0%A4%A0_%E0%A4%AA%E0%A5%81%E0%A4%A8%E0%A4%B0%E0%A5%8D%E0%A4%A8%E0%A4%BF%E0%A4%B0%E0%A5%8D%E0%A4%A6%E0%A5%87%E0%A4%B6%E0%A4%A8_%E0%A4%B9%E0%A5%87%E0%A4%A4%E0%A5%81_%E0%A4%9A%E0%A5%87%E0%A4%A4%E0%A4%BE%E0%A4%B5%E0%A4%A8%E0%A5%80?useparsoid=1

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I can reproduce this bug locally -- looks like Parsoid may be doing some eager evaluation of the parser function args which causes it to add the page to the redirect category even when that part of the #ifeq would not get executed.

I think I know what is going on. So, for all tokens, attributes are processed to expand them. That seems like it might not be a problem, but that effectively means that Parsoid is eagerly processing parser-function args. That would work better if we had a Parsoid-aware implementation of parser functions (the one in Parsoid is incomplete and only there for parser test processing). Such an implementation would handle the lazy processing properly.

In any case, there are three problems here: (a) the eager and potentially wasteful eager processing of expensive branches (b) in some cases, the eager processing can incorrectly update metadata (c) we anyway discard the expanded output since we then shuttle the whole template / parser function string back to core to process ... making the eager processing double-wasteful.

So, consider this wikitext:

{{#ifeq:{{#invoke:IsRedirect|IsRedirect|{{{1|{{PAGENAME}}}}}}}|yes|
YES-O-YES {{ombox
| type = move
| text = '''[[{{{1|{{PAGENAME}}}}}]]''' is a redirect, probably due to a page move. {{#ifexpr:{{PAGESINCATEGORY:{{{1|{{PAGENAME}}}}}}}>0|The subpages of this category may need updating to be part of the new category. Look for the {{#ifeq:{{{type|}}}|topic|PartOfTopic|IsPartOf}} template at the end of the articles and change to the new value.[[Category:Categories with articles needing breadcrumbs fixing after page move]]}}
}}[[Category:Categories where article is a redirect]]
|NO-O-NO
}}

Parsoid calls core's preprocessor on these three substrings. The first two are completely unnecessary and wasteful since the last one will generate the desired result. But, the reason the first two calls are being made is because the args are being processed in sub-pipelines, and that sub-pipeline processing in addition to being wasteful is also updating metadata.

{{#invoke:IsRedirect|IsRedirect|{{{1|{{PAGENAME}}}}}}}
{{ombox
| type = move
| text = '''[[{{{1|{{PAGENAME}}}}}]]''' is a redirect, probably due to a page move. {{#ifexpr:{{PAGESINCATEGORY:{{{1|{{PAGENAME}}}}}}}>0|The subpages of this category may need updating to be part of the new category. Look for the {{#ifeq:{{{type|}}}|topic|PartOfTopic|IsPartOf}} template at the end of the articles and change to the new value.[[Category:Categories with articles needing breadcrumbs fixing after page move]]}}
}}
{{#ifeq:{{#invoke:IsRedirect|IsRedirect|{{{1|{{PAGENAME}}}}}}}|yes|
YES-O-YES {{ombox
| type = move
| text = '''[[{{{1|{{PAGENAME}}}}}]]''' is a redirect, probably due to a page move. {{#ifexpr:{{PAGESINCATEGORY:{{{1|{{PAGENAME}}}}}}}>0|The subpages of this category may need updating to be part of the new category. Look for the {{#ifeq:{{{type|}}}|topic|PartOfTopic|IsPartOf}} template at the end of the articles and change to the new value.[[Category:Categories with articles needing breadcrumbs fixing after page move]]}}
}}[[Category:Categories where article is a redirect]]
|NO-O-NO
}}

One possible solution is to suppress attribute processing for template and parser function tokens altogether because of (c) above. Once we have a Parsoid-aware parser function implementation, that will take care of expanding the branches it needs expanded by asking Parsoid for the same.

Aha .. digging deeper, we aren't so stupid after all :slightly_smiling_face: .. the parser pipeline factory already has the attribute expander run after template expansion to avoid this kind of incorrect eager processing. So, this only happens for template tokens where the template target itself is templated (as in this example #ifeq:{{..}}) ... and so, it does need to expand that first attribute to know what the template target is. So, that logic should not be over-eager and only expand the first attribute, not all attributes, as is happening.

Change #1063897 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/services/parsoid@master] WIP: For templated template targets, only expand first attribute

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

Change #1063897 merged by jenkins-bot:

[mediawiki/services/parsoid@master] For templated template targets, only expand first attribute

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

Change #1066683 had a related patch set uploaded (by Isabelle Hurbain-Palatin; author: Isabelle Hurbain-Palatin):

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.20.0-a18

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

Change #1066683 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.20.0-a18

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

ssastry moved this task from To Deploy to To Verify on the Content-Transform-Team-WIP board.