Page MenuHomePhabricator

Hovercards wastes API call and "TypeError re.query is undefined" on external links and other links with no title
Closed, ResolvedPublic

Description

Popups makes API queries for external links like
<a class="external text" href="https://git.wikimedia.org/tree/mediawiki%2Fextensions%2FFlow">
, and then ext.popups.renderer.article.js
errors on line 60 accessing re.query.pages.

Hover over an external link on mediawiki.org with console open, see this error, and in Net tab notice the API request

https://www.mediawiki.org/w/api.php?action=query&format=json&prop=extracts%7Cpageimages%7Crevisions%7Cinfo&redirects=true&exintro=true&exsentences=2&explaintext=true&piprop=thumbnail&pithumbsize=300&rvprop=timestamp&inprop=watched&indexpageids=true&titles=

note titles is empty, so it's a pointless API request.

The fix could involve:

  1. Check link.data( 'title' ) is not empty. Better yet (?), don't make the API request if mw.Title.newFromText( title ) === null,

since that will be null for a blank or otherwise invalid title, and an invalid title is unlikely to have useful extracts and pageimages info.

  1. Test re.query before reaching inside it.
  1. Maybe Popups could skip external links altogether, either by looking at href or by adding ".external" to IGNORE_CLASSES or both. I don't know if an external link on a wiki page could have a title attr that's meaningful in the current wiki.

But I'm confused because the test for whether or not to show a popup seems duplicated in setupTriggers(), removeTooltips(), and mw.popups.render.render(), and setupTriggers() already intends to ignore links with empty titles. The find() in setupTriggers() searches for, in part,

a:not([title=""])

but that excludes links with empty titles while including links that have no title at all (such as external links), so:

  1. I think setupTriggers() should find() 'a' + notSelector + '[title]:not([title=""])'

This entire selector string should be set up once in mw.popups.SELECTOR, and shared with removeTooltips() instead of the implementation detail IGNORE_CLASSES.

Also see my comment on bug 65425.

To avoid these bugs ext.popups should eventually have a set of qunit tests that assert all the kinds of links it should ignore and all the ones which it should process.


Version: master
Severity: normal

Details

Reference
bz65929

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:17 AM
bzimport added a project: Page-Previews.
bzimport set Reference to bz65929.
bzimport added a subscriber: Unknown Object (MLST).

Change 137268 had a related patch set uploaded by Prtksxna:
core: Ignore '.external' links

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

Change 134266 had a related patch set uploaded by Prtksxna:
core: Only run the selector for the a elements once

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

Change 137268 merged by jenkins-bot:
core: Ignore '.external' links

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

All patches mentioned in this report were merged or abandoned - is there more work left to do here (if yes: please reset the bug report status to NEW or ASSIGNED), or can you close this ticket as RESOLVED FIXED?

Prtksxna moved this task from Backlog to Archive on the Page-Previews board.Dec 3 2014, 3:15 AM
Restricted Application added a subscriber: StudiesWorld. · View Herald TranscriptJan 28 2016, 5:56 PM