From e42837e97753cd5e03a8dbab865c3ca8a6bcfb93 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 30 Aug 2018 02:42:24 +0100 Subject: [PATCH] resourceloader: Remove selective build optimisation from getModuleContent() This follows 5ddd7f91c7, which factored out response building from ResourceLoader.php to ResourceLoaderModule::buildContent. As optimisation, I made this method only return the array keys needed for the current response; based $context->getOnly(). The reason for this refactoring was the creation of the 'enableModuleContentVersion' option to getVersionHash(), which would use this method to create a module response, and hash it. During the implementation of that option, I ran into a problem. getVersionHash() is called by the startup module for each registered module, to create the manifest. The context for the StartupModule request itself has "only=scripts". But, we must still compute the version hashes for whole modules, not just their scripts. I worked around that problem in aac831f9fa by creating a mock context in getVersionHash() that stubs out the 'only' parameter. This worked, but made the assumption that the scripts and styles of a module cannot differ based on the 'only' parameter. This assumption was wrong, because the 'only' parameter is part of ResourceLoaderContext and available to all getters to vary on. Fortunately, the 'enableModuleContentVersion' option is off by default and nobody currently using it was differing its output by the 'only' parameter. I intend to make use of the 'enableModuleContentVersion' option in StartupModule to fix T201686. And StartupModule outputs a manifest if the request specifies only=scripts, and outputs a warning otherwise. As such, it cannot compute its version if the 'only' parameter is stubbed out. * Remove the 'only' parameter stubbing. * Remove the selective building from the buildContent() method. This was not very useful because we need to build the whole module regardless when computing the version. As benefit, this means the in-process cache is now shared between the call from getVersionHash and the call from makeModuleResponse. Bug: T201686 Change-Id: I8a17888f95f86ac795bc2de43086225b8a8f4b78 --- .../resourceloader/ResourceLoaderModule.php | 128 ++++++++---------- .../ResourceLoaderModuleTest.php | 4 +- 2 files changed, 60 insertions(+), 72 deletions(-) diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 02d5802e97..a507ad3ef1 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -689,79 +689,77 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface { $stats = MediaWikiServices::getInstance()->getStatsdDataFactory(); $statStart = microtime( true ); - // Only include properties that are relevant to this context (e.g. only=scripts) - // and that are non-empty (e.g. don't include "templates" for modules without - // templates). This helps prevent invalidating cache for all modules when new - // optional properties are introduced. + // This MUST build both scripts and styles, regardless of whether $context->getOnly() + // is 'scripts' or 'styles' because the result is used by getVersionHash which + // must be consistent regardles of the 'only' filter on the current request. + // Also, when introducing new module content resources (e.g. templates, headers), + // these should only be included in the array when they are non-empty so that + // existing modules not using them do not get their cache invalidated. $content = []; // Scripts - if ( $context->shouldIncludeScripts() ) { - // If we are in debug mode, we'll want to return an array of URLs if possible - // However, we can't do this if the module doesn't support it - // We also can't do this if there is an only= parameter, because we have to give - // the module a way to return a load.php URL without causing an infinite loop - if ( $context->getDebug() && !$context->getOnly() && $this->supportsURLLoading() ) { - $scripts = $this->getScriptURLsForDebug( $context ); - } else { - $scripts = $this->getScript( $context ); - // Make the script safe to concatenate by making sure there is at least one - // trailing new line at the end of the content. Previously, this looked for - // a semi-colon instead, but that breaks concatenation if the semicolon - // is inside a comment like "// foo();". Instead, simply use a - // line break as separator which matches JavaScript native logic for implicitly - // ending statements even if a semi-colon is missing. - // Bugs: T29054, T162719. - if ( is_string( $scripts ) - && strlen( $scripts ) - && substr( $scripts, -1 ) !== "\n" - ) { - $scripts .= "\n"; - } + // If we are in debug mode, we'll want to return an array of URLs if possible + // However, we can't do this if the module doesn't support it. + // We also can't do this if there is an only= parameter, because we have to give + // the module a way to return a load.php URL without causing an infinite loop + if ( $context->getDebug() && !$context->getOnly() && $this->supportsURLLoading() ) { + $scripts = $this->getScriptURLsForDebug( $context ); + } else { + $scripts = $this->getScript( $context ); + // Make the script safe to concatenate by making sure there is at least one + // trailing new line at the end of the content. Previously, this looked for + // a semi-colon instead, but that breaks concatenation if the semicolon + // is inside a comment like "// foo();". Instead, simply use a + // line break as separator which matches JavaScript native logic for implicitly + // ending statements even if a semi-colon is missing. + // Bugs: T29054, T162719. + if ( is_string( $scripts ) + && strlen( $scripts ) + && substr( $scripts, -1 ) !== "\n" + ) { + $scripts .= "\n"; } - $content['scripts'] = $scripts; } + $content['scripts'] = $scripts; // Styles - if ( $context->shouldIncludeStyles() ) { - $styles = []; - // Don't create empty stylesheets like [ '' => '' ] for modules - // that don't *have* any stylesheets (T40024). - $stylePairs = $this->getStyles( $context ); - if ( count( $stylePairs ) ) { - // If we are in debug mode without &only= set, we'll want to return an array of URLs - // See comment near shouldIncludeScripts() for more details - if ( $context->getDebug() && !$context->getOnly() && $this->supportsURLLoading() ) { - $styles = [ - 'url' => $this->getStyleURLsForDebug( $context ) - ]; - } else { - // Minify CSS before embedding in mw.loader.implement call - // (unless in debug mode) - if ( !$context->getDebug() ) { - foreach ( $stylePairs as $media => $style ) { - // Can be either a string or an array of strings. - if ( is_array( $style ) ) { - $stylePairs[$media] = []; - foreach ( $style as $cssText ) { - if ( is_string( $cssText ) ) { - $stylePairs[$media][] = - ResourceLoader::filter( 'minify-css', $cssText ); - } + $styles = []; + // Don't create empty stylesheets like [ '' => '' ] for modules + // that don't *have* any stylesheets (T40024). + $stylePairs = $this->getStyles( $context ); + if ( count( $stylePairs ) ) { + // If we are in debug mode without &only= set, we'll want to return an array of URLs + // See comment near shouldIncludeScripts() for more details + if ( $context->getDebug() && !$context->getOnly() && $this->supportsURLLoading() ) { + $styles = [ + 'url' => $this->getStyleURLsForDebug( $context ) + ]; + } else { + // Minify CSS before embedding in mw.loader.implement call + // (unless in debug mode) + if ( !$context->getDebug() ) { + foreach ( $stylePairs as $media => $style ) { + // Can be either a string or an array of strings. + if ( is_array( $style ) ) { + $stylePairs[$media] = []; + foreach ( $style as $cssText ) { + if ( is_string( $cssText ) ) { + $stylePairs[$media][] = + ResourceLoader::filter( 'minify-css', $cssText ); } - } elseif ( is_string( $style ) ) { - $stylePairs[$media] = ResourceLoader::filter( 'minify-css', $style ); } + } elseif ( is_string( $style ) ) { + $stylePairs[$media] = ResourceLoader::filter( 'minify-css', $style ); } } - // Wrap styles into @media groups as needed and flatten into a numerical array - $styles = [ - 'css' => $rl->makeCombinedStyles( $stylePairs ) - ]; } + // Wrap styles into @media groups as needed and flatten into a numerical array + $styles = [ + 'css' => $rl->makeCombinedStyles( $stylePairs ) + ]; } - $content['styles'] = $styles; } + $content['styles'] = $styles; // Messages $blob = $this->getMessageBlob( $context ); @@ -805,22 +803,12 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface { * @return string Hash (should use ResourceLoader::makeHash) */ public function getVersionHash( ResourceLoaderContext $context ) { - // The startup module produces a manifest with versions representing the entire module. - // Typically, the request for the startup module itself has only=scripts. That must apply - // only to the startup module content, and not to the module version computed here. - $context = new DerivativeResourceLoaderContext( $context ); - $context->setModules( [] ); - // Version hash must cover all resources, regardless of startup request itself. - $context->setOnly( null ); - // Compute version hash based on content, not debug urls. - $context->setDebug( false ); - // Cache this somewhat expensive operation. Especially because some classes // (e.g. startup module) iterate more than once over all modules to get versions. $contextHash = $context->getHash(); if ( !array_key_exists( $contextHash, $this->versionHash ) ) { if ( $this->enableModuleContentVersion() ) { - // Detect changes directly + // Detect changes directly by hashing the module contents. $str = json_encode( $this->getModuleContent( $context ) ); } else { // Infer changes based on definition and other metrics diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php index c9253392cb..0c707d5537 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php @@ -151,8 +151,8 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase { ] ); $this->assertEquals( $raw, $module->getScript( $context ), 'Raw script' ); $this->assertEquals( - [ 'scripts' => $build ], - $module->getModuleContent( $context ), + $build, + $module->getModuleContent( $context )[ 'scripts' ], $message ); } -- 2.20.1