Page MenuHomePhabricator

Reference popups not working with unexpected URL parameters
Closed, ResolvedPublic5 Story Points

Description

Motivation
Article pages sometimes contain extra parameters, for example when you look at an older revision coming from the history.
Right now, reference previews do not work whenever the URL has any other parameter but the title parameter.

Bug example

https://en.wikipedia.beta.wmflabs.org/wiki/Dog?purge=now

This seems due to the way the title parser is used. It will return undefined in the above case.

https://gerrit.wikimedia.org/g/mediawiki/extensions/Popups/+/46cda9fa44c8aee99856606be89cc23187a7d94a/src/title.js#38

that clashes with

https://gerrit.wikimedia.org/g/mediawiki/extensions/Popups/+/46cda9fa44c8aee99856606be89cc23187a7d94a/src/index.js#237

Acceptance Criteria

  • No matter the extra parameters of the page URL, (even if it is more than one), reference previews work.

Notes

  • The behavior does not affect page previews.
  • It should be checked back with Readers-Web-Backlog team how the current implementation came into existence.

Event Timeline

Lea_WMDE updated the task description. (Show Details)Jan 29 2019, 2:10 PM
Lea_WMDE set the point value for this task to 5.
Lea_WMDE moved this task from In preparation to Ready for pickup on the Reference Previews board.

@Niedzielski do you know if there's a reason for that getTitle() does not return a valid title when there are other parameters in the link URL than title=?

See https://gerrit.wikimedia.org/g/mediawiki/extensions/Popups/+/ce2165fb96152134aee650b23a550cbb90de9584/src/title.js#43

@Niedzielski do you know if there's a reason for that getTitle() does not return a valid title when there are other parameters in the link URL than title=?
See https://gerrit.wikimedia.org/g/mediawiki/extensions/Popups/+/ce2165fb96152134aee650b23a550cbb90de9584/src/title.js#43

Ok I just found at least one reason and that's the [edit] links next to section headings. They would trigger a page preview popup ( to the own page though ). I wonder what other examples there are and if there's another way to avoid these things.

@Niedzielski do you know if there's a reason for that getTitle() does not return a valid title when there are other parameters in the link URL than title=?

I do not know. There's a test case for multiple query parameters but no explanation. Perhaps, it's to avoid showing previews for any usage of the action API. E.g., https://en.wikipedia.org/w/index.php?title=Barack_Obama&action=history.

Change 489746 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Popups@master] Teach the title parser to always accept self-links

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

thiemowmde triaged this task as Normal priority.Feb 11 2019, 6:24 PM
thiemowmde moved this task from Sprint Backlog to Review on the WMDE-QWERTY-Sprint-2019-02-06 board.

I explored a bunch of ideas to get a grip on this issue. Finding some solution isn't that hard, but I want a solution that is sustainable and guaranteed to not mess with regular page previews. It should especially not make T198652 worse.

  • As far as I can tell the title parser was intentionally build as restricted as it is. It should only accept links it absolutely must accept.
  • One idea I had was to create a whitelist of URL parameters the title parser is allowed to ignore. We would need a list of use-cases where we want reference previews to be shown, and a (hopefully) complete list of use-cases where we do not want page previews to be shown. I tried, but this gets somewhat bonkers and unmaintainable pretty quickly.
  • I believe the gold solution is to detect self-links, as discussed in T198652.

The patch https://gerrit.wikimedia.org/r/489746 tries to keep the best of both worlds: The title parser is still very strict and only accepts links it absolutely must accept. Self-links are only detected as self-links when they are really identical. No parameters are ignored. Even their order is relevant. Links like the [edit] section links are not self-links, even if they point to the same title.

Change 491002 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/Popups@master] [WIP] Alternative way of detecting self-links

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

Change 489746 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Teach the title parser to always accept self-links

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

Lea_WMDE closed this task as Resolved.Feb 20 2019, 9:09 AM
Lea_WMDE claimed this task.
Lea_WMDE moved this task from Demo to Done on the WMDE-QWERTY-Sprint-2019-02-06 board.

Change 491002 abandoned by Thiemo Kreuz (WMDE):
Alternative way of detecting self-links

Reason:
The effectively only reason for this possible rewrite have been concerns with the browser compatibility, namely Internet Explorer 11 . This doesn't seem to be an issue.

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