Setup: Adopt RequestContext::getActionName for most early callers

Most accesses to this value (other than technically incorrect use
via WebRequest::getVal), such as in various extensions,
call Action::getActionName($ctx) rather than MediaWiki::getAction
or the new RequestContext::getActionName.

Related changes recently:
* Introduced in 3fdfef96e4 (I1e259b54dca4).
* Primary caller optimised in d7beb0e4ec (I72ffc9f36613bf9).
* Warning for misuse added in Ib9fc34ca64b7c0e89.

This fixes a bug in DerivativeContext, which got exposed by the
ActionTest test cases, which happen to use DerivativeContext and
inheriting from the global RequestContext. That's not ideal for
the test to do, but did luckily help find this bug.

The issue is that when changing the title or wikipage, the action
cache be cleared and re-computed the next time it is accessed.
The RequestContext class (which is also a mutable context) does
the same and that is already covered by tests.

Bug: T314008
Depends-On: I8434442a61449c16981b19935f2dbdf18e4b4e50
Change-Id: I7d40e7ca4878d43fd7d7614d9c8cf8d29a8c7c4b
This commit is contained in:
Timo Tijhof 2021-11-06 04:52:40 +00:00 committed by Tim Starling
parent f997d67c93
commit 4ac588c634
3 changed files with 45 additions and 43 deletions

View file

@ -107,9 +107,8 @@ abstract class Action implements MessageLocalizer {
* @return string Action name
*/
final public static function getActionName( IContextSource $context ) {
return MediaWikiServices::getInstance()
->getActionFactory()
->getActionName( $context );
// Optimisation: Reuse/prime the cached value of RequestContext
return $context->getActionName();
}
/**

View file

@ -46,9 +46,9 @@ class DerivativeContext extends ContextSource implements MutableContext {
private $wikipage;
/**
* @var string
* @var string|null|false
*/
private $action;
private $action = false;
/**
* @var OutputPage
@ -144,6 +144,7 @@ class DerivativeContext extends ContextSource implements MutableContext {
*/
public function setTitle( Title $title ) {
$this->title = $title;
$this->action = null;
}
/**
@ -183,6 +184,7 @@ class DerivativeContext extends ContextSource implements MutableContext {
$this->setTitle( $pageTitle );
}
$this->wikipage = $wikiPage;
$this->action = null;
}
/**
@ -217,7 +219,17 @@ class DerivativeContext extends ContextSource implements MutableContext {
* @return string Action
*/
public function getActionName(): string {
return $this->action ?: $this->getContext()->getActionName();
if ( $this->action === false ) {
return $this->getContext()->getActionName();
}
if ( $this->action === null ) {
$this->action = MediaWikiServices::getInstance()
->getActionFactory()
->getActionName( $this );
}
return $this->action;
}
/**

View file

@ -1,25 +1,12 @@
<?php
namespace MediaWiki\Tests\Unit\Context;
use DerivativeContext;
use FauxRequest;
use HashConfig;
use IContextSource;
use Language;
use MediaWiki\Actions\ActionFactory;
use MediaWiki\Permissions\Authority;
use MediaWikiUnitTestCase;
use OutputPage;
use RequestContext;
use Title;
use User;
use WikiPage;
/**
* @coversDefaultClass DerivativeContext
* @package MediaWiki\Tests\Unit\Context
* @covers DerivativeContext
*/
class DerivativeContextTest extends MediaWikiUnitTestCase {
class DerivativeContextTest extends MediaWikiIntegrationTestCase {
public function provideGetterSetter() {
$initialContext = new RequestContext();
@ -102,27 +89,6 @@ class DerivativeContextTest extends MediaWikiUnitTestCase {
}
/**
* @covers ::__construct
* @covers ::getContext
* @covers ::setContext
* @covers ::getConfig
* @covers ::setConfig
* @covers ::getOutput
* @covers ::setOutput
* @covers ::getUser
* @covers ::setUser
* @covers ::getAuthority
* @covers ::setAuthority
* @covers ::setLanguage
* @covers ::getLanguage
* @covers ::getRequest
* @covers ::setRequest
* @covers ::getTitle
* @covers ::setTitle
* @covers ::getWikiPage
* @covers ::setWikiPage
* @covers ::getActionName
* @covers ::setActionName
* @dataProvider provideGetterSetter
*/
public function testGetterSetter(
@ -141,4 +107,29 @@ class DerivativeContextTest extends MediaWikiUnitTestCase {
$derivativeContext->$setter( $newValue );
$this->assertSame( $newValue, $derivativeContext->$getter(), 'Get new value' );
}
public function testOverideActionName() {
$parent = new RequestContext();
$parent->setActionName( 'view' );
$factory = $this->createMock( ActionFactory::class );
$factory
->method( 'getActionName' )
->willReturnOnConsecutiveCalls( 'foo', 'bar', 'baz' );
$this->setService( 'ActionFactory', $factory );
$derivative = new DerivativeContext( $parent );
$this->assertSame( 'view', $derivative->getActionName(), 'default to parent cache' );
$derivative->setTitle( $this->createMock( Title::class ) );
$this->assertSame( 'foo', $derivative->getActionName(), 'recompute after change' );
$this->assertSame( 'foo', $derivative->getActionName(), 'local cache' );
$derivative->setWikiPage( $this->createMock( WikiPage::class ) );
$this->assertSame( 'bar', $derivative->getActionName(), 'recompute after change' );
$this->assertSame( 'bar', $derivative->getActionName(), 'local cache' );
$derivative->setActionName( 'custom' );
$this->assertSame( 'custom', $derivative->getActionName(), 'override' );
}
}