Page MenuHomePhabricator
Create Task
Maniphest T274889

Use of ProtectionForm::buildForm hook (used in FlaggedRevsUIHooks::onProtectionForm) was deprecated in MediaWiki 1.36
Closed, ResolvedPublicPRODUCTION ERROR
Actions

Description

Error message
Use of ProtectionForm::buildForm hook (used in FlaggedRevsUIHooks::onProtectionForm) was deprecated in MediaWiki 1.36. [Called from MediaWiki\HookContainer\HookContainer::run]
Stack Trace
from /srv/mediawiki/php-1.36.0-wmf.30/includes/HookContainer/HookContainer.php(140)
#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, string, array)
#1 /srv/mediawiki/php-1.36.0-wmf.30/includes/debug/MWDebug.php(376): trigger_error(string, integer)
#2 /srv/mediawiki/php-1.36.0-wmf.30/includes/debug/MWDebug.php(352): MWDebug::sendRawDeprecated(string, boolean, string)
#3 /srv/mediawiki/php-1.36.0-wmf.30/includes/debug/MWDebug.php(230): MWDebug::deprecatedMsg(string, string, string, integer)
#4 /srv/mediawiki/php-1.36.0-wmf.30/includes/GlobalFunctions.php(1036): MWDebug::deprecated(string, string, string, integer)
#5 /srv/mediawiki/php-1.36.0-wmf.30/includes/HookContainer/HookContainer.php(329): wfDeprecated(string, string, boolean)
#6 /srv/mediawiki/php-1.36.0-wmf.30/includes/HookContainer/HookContainer.php(140): MediaWiki\HookContainer\HookContainer->callLegacyHook(string, array, array, array)
#7 /srv/mediawiki/php-1.36.0-wmf.30/includes/HookContainer/HookRunner.php(3145): MediaWiki\HookContainer\HookContainer->run(string, array)
#8 /srv/mediawiki/php-1.36.0-wmf.30/includes/ProtectionForm.php(499): MediaWiki\HookContainer\HookRunner->onProtectionForm__buildForm(Article, string)
#9 /srv/mediawiki/php-1.36.0-wmf.30/includes/ProtectionForm.php(307): ProtectionForm->buildForm()
#10 /srv/mediawiki/php-1.36.0-wmf.30/includes/ProtectionForm.php(237): ProtectionForm->show()
#11 /srv/mediawiki/php-1.36.0-wmf.30/includes/page/Article.php(1846): ProtectionForm->execute()
#12 /srv/mediawiki/php-1.36.0-wmf.30/includes/actions/ProtectAction.php(47): Article->protect()
#13 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(532): ProtectAction->show()
#14 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(316): MediaWiki->performAction(Article, Title)
#15 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(944): MediaWiki->performRequest()
#16 /srv/mediawiki/php-1.36.0-wmf.30/includes/MediaWiki.php(548): MediaWiki->main()
#17 /srv/mediawiki/php-1.36.0-wmf.30/index.php(53): MediaWiki->run()
#18 /srv/mediawiki/php-1.36.0-wmf.30/index.php(46): wfIndexMain()
#19 /srv/mediawiki/w/index.php(3): require(string)
#20 {main}
Impact

logspam

Notes

Related: T236218: ProtectionForm::buildForm is deprecated in favor of ProtectionForm::addFormFields

Details

Request ID
YCva9iUHmLwb-tG45pqPhAAAAEg
Request URL
pt.wikinews.org/w/index.php?title=Atl%C3%A9tico_Paranaense_se_classifica_e_faz_final_in%C3%A9dita_na_Libertadores&action=protect
Related Changes in Gerrit:
SubjectRepoBranchLines +/-
Use OOUI for action=protect formmediawiki/extensions/FlaggedRevsmaster+93 -125
Silent deprecate ProtectionForm::buildFormmediawiki/corewmf/1.36.0-wmf.31+1 -1
Silent deprecate ProtectionForm::buildFormmediawiki/coremaster+1 -1
Silent deprecate ProtectionForm::buildFormmediawiki/corewmf/1.36.0-wmf.30+1 -1
Temporarily disable FlaggedRevs ProtectionForm::buildForm hookmediawiki/extensions/FlaggedRevsmaster+0 -1
Customize query in gerrit

Related Objects
Search...

Event Timeline

• mmodell created this task.Feb 16 2021, 3:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 16 2021, 3:00 PM
• mmodell moved this task from Untriaged to Feb 2021 on the Wikimedia-production-error board.Feb 16 2021, 3:01 PM
• mmodell triaged this task as Unbreak Now! priority.Feb 16 2021, 3:06 PM
• mmodell added a parent task: T236218: ProtectionForm::buildForm is deprecated in favor of ProtectionForm::addFormFields.
• mmodell added a parent task: T271345: 1.36.0-wmf.31 deployment blockers.

made this a blocker for wmf.31, it's filtered in logstash for now.

Kizule subscribed.Feb 16 2021, 3:12 PM

We have a patch for this already. https://gerrit.wikimedia.org/r/c/663327

Kizule mentioned this in T236218: ProtectionForm::buildForm is deprecated in favor of ProtectionForm::addFormFields.Feb 16 2021, 3:16 PM
Reedy subscribed.Feb 16 2021, 4:31 PM

As usual, I have to wonder why something was marked as deprecated when it was still in use in WMF production...

gerritbot added a comment.Feb 16 2021, 4:59 PM

Change 663327 had a related patch set uploaded (by Aklapper; owner: BrandonXLF):
[mediawiki/extensions/FlaggedRevs@master] Use OOUI for action=protect form

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

gerritbot added a project: Patch-For-Review.Feb 16 2021, 4:59 PM
gerritbot added a comment.Feb 16 2021, 6:42 PM

Change 664634 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/FlaggedRevs@master] Temporarily disable FlaggedRevs ProtectionForm::buildForm hook

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

Krinkle subscribed.Feb 16 2021, 6:49 PM

Under the Stable interface policy please revert the hard-deprecation to unblock the train. Perhaps it was thought to be unused or Codesearch forgotten to be checked, but we now know otherwise. The simplest might be to comment-out the wfDeprecated() call and backport that accordingly. Other development can and should happen outside the UBN path.

gerritbot added a comment.Feb 16 2021, 6:53 PM

Change 664636 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Silent deprecate ProtectionForm::buildForm

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

gerritbot added a comment.Feb 16 2021, 7:00 PM

Change 664634 abandoned by Jdlrobson:
[mediawiki/extensions/FlaggedRevs@master] Temporarily disable FlaggedRevs ProtectionForm::buildForm hook

Reason:
See https://gerrit.wikimedia.org/r/c/mediawiki/core/ /664636, my understanding is we eventually went with a change that rendered non-OOUI HTML inside OOUI form (after considering one that just broke the feature). I missed the hard deprecation in code review. It should have been silent.

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

• mmodell mentioned this in T274934: Reason dropdown field not displayed when protecting a page on French Wikipedia to users without interface-admin user right.Feb 16 2021, 7:59 PM
gerritbot added a comment.Feb 16 2021, 8:26 PM

Change 664265 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@wmf/1.36.0-wmf.31] Silent deprecate ProtectionForm::buildForm

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

• Agusbou2015 subscribed.Feb 16 2021, 9:15 PM
gerritbot added a comment.Feb 16 2021, 9:18 PM

Change 664636 merged by jenkins-bot:
[mediawiki/core@master] Silent deprecate ProtectionForm::buildForm

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.32; 2021-02-23).Feb 16 2021, 10:00 PM
gerritbot added a comment.Feb 16 2021, 10:31 PM

Change 664648 had a related patch set uploaded (by 20after4; owner: Jdlrobson):
[mediawiki/core@wmf/1.36.0-wmf.30] Silent deprecate ProtectionForm::buildForm

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

gerritbot added a comment.Feb 16 2021, 11:03 PM

Change 664648 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.30] Silent deprecate ProtectionForm::buildForm

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

Stashbot added a comment.Feb 16 2021, 11:09 PM

Mentioned in SAL (#wikimedia-operations) [2021-02-16T23:09:53Z] <twentyafterfour@deploy1001> Synchronized php-1.36.0-wmf.30/includes/HookContainer/DeprecatedHooks.php: silence deprecation refs T274889 (duration: 01m 14s)

Jdlrobson lowered the priority of this task from Unbreak Now! to Needs Triage.Feb 16 2021, 11:43 PM
Jdlrobson removed a parent task: T271345: 1.36.0-wmf.31 deployment blockers.

Looks like these are tailing off now, so lowering priority and removing as deploy blocker.

Screen Shot 2021-02-16 at 3.42.40 PM.png (890×2 px, 163 KB)

As usual, I have to wonder why something was marked as deprecated when it was still in use in WMF production...

I'm going to interpret this as a question about what we can do better.

In short: human error, communication error, mistakes happen. We work in an extremely complex codebase, with over 200 different sites with different configurations and little to no testing and the migration of these forms to OOUI is not straightforward.

As far as I can see our current approach to catching this kind of error relies completely on poorly documented manual steps that involve using codesearch and domain knowledge.

We should have been able to detect this in our logstash instance, given our current infrastructure and the code was in beta for a good 2 weeks, so we should have been able to detect this way before train roll out. This particular change was a big one, and the work of a volunteer over a long period of time, and we've let them down by not providing adequate tooling here which frustrates me.

How can we improve this in future? Some possible suggestions.

  1. Add a Selenium test in core or the extension for protection form - this would have caught this in code review via Jenkins

2a) Add monitoring to logstash's beta instance - this would have alerted us to the issue prior to the train
The error showed up, and the code was live there for 2 weeks, but nobody noticed.
https://logstash-beta.wmflabs.org/app/kibana#/doc/logstash-*/logstash-deploy-2021.02.16/mediawiki/?id=AXeqFLCoL_WvC-2XMBWP

2b) Make logstash's beta errors UBN in future, and train blocking as they appear.

  1. Complete the code stewardship review - I think the challenge here with FlaggedRevs is that it doesn't enjoy the same maintenance level as other extensions (as a product engineer I can say very few people are testing code against it), so perhaps a reactive error monitoring approach would be the best approach rather than a selenium test. Note T185664 will help with that, so rather than funneling any frustration towards a reviewer or patch submitter on a bug report, it's probably best to be more vocal there and helping C-levels understand what not having proper stewardship means to users and staff.
Reedy added a comment.EditedFeb 16 2021, 11:52 PM
In T274889#6835771, @Jdlrobson wrote:

Looks like these are tailing off now, so lowering priority and removing as deploy blocker.

Screen Shot 2021-02-16 at 3.42.40 PM.png (890×2 px, 163 KB)

As usual, I have to wonder why something was marked as deprecated when it was still in use in WMF production...

I'm going to interpret this as a question about what we can do better.

In short: human error, communication error, mistakes happen. We work in an extremely complex codebase, with over 200 different sites with different configurations and little to no testing and the migration of these forms to OOUI is not straightforward.

As far as I can see our current approach to catching this kind of error relies completely on poorly documented manual steps that involve using codesearch and domain knowledge.

You've been doing MW development for... 9 years at this point?

A simple grep of our hosted extensions (clone the extensions meta repo; if you're doing MW core development, you should really have all the extensions checked out for this sort of purpose) or use of https://codesearch.wmcloud.org for the hook name will have found that there's numerous extensions that use it.

https://codesearch.wmcloud.org/search/?q=%22ProtectionForm%3A%3AbuildForm%22&i=nope&files=&excludeFiles=&repos=

Hooks are not new. Extension registration is not new. The subscriber is not obfuscated, it's not dynamic.

Screenshot 2021-02-16 at 23.51.16.png (1×1 px, 176 KB)

ReleaseTaggerBot edited projects, added MW-1.36-notes (1.36.0-wmf.30; 2021-02-09); removed MW-1.36-notes (1.36.0-wmf.32; 2021-02-23).Feb 17 2021, 12:00 AM
gerritbot added a comment.Feb 18 2021, 11:51 AM

Change 664265 merged by jenkins-bot:
[mediawiki/core@wmf/1.36.0-wmf.31] Silent deprecate ProtectionForm::buildForm

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

ReleaseTaggerBot edited projects, added MW-1.36-notes (1.36.0-wmf.31; 2021-02-16); removed MW-1.36-notes (1.36.0-wmf.30; 2021-02-09).Feb 18 2021, 12:00 PM
Stashbot added a comment.Feb 18 2021, 12:01 PM

Mentioned in SAL (#wikimedia-operations) [2021-02-18T12:01:15Z] <urbanecm@deploy1001> Synchronized php-1.36.0-wmf.31/includes/HookContainer/DeprecatedHooks.php: rMW28aa8718549b: Silent deprecate ProtectionForm::buildForm (T274889) (duration: 01m 14s)

gerritbot added a comment.Mar 2 2021, 6:58 AM

Change 663327 merged by jenkins-bot:
[mediawiki/extensions/FlaggedRevs@master] Use OOUI for action=protect form

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

Umherirrender closed this task as Resolved.Mar 8 2021, 10:40 PM
Maintenance_bot removed a project: Patch-For-Review.Mar 12 2021, 9:54 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and 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 · Credits