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

Revert "Setup: Promote clearActionName log message to runtime warning"

Browse source 
This reverts commit d4ce0f3255.

Reason for revert: Causes unrelated test failures, ideally usages are made safe before the patch is merged.

Bug: T312838
Change-Id: I385dca1d95033961d3844e888521750443e49c95
This commit is contained in:
Awight 2022-07-12 10:17:59 +00:00
parent d4ce0f3255
commit dc3bd3d721
3 changed files with 27 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

20
includes/MediaWiki.php
View file

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

24
includes/context/RequestContext.php
View file

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

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

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