Page MenuHomePhabricator

Promote dirty RequestContext::clearActionName to runtime warning
Closed, ResolvedPublic

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:

Prior ideas in 2020:

[…]

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.

Related Objects

Event Timeline

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

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

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

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

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

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

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

Change 737161 merged by jenkins-bot:

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

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

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

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

Change 835133 merged by jenkins-bot:

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

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

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.

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

Yeah, noticing quite a few of the above now with 1.40.0-wmf.10 (T320515) on group0. I'd mistakenly thought these were all T323153.

Filed separately as T323184: SpecialPage transclusion triggers deprecated "Unexpected clearActionName after getActionName".

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

[mediawiki/core@master] HistoryAction: Remove use of action=historysubmit hack

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

Change 858436 merged by jenkins-bot:

[mediawiki/core@master] HistoryAction: Remove use of action=historysubmit hack

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

Passing the torch to @matmarex. I suggest checking Logstash for any other issues.

If there's nothing new in Logstash, then the only remaining issue is likely the one we suppressed in MediaWiki.php using the @ silence operator (for T323254). For the runtime warning to be of most use, we should remove the silencing though. There's an idea from @Tgr at T323254#8403496 for effectively concentrating the hack within FlaggedRevs, which would be an improvement. But perhaps you see a better to fit FlaggedRevs's hook in the mental model of "compute action only once"?

I spent a few hours today reading MediaWiki.php and FlaggedRevs code. I have two observations:

  • FlaggedRevs does not need to check the action at all. MediaWiki only performs redirects, and only calls the onInitializeArticleMaybeRedirect() hook, when the raw 'action' parameter is 'view' or 'render'. So we can just remove that code, instead of carefully moving it around to avoid the cache clearing warnings.
  • We really shouldn't allow a redirect to result in changing the action. I didn't find any code path that would do it currently, but it seems possible, and I am pretty sure it would break some assumptions somewhere, and be quite confusing for users even if it worked perfectly. If we disallow it, then we don't need to worry about having to clear the action cache.

I'll propose some patches to clarify what I mean (the second point is certainly up for debate).

Change 947427 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/core@master] Revert "MediaWiki: Temp silence FR-induced clearActionName warnings"

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

Change 947427 merged by jenkins-bot:

[mediawiki/core@master] Revert "MediaWiki: Temp silence FR-induced clearActionName warnings"

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