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

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index 418f532..515f287 100644 (file)
--- 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 c834db1..d6cc646 100644 (file)
--- 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 e9a8a1f..bd6df5f 100644 (file)
--- 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 c210061..76bde25 100644 (file)
--- 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 9afa232..e094d92 100644 (file)
--- 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 60cd4a8..7f4d9a8 100644 (file)
--- 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 85a47de..2e19e1c 100644 (file)
--- 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'
                );