Page MenuHomePhabricator

Broken edit link incorrectly appearing in mobile view API responses
Closed, ResolvedPublic3 Story Points

Description

https://en.m.wikipedia.org/w/api.php?action=mobileview&format=json&page=Barack+Obama&sections=references&prop=text&revision=740641055

If you inspect the response you'll notice in the response:

<a href=\"/w/index.php?title=Special:Badtitle/dummy_title_for_API_calls_set_in_api.php&amp;action=edit&amp;section=41\" title=\"Edit section: Notes\" data-section=\"41\" class=\"mw-ui-icon mw-ui-icon-element mw-ui-icon-edit-enabled edit-page\">Edit</a>

AC

  • Strip the .edit-page links from the output.
    • The link is both broken and isn't used (probably because it's broken).

Developer notes

  • MobileFormatterTest.php assert the existence of these so the tests should be updated to reflect the edit icons not being present

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 23 2016, 6:25 PM
ovasileva triaged this task as Normal priority.Oct 20 2016, 7:50 PM
Jdlrobson raised the priority of this task from Normal to Needs Triage.Oct 20 2016, 7:51 PM
Jdlrobson updated the task description. (Show Details)
phuedx triaged this task as Normal priority.Nov 17 2016, 6:13 PM
phuedx added a subscriber: phuedx.

@Jdlrobson: Any reason why this isn't Normal priority?

I don't know of any user impact due to this issue.

This is still a problem.

Jdlrobson renamed this task from Broken edit link incorrectly appearing in mobile view responses to Broken edit link incorrectly appearing in mobile view API responses.Apr 27 2017, 3:55 PM
Jdlrobson updated the task description. (Show Details)May 16 2017, 3:36 PM
Jdlrobson set the point value for this task to 3.
pmiazga claimed this task.May 18 2017, 1:13 PM
phuedx updated the task description. (Show Details)May 18 2017, 1:19 PM

^ I removed the original AC and contradictory first developer note and replaced 'em with the AC that I think we agreed to.

Change 355168 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Title is not properly set inside ApiMobileView

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

This issue is not as easy as it looks like. There is a problem with cache and handling $title objects in ApiMobileView and ParserOutput. First I thought it's related to article protection but I couldn't reproduce it locally, so I from time to time I was refreshing the production to see the API output - but the production sometimes worked properly (was returning the correct edit link) and sometimes was returning the Badtitle one without any obvious reason. This is not an Article protection issue as it would fail all the time.

Reason: With some debugging and thanks to @Legoktm help we found out that ApiMobileView sometimes (depends on cache) calls ParserOutput::getText() which calls Skin::doEditSectionLink().
The Skin::doEditSectionLink() uses $skin->getTitle() which uses RequestContext::getTitle() which defaults to $wgTitle set inside api.php php file (doesn't use the $title created in ApiMobileView)

Solution: Pass newly created $title object to RequestContext class.

Anyone knows how to re-produce the issue locally?

Instructions to replicate

Piotr's fix

I should note Piotr's change (now merged) retains the edit section links but ensures they point to the correct place.

However....

  • The edit link repeats information provided elsewhere by the same api query
  • Is it useful to include the edit link given that edit links are easy to create from the id property? I suspect apps/other consumers who don't want to show it have to hide the link via CSS.
  • Do we want to strip edit-page from the output or leave that up to the client ?

Let's decide this and implement any decision (if needed) before moving to sign off.

Change 355168 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Title is not properly set inside ApiMobileView

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

Current fix do not strip the link as

  • issue was somewhere else, first I'm trying to fix the root cause - $title object is not passed to Core
  • production sometimes was showing correct link, sometimes incorrect - I decided to keep the old but fixed behavior. First, we thought that it always shows incorrect link
  • stripping is not so easy as ParserOutput doesn't operate on SkinMinerva, we will need another hack here (injecting SkinMinerva) in ApiMobileView.

Stripping edit links would be pretty easy if we pass SkinMinerva to ParserOutput

Jdlrobson closed this task as Resolved.May 24 2017, 5:28 PM

We talked about this in standup and concluded there was more risk in removing the edit-link.
Given existing consumers would have had to deal with the existing edit-section link anyway, removing it might cause them problems.
We can revisit this decision later on.

I have verified the fix (https://phabricator.wikimedia.org/T146491#3287958)

Jdforrester-WMF added a subscriber: Jdforrester-WMF.

Mass-moving all items tagged for MediaWiki 1.30.0-wmf.3, as that was never released; instead, we're using -wmf.4.