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

diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php
index 02d5802..a507ad3 100644 (file)
--- 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 c925339..0c707d5 100644 (file)
--- 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
                );
        }