Promote dirty RequestContext::clearActionName to runtime warning
Open, LowPublic
Actions

Description

Filing a task retroactively since the first attempt failed and we've identified some prep work that seems worth tracking here for easy reference.

Prior work in 2022 before this task was filed:

rMW3fdfef96e4ee: context: Add a cached RequestContext::getActionName method https://gerrit.wikimedia.org/r/737160
rMWd7beb0e4ec7c: MediaWiki: Don't eagerly initialize action name https://gerrit.wikimedia.org/r/808230
rMWd4ce0f3255ad: Setup: Promote clearActionName log message to runtime warning https://gerrit.wikimedia.org/r/810976 - reverted due to the warning (correctly) identifying a violation in VisualEditor, as reported in T312838: clearActionName warning is blocking CI

Prior ideas in 2020:

In T249265: Getting Action name is too heavy on dependencies at T249265#6025206, @Pchelolo wrote:

[…]

Plan

I have made an overview of all the Action::getName overrides, and all of them are returning the same key the action is registered under.

One exception is SpecialPageAction, however the modifications it does were added as a workaround to support IE6 in T22966, thus it can be removed now. Flow overrides action to return a private field set in constructor, but that's just to support make it simpler to override different flow actions.

The action itself needs to know it's name sometimes, most heavily used in FormAction::getForm.

Proposal:

Remove workaround for T22966 from Action::getActionName and SpecialPageAction::getName

Under the Stable interface policy make Action non-newable, thus deprecate and remove access to the constructor from anything except subclasses

Modify Action constructor signature to __construct( string $name, Article $article ) - note that we drop the context, since Article should already hold the context. This is not a part of this issue, but since we would be deprecating the constructor signature, we might as well do this. Have b/c support for old signature, deprecate it.

Store the $name into a private field in the class, make Action::getName non-abstract, return the name passed in the constructor. Deprecate Action::getName for overriding in subclasses

Remove Action subclasses getName overrides

Make Action::getName final.

Details

Project	Branch	Lines +/-	Subject
mediawiki/core	master	+18 -11	HistoryAction: Remove use of action=historysubmit hack
mediawiki/core	master	+20 -27	Setup: Promote clearActionName log message to runtime warning (2)
mediawiki/extensions/QuickSurveys	master	+3 -5	Change Action::getActionName to context ::getActionName to reduce cost
mediawiki/core	master	+45 -43	Setup: Adopt RequestContext::getActionName for most early callers
mediawiki/extensions/VisualEditor	master	+4 -9	ApiVisualEditorEdit: Switch to non-global FlaggablePageView::newFromTitle
mediawiki/extensions/FlaggedRevs	master	+13 -13	FlaggedRevsUIHooks: Migrate remaining hooks to newFromTitle
mediawiki/extensions/FlaggedRevs	master	+25 -28	FlaggedRevsUIHooks: Remove all-but-one call to globalArticleInstance()
mediawiki/extensions/VisualEditor	master	+11 -6	ApiVisualEditorEdit: Re-order FlaggedRevs logic to avoid breakage
mediawiki/extensions/FlaggedRevs	master	+23 -7	FlaggableWikiPage: Replace Title-expando and clone hack with MapCacheLRU

Customize query in gerrit

Related Objects
Search...

Status	Subtype	Assigned	Task
Open		None	T302623 FY2022-2023: Improve Backend Pageview Timing
Open		Krinkle	T314008 Promote dirty RequestContext::clearActionName to runtime warning
Open	PRODUCTION ERROR	Tgr	T323254 MediaWiki->initializeArticle on FlaggedRevs wikis triggers deprecated "Unexpected clearActionName after getActionName"
Resolved	PRODUCTION ERROR	Krinkle	T323184 SpecialPage transclusion triggers deprecated "Unexpected clearActionName after getActionName"
Open		None	T323338 Resolve revisiondelete/editchangetags debt around fake action/specialpage handling

Event Timeline

Krinkle created this task.Jul 28 2022, 2:54 AM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 28 2022, 2:54 AM

Krinkle mentioned this in T312838: clearActionName warning is blocking CI.Jul 28 2022, 2:55 AM

Change 817920 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/FlaggedRevs@master] FlaggableWikiPage: Replace Title-expando and clone hack with MapCacheLRU

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

gerritbot added a project: Patch-For-Review.Jul 28 2022, 3:15 AM

Change 817923 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/FlaggedRevs@master] FlaggedRevsUIHooks: Remove all-but-one call to globalArticleInstance()

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

In T312838#8071950, @matmarex wrote:

Stack traces for future reference:

08:55:32 mw-error.log:2022-07-12 06:54:41 c91d7f5c8e83 wikidb: [f7d64a7d38af2d9ed7e4f6d0] /api.php   PHP Notice: Unexpected clearActionName after getActionName already called
08:55:32 mw-error.log:#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, integer, array)
08:55:32 mw-error.log:#1 /workspace/src/includes/context/RequestContext.php(320): trigger_error(string)
08:55:32 mw-error.log:#2 /workspace/src/includes/context/RequestContext.php(185): RequestContext->clearActionName()
08:55:32 mw-error.log:#3 /workspace/src/extensions/VisualEditor/includes/ApiVisualEditorEdit.php(506): RequestContext->setTitle(Title)
08:55:32 mw-error.log:#4 /workspace/src/includes/api/ApiMain.php(1901): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->execute()
08:55:32 mw-error.log:#5 /workspace/src/includes/api/ApiMain.php(875): ApiMain->executeAction()
08:55:32 mw-error.log:#6 /workspace/src/includes/api/ApiMain.php(846): ApiMain->executeActionWithErrorHandling()
08:55:32 mw-error.log:#7 /workspace/src/api.php(90): ApiMain->execute()
08:55:32 mw-error.log:#8 /workspace/src/api.php(45): wfApiMain()
08:55:32 mw-error.log:#9 {main}
08:55:32 mw-error.log:2022-07-12 06:54:41 c91d7f5c8e83 wikidb: [f7d64a7d38af2d9ed7e4f6d0] /api.php   PHP Notice: Unexpected clearActionName after getActionName already called
08:55:32 mw-error.log:#0 [internal function]: MWExceptionHandler::handleError(integer, string, string, integer, array)
08:55:32 mw-error.log:#1 /workspace/src/includes/context/RequestContext.php(320): trigger_error(string)
08:55:32 mw-error.log:#2 /workspace/src/includes/context/RequestContext.php(185): RequestContext->clearActionName()
08:55:32 mw-error.log:#3 /workspace/src/extensions/VisualEditor/includes/ApiVisualEditorEdit.php(516): RequestContext->setTitle(Title)
08:55:32 mw-error.log:#4 /workspace/src/includes/api/ApiMain.php(1901): MediaWiki\Extension\VisualEditor\ApiVisualEditorEdit->execute()
08:55:32 mw-error.log:#5 /workspace/src/includes/api/ApiMain.php(875): ApiMain->executeAction()
08:55:32 mw-error.log:#6 /workspace/src/includes/api/ApiMain.php(846): ApiMain->executeActionWithErrorHandling()
08:55:32 mw-error.log:#7 /workspace/src/api.php(90): ApiMain->execute()
08:55:32 mw-error.log:#8 /workspace/src/api.php(45): wfApiMain()
08:55:32 mw-error.log:#9 {main}

VE code calls RequestContext::getMain()->setTitle( … ) in code that integrates with FlaggedRevs, because FlaggedRevs uses the global context for various things. We can't replace it without rewriting a chunk of FlaggedRevs […]

In T312838#8110743, @Krinkle wrote:

[…] I have an idea for how to address @matmarex's point in a way that satisfied both the needs of FlaggedRevs in ApiVisualEditorEdit hack as well as the need to not cause a dirty RequestContext action value.

Change 737161 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] Setup: Adopt RequestContext::getActionName for most early callers

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

Krinkle triaged this task as Low priority.Jul 28 2022, 3:36 AM

Change 817920 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] FlaggableWikiPage: Replace Title-expando and clone hack with MapCacheLRU

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

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.23; 2022-08-01).Aug 1 2022, 9:00 AM

Krinkle moved this task from Inbox, needs triage to Backlog: Maintenance, non-prioritized on the Performance-Team board.Aug 1 2022, 6:57 PM

Change 819195 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/VisualEditor@master] ApiVisualEditorEdit: Re-order FlaggedRevs logic to avoid breakage

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

Change 819195 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] ApiVisualEditorEdit: Re-order FlaggedRevs logic to avoid breakage

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

Change 817923 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] FlaggedRevsUIHooks: Remove all-but-one call to globalArticleInstance()

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

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.23; 2022-08-01).Aug 8 2022, 10:00 AM

Change 821200 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/FlaggedRevs@master] FlaggedRevsUIHooks: Migrate remaining hooks to newFromTitle

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

Change 821204 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/VisualEditor@master] ApiVisualEditorEdit: Switch to non-global FlaggablePageView::newFromTitle

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

Change 821200 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] FlaggedRevsUIHooks: Migrate remaining hooks to newFromTitle

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

Change 821204 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] ApiVisualEditorEdit: Switch to non-global FlaggablePageView::newFromTitle

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.26; 2022-08-22); removed MW-1.39-notes (1.39.0-wmf.25; 2022-08-15).Aug 19 2022, 4:00 AM

Change 737161 merged by jenkins-bot:

[mediawiki/core@master] Setup: Adopt RequestContext::getActionName for most early callers

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

ReleaseTaggerBot added a project: MW-1.40-notes (1.40.0-wmf.2; 2022-09-19).Sep 20 2022, 1:00 AM

Change 835133 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] Setup: Promote clearActionName log message to runtime warning (2)

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

Change 782562 had a related patch set uploaded (by Krinkle; author: Umherirrender):

[mediawiki/extensions/QuickSurveys@master] Change Action::getActionName to context ::getActionName to reduce cost

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

thiemowmde updated the task description. (Show Details)Sep 26 2022, 7:12 PM

Change 782562 merged by jenkins-bot:

[mediawiki/extensions/QuickSurveys@master] Change Action::getActionName to context ::getActionName to reduce cost

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

ReleaseTaggerBot edited projects, added MW-1.40-notes (1.40.0-wmf.4; 2022-10-03); removed MW-1.40-notes (1.40.0-wmf.2; 2022-09-19).Sep 28 2022, 9:00 PM

Change 835133 merged by jenkins-bot:

[mediawiki/core@master] Setup: Promote clearActionName log message to runtime warning (2)

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

ReleaseTaggerBot edited projects, added MW-1.40-notes (1.40.0-wmf.10; 2022-11-14); removed MW-1.40-notes (1.40.0-wmf.4; 2022-10-03).Nov 9 2022, 12:01 AM

We deployed the latest merged patch on Translatewiki.net and noticed the following warning:

[2022-11-09T13:47:33.930521+00:00] error.WARNING: [6f7237b9fde5ec3d3e3a99fa] [no req]   PHP Notice: Unexpected clearActionName after getActionName already called {"exception":"[object] (ErrorException(code: 0): PHP Notice: Unexpected clearActionName after getActionName already called at /srv/mediawiki/tags/2022-11-09_13:39:45/includes/context/RequestContext.php:311)
[stacktrace]
#0 [internal function]: MWExceptionHandler::handleError()
#1 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/context/RequestContext.php(311): trigger_error()
#2 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/context/RequestContext.php(176): RequestContext->clearActionName()
#3 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/specialpage/SpecialPageFactory.php(1506): RequestContext->setTitle()
#4 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/Parser.php(3212): MediaWiki\\SpecialPage\\SpecialPageFactory->capturePath()
#5 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/PPFrame_Hash.php(275): Parser->braceSubstitution()
#6 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/PPTemplateFrame_Hash.php(97): PPFrame_Hash->expand()
#7 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/Parser.php(3303): PPTemplateFrame_Hash->cachedExpand()
#8 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/PPFrame_Hash.php(275): Parser->braceSubstitution()
#9 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/Parser.php(3306): PPFrame_Hash->expand()
#10 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/PPFrame_Hash.php(275): Parser->braceSubstitution()
#11 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/Parser.php(3306): PPFrame_Hash->expand()
#12 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/PPFrame_Hash.php(275): Parser->braceSubstitution()
#13 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/Parser.php(2944): PPFrame_Hash->expand()
#14 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/Parser.php(1600): Parser->replaceVariables()
#15 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/parser/Parser.php(714): Parser->internalParse()
#16 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/content/WikitextContentHandler.php(303): Parser->parse()
#17 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/content/ContentHandler.php(1745): WikitextContentHandler->fillParserOutput()
#18 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/content/Renderer/ContentRenderer.php(47): ContentHandler->getParserOutput()
#19 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/Revision/RenderedRevision.php(266): MediaWiki\\Content\\Renderer\\ContentRenderer->getParserOutput()
#20 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/Revision/RenderedRevision.php(237): MediaWiki\\Revision\\RenderedRevision->getSlotParserOutputUncached()
#21 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/Revision/RevisionRenderer.php(221): MediaWiki\\Revision\\RenderedRevision->getSlotParserOutput()
#22 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/Revision/RevisionRenderer.php(158): MediaWiki\\Revision\\RevisionRenderer->combineSlotOutput()
#23 [internal function]: MediaWiki\\Revision\\RevisionRenderer->MediaWiki\\Revision\\{closure}()
#24 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/Revision/RenderedRevision.php(199): call_user_func()
#25 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/poolcounter/PoolWorkArticleView.php(87): MediaWiki\\Revision\\RenderedRevision->getRevisionParserOutput()
#26 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/poolcounter/PoolWorkArticleViewOld.php(65): PoolWorkArticleView->renderRevision()
#27 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/poolcounter/PoolCounterWork.php(163): PoolWorkArticleViewOld->doWork()
#28 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/page/ParserOutputAccess.php(299): PoolCounterWork->execute()
#29 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/page/Article.php(708): MediaWiki\\Page\\ParserOutputAccess->getParserOutput()
#30 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/page/Article.php(522): Article->generateContentOutput()
#31 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/actions/ViewAction.php(78): Article->view()
#32 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/MediaWiki.php(539): ViewAction->show()
#33 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/MediaWiki.php(317): MediaWiki->performAction()
#34 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/MediaWiki.php(901): MediaWiki->performRequest()
#35 /srv/mediawiki/tags/2022-11-09_13:39:45/includes/MediaWiki.php(559): MediaWiki->main()
#36 /srv/mediawiki/tags/2022-11-09_13:39:45/index.php(50): MediaWiki->run()
#37 /srv/mediawiki/tags/2022-11-09_13:39:45/index.php(46): wfIndexMain()
#38 {main}
","exception_url":"[no req]","reqId":"6f7237b9fde5ec3d3e3a99fa","caught_by":"mwe_handler"} []

I see the same thing locally on Vagrant. Special page transclusion results in the parser calling SpecialPageFactory::capturePath() which does some horrible title manipulation hack (cf T290706: Parser code for special page transclusion replaces main context) and that triggers the notice.

brennen mentioned this in T323153: HistoryAction/FeedUtils triggers deprecated "Unexpected clearActionName after getActionName".Nov 15 2022, 7:24 PM