From 6d14529c691324bb132881044a65d6b764df7d8e Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 24 Sep 2021 23:52:05 +0100 Subject: [PATCH] resourceloader: Preserve new 'debug' param in getScriptURLsForDebug() Follows-up Ieaf04e0c289646dd5 which changed internal references to bool(true) for 'debug' to the integer DEBUG_ constants, and introduced a new debug=2 parameter. In the refactor, I missed the setDebug() calls for DerivativeResourceLoaderContext, which were still passing a boolean, but more importantly were effectively passing debug=1 even if the pageview carried debug=2. This isn't a problem yet in production since debug=2 is currently identical in behaviour to debug=1. The impact of this issue is mainly noticed through secondary CSS requests. The URLs for primary stylesheets, and JS modules was already forwarding the current "debug" version. Test Plan: * Open Main_Page?action=edit&debug=2 * Before this patch, e.g. on mediawiki.org today, secondary stylesheet requests (part of a JS module) have debug=1. For example "modules=jquery.makeCollapsible.styles&only=styles". * After this, everything has debug=2 when the page view has debug=2. Bug: T85805 Change-Id: Ia8fba4e30397bc5890033f13417b6739b0f83c38 --- .../resourceloader/ResourceLoaderModule.php | 14 ++++-- .../ResourceLoaderFileModuleTest.php | 17 +++++-- .../ResourceLoaderModuleTest.php | 45 +++++++++++++++++++ 3 files changed, 69 insertions(+), 7 deletions(-) diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 6df96a488fd..e83b0cfd180 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -295,7 +295,7 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface { * This function must only be called when: * * 1. We're in debug mode, - * 2. there is no `only=` parameter and, + * 2. There is no `only=` parameter and, * 3. self::supportsURLLoading() returns true. * * Point 2 is prevents an infinite loop, therefore this function @@ -310,7 +310,6 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface { $derivative = new DerivativeResourceLoaderContext( $context ); $derivative->setModules( [ $this->getName() ] ); $derivative->setOnly( 'scripts' ); - $derivative->setDebug( true ); $url = $rl->createLoaderURL( $this->getSource(), @@ -353,7 +352,15 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface { * Get the URL or URLs to load for this module's CSS in debug mode. * The default behavior is to return a load.php?only=styles URL for * the module, but file-based modules will want to override this to - * load the files directly. See also getScriptURLsForDebug() + * load the files directly + * + * This function must only be called when: + * + * 1. We're in debug mode, + * 2. There is no `only=` parameter and, + * 3. self::supportsURLLoading() returns true. + * + * See also getScriptURLsForDebug(). * * @stable to override * @param ResourceLoaderContext $context @@ -364,7 +371,6 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface { $derivative = new DerivativeResourceLoaderContext( $context ); $derivative->setModules( [ $this->getName() ] ); $derivative->setOnly( 'styles' ); - $derivative->setDebug( true ); $url = $resourceLoader->createLoaderURL( $this->getSource(), diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php index 51f15355a85..e66b7f36569 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php @@ -181,14 +181,17 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase { } /** - * @covers ResourceLoader::expandUrl * @covers ResourceLoaderFileModule + * @covers ResourceLoaderModule + * @covers ResourceLoader::createLoaderURL + * @covers ResourceLoader::expandUrl */ - public function testGetScriptURLsForDebug() { + public function testGetURLsForDebug() { $ctx = $this->getResourceLoaderContext(); $module = new ResourceLoaderFileModule( [ 'localBasePath' => __DIR__ . '/../../data/resourceloader', 'remoteBasePath' => '/w/something', + 'styles' => [ 'simple.css' ], 'scripts' => [ 'script-comment.js' ], ] ); $module->setName( 'testing' ); @@ -198,7 +201,15 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase { [ 'https://example.org/w/something/script-comment.js' ], - $module->getScriptURLsForDebug( $ctx ) + $module->getScriptURLsForDebug( $ctx ), + 'script urls' + ); + $this->assertEquals( + [ 'all' => [ + '/w/something/simple.css' + ] ], + $module->getStyleURLsForDebug( $ctx ), + 'style urls' ); } diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php index 6a8905bafbc..69a6711826b 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php @@ -90,6 +90,51 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase { $module->getVersionHash( $context ); } + /** + * @covers ResourceLoaderModule::getScriptURLsForDebug + * @covers ResourceLoader + * @covers ResourceLoaderModule::getScriptURLsForDebug + */ + public function testGetURLsForDebug() { + $module = new ResourceLoaderTestModule( [ + 'script' => 'foo();', + 'styles' => '.foo { color: blue; }', + ] ); + $context = $this->getResourceLoaderContext( [ 'debug' => 'true' ] ); + $module->setConfig( $context->getResourceLoader()->getConfig() ); + + $this->assertEquals( + [ + 'https://example.org/w/load.php?debug=1&lang=en&modules=&only=scripts' + ], + $module->getScriptURLsForDebug( $context ), + 'script urls debug=true' + ); + $this->assertEquals( + [ 'all' => [ + '/w/load.php?debug=1&lang=en&modules=&only=styles' + ] ], + $module->getStyleURLsForDebug( $context ), + 'style urls debug=true' + ); + + $context = $this->getResourceLoaderContext( [ 'debug' => '2' ] ); + $this->assertEquals( + [ + 'https://example.org/w/load.php?debug=2&lang=en&modules=&only=scripts' + ], + $module->getScriptURLsForDebug( $context ), + 'script urls debug=2' + ); + $this->assertEquals( + [ 'all' => [ + '/w/load.php?debug=2&lang=en&modules=&only=styles' + ] ], + $module->getStyleURLsForDebug( $context ), + 'style urls debug=2' + ); + } + /** * @covers ResourceLoaderModule::validateScriptFile */