Page MenuHomePhabricator

MobileFrontend throws away warnings and other data from action=parse
Closed, ResolvedPublic

Description

Compare the output of the following queries:

{
    "warnings": {
        "parse": {
            "*": "'title' used without 'text', and parsed page properties were requested (did you mean to use 'page' instead of 'title'?)"
        }
    },
    "requestid": "foo",
    "servedby": "mw1200",
    "curtimestamp": "2016-07-27T16:18:39Z",
    "parse": {
        "title": "Foo",
        "pageid": 9132808,
        "text": {
            "*": "\n\n<!-- \nNewPP limit report\nParsed by mw1200\nCached time: 20160727161839\nCache expiry: 2592000\nDynamic content: false\nCPU time usage: 0.000 seconds\nReal time usage: 0.001 seconds\nPreprocessor visited node count: 0/1000000\nPreprocessor generated node count: 0/1500000\nPost\u2010expand include size: 0/2097152 bytes\nTemplate argument size: 0/2097152 bytes\nHighest expansion depth: 0/40\nExpensive parser function count: 0/500\nNumber of Wikibase entities loaded: 0-->\n\n<!-- \nTransclusion expansion time report (%,ms,calls,template)\n100.00%    0.000      1 - -total\n-->\n"
        },
        "langlinks": [],
        "categories": [],
        "links": [],
        "templates": [],
        "images": [],
        "externallinks": [],
        "sections": [],
        "displaytitle": "Foo",
        "iwlinks": [],
        "properties": []
    }
}
{
    "parse": {
        "title": "Foo",
        "pageid": 9132808,
        "text": {
            "*": "<div class=\"mf-section-0\">\n\n\n\n\n</div>"
        },
        "langlinks": [],
        "categories": [],
        "links": [],
        "templates": [],
        "images": [],
        "externallinks": [],
        "sections": [],
        "displaytitle": "Foo",
        "iwlinks": [],
        "properties": []
    }
}

Acceptance criteria

  • MobileFrontend's ApiParseExtender should copy across warnings.

Related Objects

StatusSubtypeAssignedTask
DeclinedNone
ResolvedCatrope
ResolvedSbailey
OpenNone
OpenNone
OpenNone
Resolved GWicke
Resolvedssastry
ResolvedNone
ResolvedDbrant
Resolved bearND
ResolvedMholloway
ResolvedNone
OpenNone
OpenNone
OpenNone
ResolvedArlolra
ResolvedMooeypoo
ResolvedCatrope
Resolved GWicke
OpenNone
Resolved marcoil
Resolved marcoil
Resolved GWicke
ResolvedJdforrester-WMF
DuplicateNone
Resolved bearND
OpenNone
ResolvedArlolra
DeclinedNone
DuplicateNone
DuplicateNone
DeclinedNone
DeclinedNone
DeclinedNone
OpenNone
DeclinedNone
DuplicateNone
ResolvedJdlrobson
Resolvednray

Event Timeline

Anomie raised the priority of this task from to Needs Triage.
Anomie updated the task description. (Show Details)
Anomie added a project: Readers-Web-Backlog.
Anomie added a subscriber: Anomie.
kaldari set Security to None.
Jhernandez triaged this task as Medium priority.
Jhernandez edited projects, added MobileFrontend; removed Readers-Web-Backlog.
Jhernandez added a subscriber: Jhernandez.

Any ideas why @Anomie ? I'm happy to review/merge a patch that fixes this but I'm not familiar with the code. I assume this is a problem with ApiParseExtender ?

In that sense, the "why" is ApiParseExtender::onAPIAfterExecute. It pulls the data out of the ApiResult, resets the ApiResult, then inserts only some of it back in.

Jdlrobson lowered the priority of this task from Medium to Low.Apr 17 2017, 7:46 PM
Jdlrobson added a project: patch-welcome.
Jdlrobson added a subscriber: MaxSem.

The reset seems to have been introduced by rEMFR7f93cbec7d1f: Added the ability to enable expandable sections in API. @MaxSem was this done for any particular reason?
Adding this back in doesn't seem controversial.. just needs someone to provide a patch.

That patch didn't introduce it, it just moved it around.

Don't remember why is was there in the first place.

Its kind of unclear to me if just earnings should be copied over, or if it should loop through all the returned data and copy everything there over .

I think the implication here is that all data should be copied, buy the gci task isnt super clear on that point.

Everything should be copied over, not just the warnings.

Change 473854 had a related patch set uploaded (by Pjht; owner: Pjht):
[mediawiki/extensions/MobileFrontend@master] Fix T86210

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

Change 473854 had a related patch set uploaded (by Jdlrobson; owner: Pjht):
[mediawiki/extensions/MobileFrontend@master] MobileFrontend throws away warnings and other data from action=parse

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

Change 473854 had a related patch set uploaded (by Pjht; owner: Pjht):
[mediawiki/extensions/MobileFrontend@master] Fix T86210

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

Change 473854 had a related patch set uploaded (by Jdlrobson; owner: Pjht):
[mediawiki/extensions/MobileFrontend@master] Copy all result fields in APIParseExtender::onAPIAfterExecute()

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

Change 473854 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Copy all result fields in APIParseExtender::onAPIAfterExecute()

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

Change 475572 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Simplify APIParseExtender

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

Change 475572 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Simplify APIParseExtender

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

Am now seeing warnings in the output (as intended)