Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Page MenuHomePhabricator

CentralNotice: Add ESI test string to base HTML
Closed, ResolvedPublic1 Estimated Story Points

Description

  • Add a test string, controlled via a config variable, inside a comment in the base HTML.
  • Make a config change to set the variable to the test string (see parent task)

Event Timeline

Change 842552 had a related patch set uploaded (by AndyRussG; author: AndyRussG):

[mediawiki/extensions/CentralNotice@master] Add ESI test string to base HTML

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

Change 842552 merged by jenkins-bot:

[mediawiki/extensions/CentralNotice@master] Add ESI test string to base HTML

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

Change 843590 had a related patch set uploaded (by AndyRussG; author: AndyRussG):

[operations/mediawiki-config@master] CentralNotice: Set ESI test string

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

Change 843590 merged by jenkins-bot:

[operations/mediawiki-config@master] CentralNotice: Set ESI test string

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

These changes are live on production wikis (group 0, with train deploys bringing them to groups 1 and 2 this week).

You can see the <!--esi... comment string in the base HTML on the desktop version of mediawiki.org, for example on this page.

However, it turns out that, for the mobile sites of most wikis, there's a setting that's clobbering the comment strings that CentralNotice injects. On the mobile version of the above link, both the ESI comment and the other HTML comment that CentralNotice was already injecting are absent. (We know this isn't a caching issue, since both comments are missing, and you get the same result even if logged in.)

The setting is $wgMinervaEnableSiteNotice, which is set to false for most wikis. The MinervaNeue code that does the clobbering is here. (I did test the CN change with the mobile skin and frontend, but not with that exact setting.)

It seems the place to ask for guidance on how to solve this is Web-Team-Backlog? While the project is still in an early phase of ESI testing (see parent task), the SiteNoticeAfter hook, which CN is using to inject the ESI comment, does seem the right place for that injection. Also, I imagine it would be preferable for upcoming tests to include requests for the mobile site as well as for the desktop one.

Hmmm though I guess, for the purpose of ESI testing, there's no need to add the ESI comment at the same place in the DOM where banners are normally injected.

So, it seems the only somewhat urgent question for Readers Web folks would be: is there an easy way to add the ESI comment to the DOM at this location, via CentralNotice, on the mobile skin? It would be great to have an answer to just this question somewhat prioritized if at all possible... no worries if not ofc. (The parent task has "high" priority, though I don't know the timelines for upcoming ESI tests.)

If no straightforward way to achieve this comes to mind (I also looked for another possible hook, but nothing jumped out) we can add the ESI comment elsewhere for now, and figure out a proper solution later on.

Thx so much in advance and apologies for the bother!! :)

Thanks @AndyRussG. This seems like a good candidate for our request process so that we can gain a bit of context around what is needed here and how we can help. Would you mind filling out https://www.mediawiki.org/wiki/Reading/Web/Request_process with some details?

Change 854091 had a related patch set uploaded (by AndyRussG; author: AndyRussG):

[mediawiki/extensions/CentralNotice@master] Move test ESI injection to BeforePageDisplay hook

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

Thanks @AndyRussG. This seems like a good candidate for our request process so that we can gain a bit of context around what is needed here and how we can help. Would you mind filling out https://www.mediawiki.org/wiki/Reading/Web/Request_process with some details?

Heyy, thanks much @ovasileva and many apologies for the delay! Here is the request task: T322612. It's basically just for review of the attached, updated ESI test patch. We'll submit a separate request for help with a more permanent solution to injecting an ESI banner string later on, as appropriate.

Thanks again! :)

Change 854091 merged by AndyRussG:

[mediawiki/extensions/CentralNotice@master] Move test ESI injection to BeforePageDisplay hook

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

Note: hoping to deploy the latest fix to production pretty soon, possibly this week. This would cause the test ESI comment string to be injected on both desktop and mobile sites (rather than just desktop, as is currently the case). Thanks!