Page MenuHomePhabricator

Should be possible to disable page previews on certain special pages
Closed, ResolvedPublic3 Story Points

Description

Currently, Page Previews will load and run on all pages though, fortunately for Reading Web, the links that it works on don't tend to feature on pages like Special:UserLogin or Special:Preferences. Page Previews shouldn't be loaded on a limited subset of pages to avoid any ambiguity.

To be clear, this task isn't about limiting requesting previews for pages, e.g. not requesting previews for special pages; it's about limiting the pages that the Page Previews code is loaded on.

AC

  • A configuration variable holds a blacklist of pages that Page Previews must not be loaded on, e.g. wgPopupsPageBlacklist.
  • It defaults to Special:UserLogin and Special:CreateAccount
  • If a user (logged in or out) navigates to a page in the blacklist, then they aren't delivered the PP code.
  • It applies to translated special page names e.g. https://de.m.wikipedia.org/wiki/Spezial:Anmelden

Closed Questions

  • Should this be a blacklist?

@phuedx: Yes. You're absolutely right.

  • Can we use wgContentNamespaces?

@phuedx: I'd like Page Previews to work in NS_SCHEMA for example. As @Jdlrobson says, the number of places where we want this to run far outweighs the number of places where we don't.

  • What should the default value of the blacklist be?

@phuedx: Initially, [ 'Special:UserLogin', 'Special:UserLogout' ].

Before sign off

@phuedx: Done.

  • Add a note to Schema_talk:Popups explaining that data won't be collected from blacklisted pages.

@phuedx: Done.

Testing criteria

Verify that Popups module is not loaded on translated special pages like:

The easiest way to check the popups module is to execute console.log(mw.popups.isEnabled()); in developers console. The expected output is false.

Event Timeline

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

@ovasileva there's some questions from @Vachovec1 which need addressing.

Not my call, but I would have similar questions as @Vachovec1. Aren't previews actually especially useful on list pages like https://en.wikipedia.org/wiki/Special:AllPages , https://en.wikipedia.org/wiki/Special:WhatLinksHere or https://en.wikipedia.org/wiki/Special:NewPages ?

And conversely, what is the underlying goal of this task - what benefits will we see after implementing such a blacklist? Performance gains, in the sense of e.g. Special:UserLogin loading faster?

@Tbayer The underlying goal is to not ship PagePreviews to pages where this feature isn't useful (less bytes shipped, less processing, faster rendering) - as Special:Login or Special:Preferences.

Vachovec1 added a comment.EditedJul 15 2017, 3:40 PM

@Tbayer The underlying goal is to not ship PagePreviews to pages where this feature isn't useful (less bytes shipped, less processing, faster rendering) - as Special:Login or Special:Preferences.

That's understandable. But the list of potentially "blacklisted" pages provided here by @ovasileva needs a careful handling. Some rethinking would probably be in order. No one will dispute, if Page Previews would be banned from pages like Special:Login, Special:Book or Special:Upload. Or from MediaWiki/Template/Modul namespaces. But for example Portals are used as gates to content. It's their main purpose, even. So banning of Page Previews from Portal pages looks senseless to me. There are other pages where banning the Page Previews is undesirable as well (see prevous comments from me and Tbayer for some examples).

@pmiazga: Thanks for clarifying! Have we ever tried to ballpark-estimate the performance decrease (e.g. increase in rendering time) caused by adding the page previews code? That might be interesting to know in general.
That said, we should also be aware that many pages on the above list (in particular, I would guess, those special pages where the removal would be uncontroversial) receive a very small percentage of our overall traffic - happy to run some queries if there is interest in more concrete detail. And in many cases (say Special:PrefixIndex) their rendering time is probably dominated by backend database requests anyway, as they are not cached. So I'm still curious if this change is worth the effort.

I would also think that it can be quite confusing for users if a feature they have come to know and rely on suddenly stops working with no apparent reason, even though the links in question are the same.

@Tbayer The underlying goal is to not ship PagePreviews to pages where this feature isn't useful (less bytes shipped, less processing, faster rendering) - as Special:Login or Special:Preferences.

That's understandable. But the list of potentially "blacklisted" pages provided here by @ovasileva needs a careful handling. Some rethinking would probably be in order. No one will dispute, if Page Previews would be banned from pages like Special:Login, Special:Book or Special:Upload. Or from MediaWiki/Template/Modul namespaces. But for example Portals are used as gates to content. It's their main purpose, even. So banning of Page Previews from Portal pages looks senseless to me. There are other pages where banning the Page Previews is undesirable as well (see prevous comments from me and Tbayer for some examples).

Sorry for the original confusion. I agree that we can make the blacklist a bit more refined. The list provided above is for the first iteration. It seems like there was some confusion from my side on what exactly the list would do (not display previews to certain pages versus not displaying previews on the page and for the page). I will be providing an updated version soon. Feel free to review and comment!

phuedx updated the task description. (Show Details)Jul 17 2017, 10:39 AM

@Tbayer The underlying goal is to not ship PagePreviews to pages where this feature isn't useful (less bytes shipped, less processing, faster rendering) - as Special:Login or Special:Preferences.

This and, generally speaking, there should be as few scripts as possible running on Special:UserLogin and Special:UserLogout. For obvious reasons, these are very sensitive pages.

Jdlrobson renamed this task from Don't load Page Previews on all pages to Should be possible to disable page previews on certain special pages.Jul 17 2017, 9:46 PM
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.

How about we start simple - let's just maintain a configurable blacklist and add the uncontroversial Special:UserLogin and Special:CreateAccount
We can then easily disable other pages on a case by case basis one at a time.

Let's keep discussion about where not to show page previews until another task (T170893) - we need to setup the infrastructure first which seems uncontroversial.

@Jdlrobson - since we're creating the blacklist anyhow, why not include the entire list?

nm, just saw the subtask

Jdlrobson set the point value for this task to 3.Jul 18 2017, 4:31 PM

Change 368472 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/Popups@master] Disable Previews on blacklisted pages

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

@Tbayer just a heads-up that no Previews-related event logging will happen on pages where Previews are disabled. This came up during code review.

phuedx updated the task description. (Show Details)Jul 31 2017, 5:49 AM

I assume this can skip QA as behaviour cannot be easily tested given the two pages Special:UserLogin and Special:CreateAccount contain no links... This can be easily verified by a developer in sign off by checking the ResourceLoader module state with mw.loader.getState('ext.popups')

bmansurov removed bmansurov as the assignee of this task.Jul 31 2017, 6:01 PM
bmansurov added a subscriber: bmansurov.

Change 368472 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Disable Previews on blacklisted pages

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

phuedx updated the task description. (Show Details)Aug 1 2017, 8:35 AM
phuedx updated the task description. (Show Details)
phuedx updated the task description. (Show Details)Aug 1 2017, 8:45 AM
phuedx added a comment.Aug 1 2017, 8:49 AM

I've confirmed that mw.loader.getState( 'ext.popups' ) is "registered" – not "ready" – on the UserLogin and CreateAccount special pages 👍

Note well that T170893: Disable page previews on various special pages covers extending the list.

phuedx closed this task as Resolved.Aug 1 2017, 8:51 AM
phuedx claimed this task.

🎉

I see this task has been closed, but it looks like these two action items from the task description are still open:

Regarding the second item, do we happen to have a ballpark estimate for the percentage of total pageviews affected?

For the record, it has been reported that this change caused T173411. Personally, I'm still curious whether the security and performance gains achieved by this task were worth the effort and added complications.

Jdlrobson reopened this task as Open.Aug 21 2017, 7:32 PM

were worth the effort and added complications.

T173615 is a useful output in my opinion. This should have been a trivial harmless change.

Reopening as the sign off steps have not been followed so this is not actually resolved. Thanks for pointing that out.

phuedx updated the task description. (Show Details)Aug 22 2017, 9:29 AM
phuedx updated the task description. (Show Details)
phuedx updated the task description. (Show Details)Aug 22 2017, 9:50 AM

Can someone review those changes and resolve this task as necessary?

Jdlrobson closed this task as Resolved.Aug 22 2017, 2:46 PM

LGTM

Jdlrobson reopened this task as Open.Aug 23 2017, 5:56 PM
Jdlrobson updated the task description. (Show Details)

As discussed in T170893 there is an issue with the implementation. I've updated acceptance criteria.

Jdlrobson removed phuedx as the assignee of this task.Aug 24 2017, 2:18 PM
pmiazga claimed this task.Aug 24 2017, 5:08 PM

Question: what if someone puts translated title inside config - do we care? Can we assume that configs will always have canonical names?

Change 373688 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] Use canonical name for NS_SPECIAL titles when checking the blacklist

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

Change 373696 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[operations/mediawiki-config@master] Fix incorrect Special:Userlogin name in Popups blacklist

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

Question: what if someone puts translated title inside config - do we care? Can we assume that configs will always have canonical names?

Can you update the inline documentation of the configuration variable to reflect this, if it doesn't already?

Restricted Application added a subscriber: jeblad. · View Herald TranscriptAug 25 2017, 9:04 AM

@phuedx I added an information to extension.json that every page name has to be in canonical form.

Restricted Application added a subscriber: Dereckson. · View Herald TranscriptAug 25 2017, 3:44 PM

Change 373688 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Use canonical name for NS_SPECIAL titles when checking the blacklist

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

jeblad removed a subscriber: jeblad.Aug 25 2017, 9:34 PM
pmiazga added a comment.EditedAug 28 2017, 1:35 PM

There is a config change required after deploying this change to production. @Framawiki - thanks for pointing this out. I'll add Wikimedia-Site-requests tag.

Edit: Jdlrobson already added that tag couple days ago.

pmiazga updated the task description. (Show Details)Aug 28 2017, 5:15 PM

Change 373696 merged by jenkins-bot:
[operations/mediawiki-config@master] Fix incorrect Special:Userlogin name in Popups blacklist

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

Note, since https://gerrit.wikimedia.org/r/373688 is riding the change I was only able to verify that Popups is still disabled on English Wikipedia.
To completely sign this off we'll need to wait until Thursday.

pmiazga removed pmiazga as the assignee of this task.Aug 31 2017, 5:02 PM
Jdlrobson closed this task as Resolved.Aug 31 2017, 6:21 PM
Jdlrobson claimed this task.

A visit to https://he.wikipedia.org/wiki/%D7%9E%D7%99%D7%95%D7%97%D7%93:%D7%9B%D7%A0%D7%99%D7%A1%D7%94_%D7%9C%D7%97%D7%A9%D7%91%D7%95%D7%9F

and debugging:

mw.loader.getState('ext.popups')
registered

shows this is fixed.