Page MenuHomePhabricator
Create Task
Maniphest T210983

CentralNotice protects CentralNotice templates endlessly each time a template is editted, polluting history and logs
Closed, ResolvedPublic4 Estimated Story Points
Actions

Description

Hi. Please take a look for example at this page history. Every time the template is amended via Special:CentralNoticeBanners CentralNotice issues a new protection to the page. This is unnecesary and unnecessarily pollutes the page history and the protection log. Please have CentralNotice check if the page is protected and its settings before issuing a new protection

In the long term I suggest a dedicated namespace for CentralNotice stuff, protected by default (via $wgNamespaceRestrictions) to the desired levels. Issuing indefinite protections cause Special:ProtectedPages to become increasingly long as well, making it difficult to patrol.

Thank you.

Details

ProjectBranchLines +/-Subject
mediawiki/extensions/CentralNoticemaster+22 -0Don't re-protect banner if it's already protected
mediawiki/coremaster+41 -29Pass READ_LATEST in $flags to Title::loadRestrictions
mediawiki/coremaster+1 -0WikiPage::doUpdateRestrictions checks DB_MASTER
mediawiki/coremaster+14 -8Option to load restrictions from DB_MASTER
Customize query in gerrit

Event Timeline

MarcoAurelio created this task.Dec 3 2018, 9:07 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 3 2018, 9:07 AM
• DStrine added a project: Fundraising-Backlog.Dec 3 2018, 4:58 PM
AndyRussG added a subscriber: AndyRussG.Dec 3 2018, 6:41 PM
Pcoombe added a subscriber: Pcoombe.Dec 3 2018, 7:49 PM
• DStrine moved this task from Triage to Sprint +1 on the Fundraising-Backlog board.Dec 3 2018, 8:43 PM
• DStrine moved this task from Sprint +1 to Current Sprint on the Fundraising-Backlog board.
• DStrine added a project: Fundraising Sprint Window dressing is mostly olive oil.
MarcoAurelio added a comment.Dec 4 2018, 11:59 AM

In addition, it clutters Special:RecentChanges badly as well (which is happening at Meta right now). Thanks.

gerritbot added a subscriber: gerritbot.Dec 7 2018, 8:37 PM

Change 478257 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/extensions/CentralNotice@master] Don't re-protect banner if it's already protected

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

gerritbot added a project: Patch-For-Review.Dec 7 2018, 8:37 PM
Ejegg moved this task from Backlog to Review on the Fundraising Sprint Window dressing is mostly olive oil board.Dec 7 2018, 11:14 PM
• DStrine added a project: Fundraising Sprint XML ate my homework.Dec 11 2018, 9:14 PM
Ejegg moved this task from Backlog to Review on the Fundraising Sprint XML ate my homework board.Dec 11 2018, 10:40 PM
Ejegg added a subscriber: Ejegg.Dec 14 2018, 12:26 AM

I couldn't replicate this either, and was pretty confused about why it would happen, since there's code in WikiPage::doUpdateRestrictions that exits early if the protections are already as they should be. I think it's a result of the way we're saving new banners - we first save a blank page, then do an edit. Now we're adding protection on the blank page, and protecting again on the edit. The checks inside doUpdateRestrictions must be calling on a data store that isn't immediately updated in the production cluster, so it always shows the protection twice.

Looking at that page history I see the two protection actions in the same minute, but for the later edit at 9:15 there's no additional protection in the log.

Ejegg moved this task from Review to Doing on the Fundraising Sprint XML ate my homework board.Dec 14 2018, 4:29 PM
Ejegg added a comment.Dec 14 2018, 4:57 PM

OK, I guess we're NOT actually saving twice in the same form submit, and I see doubled protections in the logs up to two minutes apart. My new theory is that it's due to DB replica delay, since Title::loadRestrictions always goes to the replica. It also caches restrictions, but with a key tied to the revision ID, which should be different on the second save / protect.

MarcoAurelio added a comment.Dec 14 2018, 5:10 PM

Hello. The same happens with {{{text}}} banner content, cfr. https://meta.wikimedia.org/w/index.php?title=MediaWiki:Centralnotice-WMCO_Wikivacaciones2018-text2/es&action=history and protection log). Thanks for investigating this issue. Best regards.

gerritbot added a comment.Dec 14 2018, 8:08 PM

Change 479752 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/core@master] Option to load restrictions from DB_MASTER

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

Ejegg added a comment.Dec 14 2018, 8:25 PM

OK, I was able to recreate this on beta: https://meta.wikimedia.beta.wmflabs.org/w/index.php?title=MediaWiki:Centralnotice-template-TestLoggingBanner&action=history

Ejegg claimed this task.Dec 18 2018, 12:11 AM
Ejegg triaged this task as Medium priority.
gerritbot added a comment.Dec 18 2018, 3:24 PM

Change 479752 merged by jenkins-bot:
[mediawiki/core@master] Option to load restrictions from DB_MASTER

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.12; 2019-01-08).Dec 18 2018, 4:01 PM
gerritbot added a comment.Dec 20 2018, 12:57 AM

Change 480574 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/core@master] Pass READ_LATEST in $flags to Title::loadRestrictions

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

gerritbot added a comment.Dec 20 2018, 4:42 PM

Change 480990 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/core@master] WikiPage::doUpdateRestrictions checks DB_MASTER

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

Ejegg moved this task from Doing to Review on the Fundraising Sprint XML ate my homework board.Dec 20 2018, 4:49 PM
gerritbot added a comment.Jan 7 2019, 5:29 PM

Change 480574 merged by jenkins-bot:
[mediawiki/core@master] Pass READ_LATEST in $flags to Title::loadRestrictions

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

gerritbot added a comment.Jan 7 2019, 8:28 PM

Change 480990 merged by jenkins-bot:
[mediawiki/core@master] WikiPage::doUpdateRestrictions checks DB_MASTER

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

Ejegg moved this task from Review to Pending Deployment on the Fundraising Sprint XML ate my homework board.Jan 8 2019, 1:16 PM
• DStrine added a project: Fundraising Sprint A series of unfortunate event handlers.Jan 8 2019, 9:21 PM
AndyRussG moved this task from Backlog to Pending Deployment on the Fundraising Sprint A series of unfortunate event handlers board.Jan 8 2019, 10:42 PM
gerritbot added a comment.Jan 8 2019, 10:48 PM

Change 478257 abandoned by Ejegg:
Don't re-protect banner if it's already protected

Reason:
Core change was merged!

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

Ejegg moved this task from Pending Deployment to Done on the Fundraising Sprint A series of unfortunate event handlers board.Jan 16 2019, 4:25 PM

This is now deployed to all wikis. Now let's keep an eye on the protection log to see if it stops getting multiple entries

• DStrine added a project: Fundraising Sprint Bert and Ernie's Excellent Adventure.Jan 22 2019, 9:13 PM
AndyRussG moved this task from Backlog to Pending Deployment on the Fundraising Sprint Bert and Ernie's Excellent Adventure board.Jan 22 2019, 9:39 PM
AndyRussG moved this task from Pending Deployment to Done on the Fundraising Sprint Bert and Ernie's Excellent Adventure board.
Ejegg closed this task as Resolved.Jan 22 2019, 10:11 PM

Looking at https://meta.wikimedia.org/wiki/Special:RecentChanges?namespace=8&limit=500&days=7&urlversion=2 I still see plenty of protection for formerly-unprotected translated messages, but I don't see any duplicates like we had before.

Ejegg set the point value for this task to 4.Jul 5 2019, 1:24 AM
Maintenance_bot removed a project: Patch-For-Review.Jul 5 2019, 2:11 AM
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL