diff --git a/includes/MediaWiki.php b/includes/MediaWiki.php index 59fd8a70c21..94f8b524762 100644 --- a/includes/MediaWiki.php +++ b/includes/MediaWiki.php @@ -42,6 +42,9 @@ 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; @@ -170,7 +173,11 @@ class MediaWiki { * @return string Action */ public function getAction(): string { - return $this->context->getActionName(); + if ( $this->action === null ) { + $this->action = $this->context->getActionName(); + } + + return $this->action; } /** @@ -289,11 +296,10 @@ 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 ), @@ -411,8 +417,8 @@ class MediaWiki { $title = $this->context->getTitle(); $services = MediaWikiServices::getInstance(); if ( $this->context->canUseWikiPage() ) { - // Optimization: Reuse the WikiPage instance from context, to avoid - // repeat fetching or computation of data already loaded. + // Reuse the WikiPage instance from context, as it may already have been initialized + // by an earlier this->getAction() call. $page = $this->context->getWikiPage(); } else { // This case should not happen, but just in case. @@ -471,14 +477,12 @@ 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 7bcbe784790..b294fbed7c0 100644 --- a/includes/context/RequestContext.php +++ b/includes/context/RequestContext.php @@ -294,9 +294,10 @@ 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 in these classes. + // as potentially slow extension hooks at various level in these + // classes. // - // This value is frequently needed in OutputPage and in various + // This is value frequently needed in OutputPage and in various // Skin-related methods and classes. if ( $this->action === null ) { $this->action = MediaWikiServices::getInstance() @@ -309,15 +310,18 @@ class RequestContext implements IContextSource, MutableContext { private function clearActionName(): void { if ( $this->action !== null ) { - // If we're clearing after something else has actually already computed the action, - // emit a warning. + // 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. // - // 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' ); + // 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 ] + ); $this->action = null; } } diff --git a/tests/phpunit/includes/context/RequestContextTest.php b/tests/phpunit/includes/context/RequestContextTest.php index ab9b99b9930..97aededa5e6 100644 --- a/tests/phpunit/includes/context/RequestContextTest.php +++ b/tests/phpunit/includes/context/RequestContextTest.php @@ -190,8 +190,7 @@ class RequestContextTest extends MediaWikiIntegrationTestCase { $this->assertSame( 'aaa', $context->getActionName(), 'first from factory' ); $this->assertSame( 'aaa', $context->getActionName(), 'cached first' ); - // Ignore warning from clearActionName - @$context->setTitle( $this->createMock( Title::class ) ); + $context->setTitle( $this->createMock( Title::class ) ); $this->assertSame( 'bbb', $context->getActionName(), 'second from factory' ); $this->assertSame( 'bbb', $context->getActionName(), 'cached second' ); }