From 9a4a754231482f3faf0061be1a177860654bcae7 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 8 Mar 2019 20:33:04 +0000 Subject: [PATCH] resourceloader: Replace ResourceLoaderDebug config use with context Reduce our reliance on static state and configuration, and propagate more state in explicit ways, through context, and request parameters. OutputPage creates ResourceLoaderContext and ResourceLoaderClientHtml based on the configuration (via ResourceLoader::inDebugMode). Everything within those classes should not need to check it again. * ResourceLoaderClientHtml: Already doesn't check MW config, but it's test was still mocking it. Removed now, and confirmed that it passes both with true and false. The individual test cases set debug=true/false as needed already. It's sets were previously relying on the accidental behaviour that within a unit test, we don't serialise over HTTP, which meant that a pure PHP boolean would survive. With the new raw `=== 'true'` check, this no longer works. Set it as a string explicitly instead, which is the only thing we support outside unit tests as well. * ResourceLoaderContext: Remove fallback to MW config when 'debug' is unset. This is never unset in practice given that all load.php urls have it set by OutputPage based on ResourceLoader::inDebugMode. This change means that manually constructed ad-hoc load.php urls that are missing 'debug=' parameter, will now always be read as debug=false. This was the default already, but could previously be changed through wgResourceLoaderDebug. When changing wgResourceLoaderDebug, everything will still have debug=true as before. The only change is when constructing load.php urls manually and explicitly not set it. Bug: T32956 Change-Id: Ie3424be46e2b8311968f3068ca08ba6a1139224a --- includes/resourceloader/ResourceLoader.php | 2 +- includes/resourceloader/ResourceLoaderContext.php | 5 +---- tests/phpunit/ResourceLoaderTestCase.php | 2 ++ .../resourceloader/ResourceLoaderClientHtmlTest.php | 7 +++---- .../phpunit/includes/resourceloader/ResourceLoaderTest.php | 2 +- 5 files changed, 8 insertions(+), 10 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index 3d1335049b..b36b8d0c76 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -1106,7 +1106,7 @@ MESSAGE; // mw.loader.implement will use globalEval if scripts is a string. // Minify manually here, because general response minification is // not effective due it being a string literal, not a function. - if ( !self::inDebugMode() ) { + if ( !$context->getDebug() ) { $scripts = self::filter( 'minify-js', $scripts ); // T107377 } } else { diff --git a/includes/resourceloader/ResourceLoaderContext.php b/includes/resourceloader/ResourceLoaderContext.php index 67de1921b2..a625970825 100644 --- a/includes/resourceloader/ResourceLoaderContext.php +++ b/includes/resourceloader/ResourceLoaderContext.php @@ -72,10 +72,7 @@ class ResourceLoaderContext implements MessageLocalizer { // Various parameters $this->user = $request->getRawVal( 'user' ); - $this->debug = $request->getFuzzyBool( - 'debug', - $this->getConfig()->get( 'ResourceLoaderDebug' ) - ); + $this->debug = $request->getRawVal( 'debug' ) === 'true'; $this->only = $request->getRawVal( 'only', null ); $this->version = $request->getRawVal( 'version', null ); $this->raw = $request->getFuzzyBool( 'raw' ); diff --git a/tests/phpunit/ResourceLoaderTestCase.php b/tests/phpunit/ResourceLoaderTestCase.php index cadd0ff3c7..ca5ff6c549 100644 --- a/tests/phpunit/ResourceLoaderTestCase.php +++ b/tests/phpunit/ResourceLoaderTestCase.php @@ -26,6 +26,7 @@ abstract class ResourceLoaderTestCase extends MediaWikiTestCase { $options = [ 'lang' => $options ]; } $options += [ + 'debug' => 'true', 'lang' => 'en', 'dir' => 'ltr', 'skin' => 'vector', @@ -35,6 +36,7 @@ abstract class ResourceLoaderTestCase extends MediaWikiTestCase { ]; $resourceLoader = $rl ?: new ResourceLoader(); $request = new FauxRequest( [ + 'debug' => $options['debug'], 'lang' => $options['lang'], 'modules' => $options['modules'], 'only' => $options['only'], diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php index 8fdf5dd075..3f9d52b450 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderClientHtmlTest.php @@ -21,7 +21,6 @@ class ResourceLoaderClientHtmlTest extends PHPUnit\Framework\TestCase { 'ResourceModuleSkinStyles' => [], 'ResourceModules' => [], 'EnableJavaScriptTest' => false, - 'ResourceLoaderDebug' => false, 'LoadScript' => '/w/load.php', ] ); return new ResourceLoaderContext( @@ -329,7 +328,7 @@ Deprecation message.' ] 'output' => '', ], [ - 'context' => [ 'sync' => true ], + 'context' => [ 'sync' => '1' ], 'modules' => [ 'test.scripts.raw' ], 'only' => ResourceLoaderModule::TYPE_SCRIPTS, 'output' => '', @@ -347,14 +346,14 @@ Deprecation message.' ] 'output' => '', ], [ - 'context' => [ 'debug' => true ], + 'context' => [ 'debug' => 'true' ], 'modules' => [ 'test.styles.pure', 'test.styles.mixed' ], 'only' => ResourceLoaderModule::TYPE_STYLES, 'output' => '' . "\n" . '', ], [ - 'context' => [ 'debug' => false ], + 'context' => [ 'debug' => 'false' ], 'modules' => [ 'test.styles.pure', 'test.styles.mixed' ], 'only' => ResourceLoaderModule::TYPE_STYLES, 'output' => '', diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php index f6bf7f1785..5941c6eb82 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderTest.php @@ -767,11 +767,11 @@ END }, $scripts ); $rl->register( $modules ); - $this->setMwGlobals( 'wgResourceLoaderDebug', $debug ); $context = $this->getResourceLoaderContext( [ 'modules' => implode( '|', array_keys( $modules ) ), 'only' => 'scripts', + 'debug' => $debug ? 'true' : 'false', ], $rl ); -- 2.20.1