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

Description

Error

mwversion: 1.39.0-wmf.5
reqId: 1e5e20bb-eb85-4dfc-aab1-76f899e509b4
Find reqId in Logstash

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.

Details

Request URL: https://it.wikipedia.org/wiki/Montesarchio?action=submit&veswitched=*

Related Changes in Gerrit:

Subject	Repo	Branch	Lines +/-
Revert "Temporarily undeprecate EditPage::$textbox2"	mediawiki/core	master	+2 -3
Temporarily undeprecate EditPage::$textbox2	mediawiki/core	master	+3 -2
Remove expensive logging fields and related instrumentation	mediawiki/extensions/TwoColConflict	master	+4 -215
Temporarily undeprecate EditPage::$textbox2	mediawiki/core	REL1_38	+3 -2
Temporarily undeprecate EditPage::$textbox2	mediawiki/core	wmf/1.39.0-wmf.6	+3 -2
Rearrange code to reduce refs to EditPage::$textbox2 property	mediawiki/core	master	+5 -7
Make some fields optional	schemas/event/secondary	master	+496 -7

Customize query in gerrit

Related Objects
Search...

Status	Subtype	Assigned	Task
Open		None	T20654 EditPage.php needs rewrite: Separate DB and UI logic
Open		None	T11968 Reduce use of global and public variables
Resolved		DannyS712	T252907 Reduce public variables and methods in EditPage
Resolved	PRODUCTION ERROR	Daimona	T305028 PHP Deprecated: Use of EditPage::$textbox2 was deprecated in MediaWiki 1.38. [Called from TwoColConflict\Hooks\TwoColConflictHooks::onEditPageBeforeConflictDiff]

Event Timeline

hashar created this task.Mar 30 2022, 8:18 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 30 2022, 8:18 AM

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

thiemowmde added a project: WMDE-TechWish-Maintenance.Mar 30 2022, 8:31 AM

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 added a parent task: T252907: Reduce public variables and methods in EditPage.Apr 5 2022, 3:02 PM

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

gerritbot added a project: Patch-For-Review.Apr 7 2022, 7:22 AM

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

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.7; 2022-04-11).Apr 7 2022, 12:00 PM

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

In T305028#7837459, @thiemowmde wrote:

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.

thiemowmde moved this task from Needs discussion or investigation to Watching or blocked on the WMDE-TechWish-Maintenance board.Apr 8 2022, 5:19 PM

thiemowmde moved this task from Backlog to Watching on the Two-Column-Edit-Conflict-Merge board.

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

thiemowmde added a project: WMDE-TechWish-Sprint-2022-03-30.Apr 11 2022, 6:56 AM

thiemowmde moved this task from Sprint Backlog to Watching / Epic / Stalled on the WMDE-TechWish-Sprint-2022-03-30 board.

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)

In T305028#7847566, @thiemowmde wrote:

@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.

dancy removed a parent task: T305213: 1.39.0-wmf.7 deployment blockers.Apr 12 2022, 3:17 PM

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.6; 2022-04-04); removed MW-1.39-notes (1.39.0-wmf.7; 2022-04-11).Apr 12 2022, 4:00 PM

Zabe subscribed.May 10 2022, 4:52 PM

Change 778642 merged by jenkins-bot:

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

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

ReleaseTaggerBot added a project: MW-1.38-notes.May 10 2022, 6:00 PM

dancy unsubscribed.May 10 2022, 10:11 PM

Change 778209 merged by jenkins-bot:

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

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

thiemowmde mentioned this in rESCCf3dd4ea105db: Remove expensive logging fields and related instrumentation.Aug 9 2022, 10:57 AM

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.25; 2022-08-15); removed MW-1.39-notes (1.39.0-wmf.6; 2022-04-04).Aug 9 2022, 11:00 AM

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 mentioned this in T316577: Find replacement for EditPage::$textbox2 and deprecate the public access.Aug 29 2022, 6:35 PM

In T305028#7841674, @thiemowmde wrote:

[...] 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

Change #821726 abandoned by Daimona Eaytoy: