From c9318edc2dd531d3d31e233665b92c2e303bdf01 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Mon, 10 Jun 2019 16:00:16 +0100 Subject: [PATCH] resourceloader: Warn on ResourceLoader::construct without Config The only remaining use of 'new ResourceLoader' is in tests, which have been migrated in this commit to either passing the real config explicitly (for integration tests), or by passing a HashConfig from a new 'getMinimalConfig' method which has only the keys required for the tests to pass (e.g. avoid any ConfigExeption for unknown keys). Also clean up some related code quality issues: * Migrate wfScript() to $conf->get() so that the local Config is used, instead of implicitly using global variables. This isn't deprecated for MediaWiki generally, but done here to prepare ResourceLoader for becoming a standalone library. * Remove mocking of 'CacheEpoch' config, this is no longer used anywhere in ResourceLoader. * Change EmptyResourceLoader to use the minimal config by default and remove code duplication by calling the parent. Update the small number of uses that are integration tests, to explicitly pass in the live config as needed. And for the one case that tests the 'startup' module, it no longer needs to register it manually given this is part of ResourceLoader::__construct() by default. Bug: T32956 Change-Id: I127346fd530fa66f205156e545758b1c29d0fac0 --- includes/resourceloader/ResourceLoader.php | 5 ++-- .../ResourceLoaderStartUpModule.php | 4 +-- tests/phpunit/ResourceLoaderTestCase.php | 26 +++++++++---------- .../DerivativeResourceLoaderContextTest.php | 5 +++- .../resourceloader/MessageBlobStoreTest.php | 14 +++++----- .../ResourceLoaderContextTest.php | 2 ++ .../resourceloader/ResourceLoaderTest.php | 17 ++++++------ 7 files changed, 39 insertions(+), 34 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index 418f532af5..515f287a6c 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -236,15 +236,14 @@ class ResourceLoader implements LoggerAwareInterface { /** * Register core modules and runs registration hooks. - * @param Config|null $config [optional] + * @param Config|null $config * @param LoggerInterface|null $logger [optional] */ public function __construct( Config $config = null, LoggerInterface $logger = null ) { $this->logger = $logger ?: new NullLogger(); if ( !$config ) { - // TODO: Deprecate and remove. - $this->logger->debug( __METHOD__ . ' was called without providing a Config instance' ); + wfDeprecated( __METHOD__ . ' without a Config instance', '1.34' ); $config = MediaWikiServices::getInstance()->getMainConfig(); } $this->config = $config; diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php index c834db10e2..d6cc646095 100644 --- a/includes/resourceloader/ResourceLoaderStartUpModule.php +++ b/includes/resourceloader/ResourceLoaderStartUpModule.php @@ -70,14 +70,14 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { // Build list of variables $skin = $context->getSkin(); $vars = [ - 'wgLoadScript' => wfScript( 'load' ), + 'wgLoadScript' => $conf->get( 'LoadScript' ), 'debug' => $context->getDebug(), 'skin' => $skin, 'stylepath' => $conf->get( 'StylePath' ), 'wgUrlProtocols' => wfUrlProtocols(), 'wgArticlePath' => $conf->get( 'ArticlePath' ), 'wgScriptPath' => $conf->get( 'ScriptPath' ), - 'wgScript' => wfScript(), + 'wgScript' => $conf->get( 'Script' ), 'wgSearchType' => $conf->get( 'SearchType' ), 'wgVariantArticlePath' => $conf->get( 'VariantArticlePath' ), // Force object to avoid "empty" associative array from diff --git a/tests/phpunit/ResourceLoaderTestCase.php b/tests/phpunit/ResourceLoaderTestCase.php index e9a8a1fa3c..bd6df5f39c 100644 --- a/tests/phpunit/ResourceLoaderTestCase.php +++ b/tests/phpunit/ResourceLoaderTestCase.php @@ -2,7 +2,6 @@ use MediaWiki\MediaWikiServices; use Psr\Log\LoggerInterface; -use Psr\Log\NullLogger; abstract class ResourceLoaderTestCase extends MediaWikiTestCase { // Version hash for a blank file module. @@ -34,7 +33,7 @@ abstract class ResourceLoaderTestCase extends MediaWikiTestCase { 'only' => 'scripts', 'safemode' => null, ]; - $resourceLoader = $rl ?: new ResourceLoader(); + $resourceLoader = $rl ?: new ResourceLoader( MediaWikiServices::getInstance()->getMainConfig() ); $request = new FauxRequest( [ 'debug' => $options['debug'], 'lang' => $options['lang'], @@ -57,16 +56,23 @@ abstract class ResourceLoaderTestCase extends MediaWikiTestCase { // For ResourceLoader::inDebugMode since it doesn't have context 'ResourceLoaderDebug' => true, - // Avoid influence from wgInvalidateCacheOnLocalSettingsChange - 'CacheEpoch' => '20140101000000', - - // For wfScript() + // For ResourceLoaderStartUpModule and ResourceLoader::__construct() 'ScriptPath' => '/w', 'Script' => '/w/index.php', 'LoadScript' => '/w/load.php', + + // For ResourceLoader::register() - TODO: Inject somehow T32956 + 'ResourceModuleSkinStyles' => [], + + // For ResourceLoader::respond() - TODO: Inject somehow T32956 + 'UseFileCache' => false, ]; } + public static function getMinimalConfig() { + return new HashConfig( self::getSettings() ); + } + protected function setUp() { parent::setUp(); @@ -171,14 +177,8 @@ class ResourceLoaderFileModuleTestModule extends ResourceLoaderFileModule { } class EmptyResourceLoader extends ResourceLoader { - // TODO: This won't be needed once ResourceLoader is empty by default - // and default registrations are done from ServiceWiring instead. public function __construct( Config $config = null, LoggerInterface $logger = null ) { - $this->setLogger( $logger ?: new NullLogger() ); - $this->config = $config ?: MediaWikiServices::getInstance()->getMainConfig(); - // Source "local" is required by StartupModule - $this->addSource( 'local', $this->config->get( 'LoadScript' ) ); - $this->setMessageBlobStore( new MessageBlobStore( $this, $this->getLogger() ) ); + parent::__construct( $config ?: ResourceLoaderTestCase::getMinimalConfig(), $logger ); } public function getErrors() { diff --git a/tests/phpunit/includes/resourceloader/DerivativeResourceLoaderContextTest.php b/tests/phpunit/includes/resourceloader/DerivativeResourceLoaderContextTest.php index c210061191..76bde259df 100644 --- a/tests/phpunit/includes/resourceloader/DerivativeResourceLoaderContextTest.php +++ b/tests/phpunit/includes/resourceloader/DerivativeResourceLoaderContextTest.php @@ -16,7 +16,10 @@ class DerivativeResourceLoaderContextTest extends PHPUnit\Framework\TestCase { 'skin' => 'fallback', 'target' => 'test', ] ); - return new ResourceLoaderContext( new ResourceLoader(), $request ); + return new ResourceLoaderContext( + new ResourceLoader( ResourceLoaderTestCase::getMinimalConfig() ), + $request + ); } public function testChangeModules() { diff --git a/tests/phpunit/includes/resourceloader/MessageBlobStoreTest.php b/tests/phpunit/includes/resourceloader/MessageBlobStoreTest.php index 9afa232bd4..e094d92b9d 100644 --- a/tests/phpunit/includes/resourceloader/MessageBlobStoreTest.php +++ b/tests/phpunit/includes/resourceloader/MessageBlobStoreTest.php @@ -25,7 +25,7 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testBlobCreation() { $module = $this->makeModule( [ 'mainpage' ] ); - $rl = new ResourceLoader(); + $rl = new EmptyResourceLoader(); $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( null, $rl ); @@ -36,7 +36,7 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testBlobCreation_empty() { $module = $this->makeModule( [] ); - $rl = new ResourceLoader(); + $rl = new EmptyResourceLoader(); $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( null, $rl ); @@ -47,7 +47,7 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testBlobCreation_unknownMessage() { $module = $this->makeModule( [ 'i-dont-exist', 'mainpage', 'i-dont-exist2' ] ); - $rl = new ResourceLoader(); + $rl = new EmptyResourceLoader(); $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( null, $rl ); @@ -59,7 +59,7 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testMessageCachingAndPurging() { $module = $this->makeModule( [ 'example' ] ); - $rl = new ResourceLoader(); + $rl = new EmptyResourceLoader(); $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( [ 'fetchMessage' ], $rl ); @@ -104,7 +104,7 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testPurgeEverything() { $module = $this->makeModule( [ 'example' ] ); - $rl = new ResourceLoader(); + $rl = new EmptyResourceLoader(); $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( [ 'fetchMessage' ], $rl ); // Advance this new WANObjectCache instance to a normal state. @@ -138,7 +138,7 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { public function testValidateAgainstModuleRegistry() { // Arrange version 1 of a module $module = $this->makeModule( [ 'foo' ] ); - $rl = new ResourceLoader(); + $rl = new EmptyResourceLoader(); $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( [ 'fetchMessage' ], $rl ); $blobStore->expects( $this->once() ) @@ -157,7 +157,7 @@ class MessageBlobStoreTest extends PHPUnit\Framework\TestCase { // must always match the set of message keys required by the module. // We do not receive purges for this because no messages were changed. $module = $this->makeModule( [ 'foo', 'bar' ] ); - $rl = new ResourceLoader(); + $rl = new EmptyResourceLoader(); $rl->register( $module->getName(), $module ); $blobStore = $this->makeBlobStore( [ 'fetchMessage' ], $rl ); $blobStore->expects( $this->exactly( 2 ) ) diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php index 60cd4a8778..7f4d9a8137 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderContextTest.php @@ -15,6 +15,8 @@ class ResourceLoaderContextTest extends PHPUnit\Framework\TestCase { return new EmptyResourceLoader( new HashConfig( [ 'ResourceLoaderDebug' => false, 'LoadScript' => '/w/load.php', + // For ResourceLoader::register() + 'ResourceModuleSkinStyles' => [], ] ) ); } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php index 85a47de4b5..2e19e1ce39 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php @@ -111,7 +111,7 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { $resourceLoader->register( 'test.foo', new ResourceLoaderTestModule() ); $resourceLoader->register( 'test.bar', new ResourceLoaderTestModule() ); $this->assertEquals( - [ 'test.foo', 'test.bar' ], + [ 'startup', 'test.foo', 'test.bar' ], $resourceLoader->getModuleNames() ); } @@ -318,7 +318,7 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { * @covers ResourceLoader::getSources */ public function testAddSource( $name, $info, $expected ) { - $rl = new ResourceLoader; + $rl = new EmptyResourceLoader; $rl->addSource( $name, $info ); if ( is_array( $expected ) ) { foreach ( $expected as $source ) { @@ -333,7 +333,7 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { * @covers ResourceLoader::addSource */ public function testAddSourceDupe() { - $rl = new ResourceLoader; + $rl = new EmptyResourceLoader; $this->setExpectedException( MWException::class, 'ResourceLoader duplicate source addition error' ); @@ -345,7 +345,7 @@ class ResourceLoaderTest extends ResourceLoaderTestCase { * @covers ResourceLoader::addSource */ public function testAddSourceInvalid() { - $rl = new ResourceLoader; + $rl = new EmptyResourceLoader; $this->setExpectedException( MWException::class, 'with no "loadScript" key' ); $rl->addSource( 'foo', [ 'x' => 'https://example.org/w/load.php' ] ); } @@ -623,7 +623,7 @@ END * @covers ResourceLoader::getLoadScript */ public function testGetLoadScript() { - $rl = new ResourceLoader(); + $rl = new EmptyResourceLoader(); $sources = self::fakeSources(); $rl->addSource( $sources ); foreach ( [ 'examplewiki', 'example2wiki' ] as $name ) { @@ -881,12 +881,13 @@ END * @covers ResourceLoader::makeModuleResponse */ public function testMakeModuleResponseStartupError() { - $rl = new EmptyResourceLoader(); + // This is an integration test that uses a lot of MediaWiki state, + // provide the full Config object here. + $rl = new EmptyResourceLoader( MediaWikiServices::getInstance()->getMainConfig() ); $rl->register( [ 'foo' => self::getSimpleModuleMock( 'foo();' ), 'ferry' => self::getFailFerryMock(), 'bar' => self::getSimpleModuleMock( 'bar();' ), - 'startup' => [ 'class' => ResourceLoaderStartUpModule::class ], ] ); $context = $this->getResourceLoaderContext( [ @@ -897,7 +898,7 @@ END ); $this->assertEquals( - [ 'foo', 'ferry', 'bar', 'startup' ], + [ 'startup', 'foo', 'ferry', 'bar' ], $rl->getModuleNames(), 'getModuleNames' ); -- 2.20.1