Page MenuHomePhabricator

Red links displayed as regular links when Restbase enabled.
Closed, ResolvedPublic2 Estimated Story Points



When Restbase is enabled, red links in an article are displayed as blue, and produce 404 errors when clicked.

Steps to reproduce

  1. Search for 'Staraya Ladoga'
  2. go to 'Sights and landmarks' - in the third paragraph there is a blue link 'Yelena River'
  3. click on the link - the app either crashes or displays 'This page does not exist'

Expected results

  • 'Yelena River' should not be a link, and should not be clickable.

Actual results

The app crashes or displays a page 'This page does not exist' :

Environments observed

App version: 2.1.134-alpha-2015-11-20
Android OS versions: Android 5.1.1
Device model: Nexus 5
Device language: English

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I can't reproduce this on my device (Android 6, Nexus 5, version 2.1.134-alpha-2015-11-20). The link does not show in the paragraph.

Dbrant renamed this task from [2.1.134-alpha-2015-11-20 Regression] Crash on red links that are displayed as valid links to Red links displayed as regular links when Restbase enabled..Nov 24 2015, 3:44 PM
Dbrant updated the task description. (Show Details)
Dbrant set Security to None.
Dbrant added a subscriber: Dbrant.Nov 24 2015, 3:50 PM

This is an unfortunate limitation of Parsoid, wherein it doesn't distinguish between redlinks and normal links in its output, so the app has no way of knowing which is which. T39902

Until T39902 is resolved, we could add an API call in the content service to get the list of missing pages and handle them appropriately, since the workaround is already in place in the API for Visual Editor:

"links": {
  "missing": [
  "Module:Location map/data/Russia Leningrad Oblast",
  "Erik Haakonsson",
  "Vodskaya pyatina",
  "Yelena River",
  "Ivan Ivanov (artist)",
  "Piotr Fomin"

I'd rather do this than roll the content service out to production with dead links.

Mholloway triaged this task as High priority.Mar 2 2016, 11:48 PM

Change 274638 had a related patch set uploaded (by Mholloway):
Hide redlinks

Mholloway claimed this task.Mar 3 2016, 8:30 PM
Mholloway moved this task from Incoming to Doing on the Mobile-Content-Service board.
MBinder_WMF set the point value for this task to 2.

Change 274638 merged by Mobrovac:
Hide links to missing pages

Dbrant updated the task description. (Show Details)Mar 7 2016, 7:52 PM

I repeated the test with the 2016-03-07 release of the app. 'Yelena River' is still a clickable link, and I'm getting 'An unknown error occurred' screen instead of 'This page does not exist' screen. The link actually never loads the article but when clicking 'Read full article' that's when i get the error.

Hi @Nicholas.tsg, this is actually a server-side change that was *just* deployed (and is now being un-deployed because of a problem). Sorry, I probably moved it to the QA column prematurely. I'll update here when it's ready for testing.

Deskana removed a subscriber: Deskana.Mar 10 2016, 1:15 AM

This patch was reverted because it caused excessive server load in production. See T129237: Investigate server flapping after 3/7/2016 deploy.

Change 278403 had a related patch set uploaded (by Mholloway):
Hide links to missing pages, Take 2

Change 278403 merged by Mobrovac:
Hide links to missing pages, Take 2

I had to revert the new patch in production since it still caused the server to flap (as well as contained a separate issue causing a 500 error, though that's easily fixable).

On the second patch, Mobrovac wrote:

In general, the likely problem might be that all of the transforms that are done take a long time, so they prolong the event loop and come to a point of not allowing new requests to be accepted.
TL;DR this should be good enough(TM), but you will need to roll it out carefully. In the long run, we should probably think about a way to have a multi-step transform pass to allow I/O events to happen in time.

I guess the next step is to improve the efficiency of our transforms; it might be a blocker for this and is a good idea in any case.

Mholloway added a comment.EditedMar 25 2016, 8:27 PM

Reverting in the dev branch needs a +2, btw:

Mholloway removed Mholloway as the assignee of this task.Mar 28 2016, 6:50 PM

An user notified me of this problem at a edit-a-thon today. What is the status of this?

This is still the case. I’m using the Wikipedia for Android app on Samsung Galaxy S5 mini (version 2.4.157-r-2016-09-28).

bearND added a subscriber: bearND.Oct 8 2016, 4:37 PM

This is still blocked on T39902: RFC: Implement rendering of redlinks in Parsoid HTML as post-processor. Looks like something is finally happening there.

@Mholloway I don't think this warrants to be a child of the Mobile-section 2.0 task since we want to have this fixed for 1.0 as well.

Change 344546 had a related patch set uploaded (by Mholloway):
[mediawiki/services/mobileapps@master] Mark up missing links in Parsoid article HTML

Change 347999 had a related patch set uploaded (by Mholloway):
[mediawiki/core@master] ApiQueryLinks: add query parameter to filter by exists or !exists

+1 from OTRS ticket 9863129

Change 347999 abandoned by Mholloway:
ApiQueryLinks: add query parameter to filter by exists or !exists

It seems enthusiasm for this is muted. Given that the app's issue is being solved by other means, why don't I abandon this. Maybe I'll extract and expand the new unit tests to resubmit separately another day.

Change 344546 abandoned by Mholloway:
Mark up missing links in Parsoid article HTML

T39902 where are you

Good news! The Parsoid redlink markup feature has been deployed. I am still seeing missing links in production, but when working with a local RESTBase instance, redlinks are marked up in Parsoid HTML and removed by MCS , which tells me that in production we now just need to wait for old content to age out of cache.

Once the case in the description is verified fixed in production I'd say we can call this resolved.

...scratch that. Only deployed to beta cluster, which is also consistent with those results. Watch this space.

Change 361507 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[apps/android/wikipedia@master] Update MCS content format versions for mobile-sections and definitions

Change 361507 merged by jenkins-bot:
[apps/android/wikipedia@master] Update MCS content format versions for mobile-sections and definitions

Mholloway closed this task as Resolved.Jul 6 2017, 7:26 PM