Page MenuHomePhabricator

Use of ProtectionForm::buildForm hook (used in FlaggedRevsUIHooks::onProtectionForm) was deprecated in MediaWiki 1.36
Open, Needs TriagePublicPRODUCTION ERROR

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

Event Timeline

mmodell created this task.Tue, Feb 16, 3:00 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptTue, Feb 16, 3:00 PM
mmodell triaged this task as Unbreak Now! priority.Tue, Feb 16, 3:06 PM

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

Reedy added a subscriber: Reedy.Tue, Feb 16, 4:31 PM

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

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

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

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.

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

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

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

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

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

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

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

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

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.Tue, Feb 16, 11:43 PM

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

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.EditedTue, Feb 16, 11:52 PM

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

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.

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

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

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