diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php index 94f8b524762..59fd8a70c21 100644 --- a/includes/MediaWiki.php +++ b/includes/MediaWiki.php @@ -42,9 +42,6 @@ class MediaWiki { private $context; /** @var Config */ private $config; - - /** @var string|null Cache what action this request is */ - private $action; /** @var int Class DEFER_* constant; how non-blocking post-response tasks should run */ private $postSendStrategy; @@ -173,11 +170,7 @@ class MediaWiki { * @return string Action */ public function getAction(): string { - if ( $this->action === null ) { - $this->action = $this->context->getActionName(); - } - - return $this->action; + return $this->context->getActionName(); } /** @@ -296,10 +289,11 @@ class MediaWiki { $this->context->setRequest( $derivateRequest ); // Do not varnish cache these. May vary even for anons $output->lowerCdnMaxage( 0 ); + // NOTE: This also clears any action cache. + // Action should not have been computed yet, but if it was, + // we reset it because special pages only support "view". $this->context->setTitle( $target ); $wgTitle = $target; - // Reset action type cache. (Special pages have only view) - $this->action = null; $title = $target; $output->addJsConfigVars( [ 'wgInternalRedirectTargetUrl' => $target->getLinkURL( $query ), @@ -417,8 +411,8 @@ class MediaWiki { $title = $this->context->getTitle(); $services = MediaWikiServices::getInstance(); if ( $this->context->canUseWikiPage() ) { - // Reuse the WikiPage instance from context, as it may already have been initialized - // by an earlier this->getAction() call. + // Optimization: Reuse the WikiPage instance from context, to avoid + // repeat fetching or computation of data already loaded. $page = $this->context->getWikiPage(); } else { // This case should not happen, but just in case. @@ -477,12 +471,14 @@ class MediaWiki { $rarticle->setRedirectedFrom( $title ); $article = $rarticle; + // NOTE: This also clears any action cache $this->context->setTitle( $target ); $this->context->setWikiPage( $article->getPage() ); } } } else { // Article may have been changed by hook + // NOTE: This also clears any action cache $this->context->setTitle( $article->getTitle() ); $this->context->setWikiPage( $article->getPage() ); } diff --git a/includes/context/RequestContext.php b/includes/context/RequestContext.php index b294fbed7c0..7bcbe784790 100644 --- a/includes/context/RequestContext.php +++ b/includes/context/RequestContext.php @@ -294,10 +294,9 @@ class RequestContext implements IContextSource, MutableContext { // expensive operations to compute this. The computation involves creation // of Article, WikiPage, and ContentHandler objects (and the various // database queries these classes require to be instantiated), as well - // as potentially slow extension hooks at various level in these - // classes. + // as potentially slow extension hooks in these classes. // - // This is value frequently needed in OutputPage and in various + // This value is frequently needed in OutputPage and in various // Skin-related methods and classes. if ( $this->action === null ) { $this->action = MediaWikiServices::getInstance() @@ -310,18 +309,15 @@ class RequestContext implements IContextSource, MutableContext { private function clearActionName(): void { if ( $this->action !== null ) { - // Log if cleared after something already computed it as that is - // likely to cause bugs (given the first caller may be using it for - // something), and also because it's an expensive thing to needlessly - // compute multiple times even when it produces the same value. + // If we're clearing after something else has actually already computed the action, + // emit a warning. // - // TODO: Once confident we don't rely on this, - // change to E_USER_WARNING with trigger_error and silence error - // in relevant tests. - $logger = LoggerFactory::getInstance( 'Setup' ); - $logger->warning( 'Changing action after getActionName was already called', - [ 'exception' => new Exception ] - ); + // Doing so is unstable, given the first caller got something that turns out to be + // incomplete or incorrect. Even if we end up re-creating an instance of the same + // class, we may now be acting on a different title/skin/user etc. + // + // Re-computing the action is expensive and can be a performance problem (T302623). + trigger_error( 'Unexpected clearActionName after getActionName already called' ); $this->action = null; } } diff --git a/tests/phpunit/includes/context/RequestContextTest.php b/tests/phpunit/includes/context/RequestContextTest.php index 97aededa5e6..ab9b99b9930 100644 --- a/tests/phpunit/includes/context/RequestContextTest.php +++ b/tests/phpunit/includes/context/RequestContextTest.php @@ -190,7 +190,8 @@ class RequestContextTest extends MediaWikiIntegrationTestCase { $this->assertSame( 'aaa', $context->getActionName(), 'first from factory' ); $this->assertSame( 'aaa', $context->getActionName(), 'cached first' ); - $context->setTitle( $this->createMock( Title::class ) ); + // Ignore warning from clearActionName + @$context->setTitle( $this->createMock( Title::class ) ); $this->assertSame( 'bbb', $context->getActionName(), 'second from factory' ); $this->assertSame( 'bbb', $context->getActionName(), 'cached second' ); }