Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Setup: Promote clearActionName log message to runtime warning

Browse source 
Introduced in 3fdfef96e4 (I1e259b54dca4), and as of d7beb0e4ec
(I72ffc9f36613bf9) the debug message is no longer triggered in a
typical local install, nor in production.

Also:
* Improve detection and attribution by removing the last bit of cache
  duplication in MediaWiki.php and defer to RequestContext.php.

Change-Id: Ib9fc34ca64b7c0e89a774bee9a36fa3882eb4ec3
This commit is contained in:
Timo Tijhof 2022-07-04 14:19:59 -07:00 committed by Tim Starling
parent e6e35021d4
commit d4ce0f3255
3 changed files with 20 additions and 27 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
includes/MediaWiki.php
View file

@ -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() );
}

24
includes/context/RequestContext.php
View file

@ -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;
}
}

3
tests/phpunit/includes/context/RequestContextTest.php
View file

@ -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' );
}