Page MenuHomePhabricator

Sitenotice hide button is displayed even if there is no sitenotice
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
A [hide] button appears even if there's no sitenotice shown.

image.png (472×1 px, 160 KB)

What should have happened instead?:
There should be no [hide] button when there's no sitenotice.

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):
I've been able to see the issue on en, pl, de and fr wikis so that it's likely not wiki-specific and doesn't depend on pages in MediaWiki namespace.

Event Timeline

The same on itwiki and mediawiki, It's been happening for two days.

This is extra confusing for users when there is a CentralNotice displayed, since it might look like it would hide the CentralNotice but instead it does nothing.

en.wikipedia.org_wiki_Main_Page.png (543×1 px, 180 KB)

It's possible that this change affected the order in which the various handlers for the SiteNoticeAfter hook run. If so, there's a pretty good chance that applying the same kind of change in CentralNotice – https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralNotice/+/961873 – will fix it next week.

As far I know the documentation for hooks does not say something about the order of hook handler execution or if there is a guarded order. Often the order depends on the order of extension load wfLoadExtensions()
Hooks without the HookHandlers attribute in extension.json are treated as "legacy style handler" and added to wgHooks. It seems the legacy hooks are running first.

At the moment the order in CommonsSettings.php is that DismissableSiteNotice gets loaded first (line 2015, while CentralNotice is loaded in 2150).

As CentralNoticeHooks::onSiteNoticeAfter added a constant value to the $notice, DismissableSiteNotice now says there is something to dismiss.
Also Collection, GrowthExperiments and WikidataPageBanner are implements the hook and possible adding things.

Change 962116 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/extensions/DismissableSiteNotice@master] Ignore <!--comment--> only site notices

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

Some options for this task:

  • Revert the patch set from T346486, it does not have dependencies and the old code still works. This is not forward-looking as the task needs fixing at some time as legacy hooks would not supported all the time.
  • Change the execution order of the hooks, the order is defined on the array_merge in HookContainer::getHandlers, has side effects on all hooks
  • Move use of $wgHooks in CentralNotice to HookContainer::register (something as part of T331602), that would work when the load order remains, not in cases for different load order of the both extensions
  • Fix T259903: Merge DismissableSiteNotice extension into core
  • Ignore comments when determine if the sitenotice is empty, like done in the linked patch set above, extension can still break with the other extensions depending on load order
  • Try to look for <div id="localNotice" and just wrap that.
  • Create own hook just for the local sitenotice
  • ...

Thanks for looking into it.

Out of these options, I would support the new hook just for the local sitenotice, or T259903: Merge DismissableSiteNotice extension into core. I'd be happy to review patches if anyone works on these. Everything else would work as well, but those options seem like they'd make the code worse.

For now I'll merge your patch to ignore comments, as it makes things worse in just one small place :) and it still seems better than reverting the patch and continuing to rely on the order of hook handlers.

Change 962116 merged by jenkins-bot:

[mediawiki/extensions/DismissableSiteNotice@master] Ignore <!--comment--> only site notices

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

matmarex assigned this task to Umherirrender.

Works correctly on beta – https://de.wikipedia.beta.wmflabs.org/ (which has no notice) no longer displays the button, and https://en.wikipedia.beta.wmflabs.org/ (which has a notice) still displays the button as expected.

The change will be deployed to Wikimedia wikis next week, Tuesday–Thursday, per the usual schedule (unless someone volunteers to backport it).

Change 962212 had a related patch set uploaded (by Bartosz Dziewoński; author: Umherirrender):

[mediawiki/extensions/DismissableSiteNotice@wmf/1.41.0-wmf.28] Ignore <!--comment--> only site notices

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

Change 962212 merged by jenkins-bot:

[mediawiki/extensions/DismissableSiteNotice@wmf/1.41.0-wmf.28] Ignore <!--comment--> only site notices

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

Mentioned in SAL (#wikimedia-operations) [2023-10-02T21:07:45Z] <kindrobot@deploy2002> Started scap: Backport for [[gerrit:962212|Ignore <!--comment--> only site notices (T347645)]], [[gerrit:962213|HookUtils: Fix checking page props (T347878)]], [[gerrit:962214|Fix diff title escaping (T347578)]], [[gerrit:962215|Diff: Add missing .mw-diff-inline-moved selector]]

Mentioned in SAL (#wikimedia-operations) [2023-10-02T21:08:59Z] <kindrobot@deploy2002> kindrobot and matmarex: Backport for [[gerrit:962212|Ignore <!--comment--> only site notices (T347645)]], [[gerrit:962213|HookUtils: Fix checking page props (T347878)]], [[gerrit:962214|Fix diff title escaping (T347578)]], [[gerrit:962215|Diff: Add missing .mw-diff-inline-moved selector]] synced to the testservers (https://wikitech.wikimedia.org/wiki/Mwdebug)

Mentioned in SAL (#wikimedia-operations) [2023-10-02T21:17:51Z] <kindrobot@deploy2002> Finished scap: Backport for [[gerrit:962212|Ignore <!--comment--> only site notices (T347645)]], [[gerrit:962213|HookUtils: Fix checking page props (T347878)]], [[gerrit:962214|Fix diff title escaping (T347578)]], [[gerrit:962215|Diff: Add missing .mw-diff-inline-moved selector]] (duration: 10m 06s)

Backported, this is fixed on all Wikimedia wikis now.