Page MenuHomePhabricator

PHP Deprecated: Use of EditPage::$textbox2 was deprecated in MediaWiki 1.38. [Called from TwoColConflict\Hooks\TwoColConflictHooks::onEditPageBeforeConflictDiff]
Closed, ResolvedPublicPRODUCTION ERROR

Description

Error
normalized_message
[{reqId}] {exception_url}   PHP Deprecated: Use of EditPage::$textbox2 was deprecated in MediaWiki 1.38. [Called from TwoColConflict\Hooks\TwoColConflictHooks::onEditPageBeforeConflictDiff]
exception.trace
from /srv/mediawiki/php-1.39.0-wmf.5/extensions/TwoColConflict/includes/Hooks/TwoColConflictHooks.php(156)
#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, string, array)
#1 /srv/mediawiki/php-1.39.0-wmf.5/includes/debug/MWDebug.php(377): trigger_error(string, integer)
#2 /srv/mediawiki/php-1.39.0-wmf.5/includes/debug/MWDebug.php(353): MWDebug::sendRawDeprecated(string, boolean, string)
#3 /srv/mediawiki/php-1.39.0-wmf.5/includes/debug/MWDebug.php(232): MWDebug::deprecatedMsg(string, string, string, integer)
#4 /srv/mediawiki/php-1.39.0-wmf.5/includes/GlobalFunctions.php(1013): MWDebug::deprecated(string, string, string, integer)
#5 /srv/mediawiki/php-1.39.0-wmf.5/includes/debug/DeprecationHelper.php(191): wfDeprecated(string, string, NULL, integer)
#6 /srv/mediawiki/php-1.39.0-wmf.5/extensions/TwoColConflict/includes/Hooks/TwoColConflictHooks.php(189): EditPage->__get(string)
#7 /srv/mediawiki/php-1.39.0-wmf.5/extensions/TwoColConflict/includes/Hooks/TwoColConflictHooks.php(156): TwoColConflict\Hooks\TwoColConflictHooks->doEditPageBeforeConflictDiff(EditPage, OutputPage)
#8 /srv/mediawiki/php-1.39.0-wmf.5/includes/HookContainer/HookContainer.php(338): TwoColConflict\Hooks\TwoColConflictHooks::onEditPageBeforeConflictDiff(EditPage, OutputPage)
#9 /srv/mediawiki/php-1.39.0-wmf.5/includes/HookContainer/HookContainer.php(137): MediaWiki\HookContainer\HookContainer->callLegacyHook(string, array, array, array)
#10 /srv/mediawiki/php-1.39.0-wmf.5/includes/HookContainer/HookRunner.php(1489): MediaWiki\HookContainer\HookContainer->run(string, array)
#11 /srv/mediawiki/php-1.39.0-wmf.5/includes/EditPage.php(3951): MediaWiki\HookContainer\HookRunner->onEditPageBeforeConflictDiff(EditPage, OutputPage)
#12 /srv/mediawiki/php-1.39.0-wmf.5/includes/EditPage.php(3171): EditPage->showConflict()
#13 /srv/mediawiki/php-1.39.0-wmf.5/includes/EditPage.php(727): EditPage->showEditForm()
#14 /srv/mediawiki/php-1.39.0-wmf.5/includes/actions/EditAction.php(71): EditPage->edit()
#15 /srv/mediawiki/php-1.39.0-wmf.5/includes/actions/SubmitAction.php(38): EditAction->show()
#16 /srv/mediawiki/php-1.39.0-wmf.5/includes/MediaWiki.php(545): SubmitAction->show()
#17 /srv/mediawiki/php-1.39.0-wmf.5/includes/MediaWiki.php(321): MediaWiki->performAction(Article, Title)
#18 /srv/mediawiki/php-1.39.0-wmf.5/includes/MediaWiki.php(911): MediaWiki->performRequest()
#19 /srv/mediawiki/php-1.39.0-wmf.5/includes/MediaWiki.php(565): MediaWiki->main()
#20 /srv/mediawiki/php-1.39.0-wmf.5/index.php(53): MediaWiki->run()
#21 /srv/mediawiki/php-1.39.0-wmf.5/index.php(46): wfIndexMain()
#22 /srv/mediawiki/w/index.php(3): require(string)
#23 {main}
Impact
Notes

Found after deploying 1.39.0-wmf.5 T300204. I am not marking it as a blocker since it is a deprecation notice.

Event Timeline

I have added a filter in Logstsash to remove this log from the mediawiki-new-errors dashboard.

thiemowmde added subscribers: awight, thiemowmde.

I had a quick look. There is only one reference to EditPage::$textbox2. It's in some EventLogging code. This is entirely optional and meant to go away some day anyway.

Also the information is stored in two places at this point, in $textbox2 and in TextConflictHelper::$yourtext. It's certainly possible to replace one with the other. But this requires some refactoring, e.g. moving the entire log code into TextConflictHelper.

In case you really want to get rid of this $textbox2 reference as fast as possible the best option might be to remove the small piece of log code that relies on $textbox2. I believe we do not visualize the two numbers that rely on $textbox2, or do we?

Krinkle triaged this task as High priority.EditedApr 6 2022, 11:49 PM
Krinkle moved this task from Untriaged to Mar 2022 on the Wikimedia-production-error board.
Krinkle subscribed.

Blocking next train as per train policy. As we forgot to do this last week, you got one week extra :)

Change 778207 had a related patch set uploaded (by Awight; author: Awight):

[schemas/event/secondary@master] Make some fields optional

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

Change 778209 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/TwoColConflict@master] Remove two logging fields

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

Change 778207 abandoned by Awight:

[schemas/event/secondary@master] Make some fields optional

Reason:

Okay, that's frustrating... I guess I'll have to use a magic value like "-1" which obviously will take the same type of client modification that the "cannot remove required" rule was supposed to avoid.

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

Blocking next train as per […]

Because of a deprecation? Can we please undeprecate it until the team had a chance to properly plan this?

Change 775295 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Rearrange code to reduce refs to EditPage::$textbox2 property

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

Change 775295 merged by jenkins-bot:

[mediawiki/core@master] Rearrange code to reduce refs to EditPage::$textbox2 property

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

Change 778282 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Temporarily undeprecate EditPage::$textbox2

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

Blocking next train as per […]

Because of a deprecation? Can we please undeprecate it […]

Specifically a hard deprecation. Something that per our stability policy should not happen in production. I think both this policy and the train policy for it are reasonable.

Your point is however also very reasonable. With my hat of production error triager, I don't have an opinion on the deprecation. Undeprecating is as fine a solution as anything. Given our policy, it was unknown by the person deprecating it that it was used. If migration is non-trivial, then undeprecating seems like a natural first step indeed.

Change 778282 merged by jenkins-bot:

[mediawiki/core@master] Temporarily undeprecate EditPage::$textbox2

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

While I 100% agree that this property should be private, I'm not happy about the process. Is it a mistake that the original patch https://gerrit.wikimedia.org/r/762063 doesn't mention a Phabricator ticket? I mean, a change so significant to such a significant part of core needs a place to be discussed and communicated, doesn't it? Usages seem to be obvious. Sure, we can look into this, look into options, estimate it, assign resources, and take the responsibility, as we always do when core changes. But I don't find it a nice move when a) a logspam ticket is the first time we hear about this, b) I respond the same day, but the only thing that happens a week later ist that c) the priority is raised by turning this into a deployment blocker. I hope you understand.

Thanks for the revert.

I just had a look at this code again and realized that we are talking about an EditPageBeforeConflictDiff hook handler here. A hook that is called when a conflict happened, right before the normal conflict handling screen would be shown to the user, to give extensions (like Two-Column-Edit-Conflict-Merge) a chance to give the user something different instead. That's how it's documented on mediawiki.org since 2008. To do this it must be possible to access both versions of the conflicting content. Accessing the version that's already in the database is not a problem. But the conflicting one doesn't exist anywhere but in memory at this point, specifically in EditPage::$textbox2.

Problem is: The only parameters of the hook are the EditPage instance that just called the hook (i.e. $this) and the current OutputPage. There is no parameter for the conflicting text. It appears like the hook was build with the assumption that it can access EditPage::$textbox2. From today's perspective that wasn't good design. But it is what it is.

Possible ways forward include a new public getter that returns the contents of EditPage::$textbox2. Hook handlers can call this. Or we append EditPage::$textbox2 as a new parameter to the hook. The hook appears to be rarely used, so it's probably not much of a problem to change, replace, or even remove it (after replacing all known callers with something else that works for them).

At the moment the WMDE team doesn't have much resources for something like this that seems to be more on the nice-to-have side of things, unless we are told this is urgent and blocks other teams. I see this is now linked to T252907, but textbox2 is currently not mentioned there. I suggest to close this logspam ticket here and make a new one to track the usages, changes or possibly even the deprecation of the EditPageBeforeConflictDiff hook. Please link or copy what we analyzed here. I hope it will be helpful.

Krinkle assigned this task to Daimona.
Krinkle added a subscriber: Daimona.

Agreed. I'll leave it to @Daimona to decide how and whether to file a separate task for refactoring textbox2 in particular.

dancy subscribed.

I'm steadying seeing steady reports of this warning in .6

Notice: I have the train blocked on this ticket. I'm hoping this can be resolved before Tuesday.

Change 778641 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Daimona Eaytoy):

[mediawiki/core@wmf/1.39.0-wmf.6] Temporarily undeprecate EditPage::$textbox2

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

Change 778642 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Daimona Eaytoy):

[mediawiki/core@REL1_38] Temporarily undeprecate EditPage::$textbox2

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

@dancy, I created backports for the versions that need one. Unfortunately I'm unable to assist these any further.

Change 778641 merged by jenkins-bot:

[mediawiki/core@wmf/1.39.0-wmf.6] Temporarily undeprecate EditPage::$textbox2

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

Mentioned in SAL (#wikimedia-operations) [2022-04-12T15:06:43Z] <dancy@deploy1002> Synchronized php-1.39.0-wmf.6/includes/EditPage.php: Backport: [[gerrit:778641|Temporarily undeprecate EditPage::$textbox2 (T305028)]] (duration: 00m 52s)

@dancy, I created backports for the versions that need one. Unfortunately I'm unable to assist these any further.

Thanks for the backport. I just deployed it and the warnings are fading away. I'm removing this task as a train blocker.

Change 778642 merged by jenkins-bot:

[mediawiki/core@REL1_38] Temporarily undeprecate EditPage::$textbox2

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

Change 778209 merged by jenkins-bot:

[mediawiki/extensions/TwoColConflict@master] Remove expensive logging fields and related instrumentation

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

Change 821726 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] Revert "Temporarily undeprecate EditPage::$textbox2"

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

Umherirrender subscribed.

[...] I suggest to close this logspam ticket here and make a new one to track the usages, changes or possibly even the deprecation of the EditPageBeforeConflictDiff hook. Please link or copy what we analyzed here. I hope it will be helpful.

Have created T316577, but seems already fixed. Maybe reopen that task to discuss the deprecation of hook EditPageBeforeConflictDiff