resourceloader: Use 'enableModuleContentVersion' for startup module

This significantly simplifies the getVersionHash implementation for
StartupModule, and fixes a couple of bugs.

Previously, the startup module's E-Tag was determined by the
'getDefinitionSummary' method, which combined the E-Tag values
from all registered modules, plus what we thought is all information
used by 'getScript' (config vars, embedded script files, list
of base modules, ...)

However, this were various things part of the manifest that it
forgot about, including:

* Changes to the list of dependencies of a module.
* Changes to the name of module.
* Changes to the cache group of module.
* Adding or removing a foreign module source (mw.loader.addSource).

These are all quite rare, and when they do change, they usually
also involve a change that *was* tracked already. But, sometimes
they don't and that's when bugs happened.

Instead of the tracking array of getDefinitionSummary, we now
use the 'enableModuleContentVersion' option for StartupModule,
which simply calls the actual getScript() method and hashes that.

Of note: When an exception happens with the version computation of
any individual module, we catch it, log it, and continue with the
rest. Previously, the first time such error was discovered at
run-time would be in the getCombinedVersion() call from
StartupModule::getAllModuleHashes(). That public getCombinedVersion()
method of ResourceLoader had the benefit of also outputting details
of that exception in the HTTP response output. In order to keep that
behaviour, I made outputErrorAndLog() public so that StartupModule
can call it directly now. This is covered by
ResourceLoaderTest::testMakeModuleResponseStartupError.

Bug: T201686
Change-Id: I8e8d3a2cd2ccd68d2d78e988bcdd0d77fbcbf1d4

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index fc0ca1d..8f5d083 100644 (file)
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -627,14 +627,13 @@ class ResourceLoader implements LoggerAwareInterface {
        /**
         * Add an error to the 'errors' array and log it.
         *
-        * Should only be called from within respond().
-        *
+        * @private For internal use by ResourceLoader and ResourceLoaderStartUpModule.
         * @since 1.29
         * @param Exception $e
         * @param string $msg
         * @param array $context
         */
-       protected function outputErrorAndLog( Exception $e, $msg, array $context = [] ) {
+       public function outputErrorAndLog( Exception $e, $msg, array $context = [] ) {
                MWExceptionHandler::logException( $e );
                $this->logger->warning(
                        $msg,
@@ -659,9 +658,8 @@ class ResourceLoader implements LoggerAwareInterface {
                        try {
                                return $this->getModule( $module )->getVersionHash( $context );
                        } catch ( Exception $e ) {
-                               // If modules fail to compute a version, do still consider the versions
-                               // of other modules - don't set an empty string E-Tag for the whole request.
-                               // See also T152266 and StartupModule::getModuleRegistrations().
+                               // If modules fail to compute a version, don't fail the request (T152266)
+                               // and still compute versions of other modules.
                                $this->outputErrorAndLog( $e,
                                        'Calculating version for "{module}" failed: {exception}',
                                        [

diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php
index 18cc4d5..8140c2c 100644 (file)
--- a/includes/resourceloader/ResourceLoaderStartUpModule.php
+++ b/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -40,21 +40,13 @@ use MediaWiki\MediaWikiServices;
  *   See also: OutputPage::disallowUserJs()
  */
 class ResourceLoaderStartUpModule extends ResourceLoaderModule {
-
-       // Cache for getConfigSettings() as it's called by multiple methods
-       protected $configVars = [];
        protected $targets = [ 'desktop', 'mobile' ];
 
        /**
         * @param ResourceLoaderContext $context
         * @return array
         */
-       protected function getConfigSettings( $context ) {
-               $hash = $context->getHash();
-               if ( isset( $this->configVars[$hash] ) ) {
-                       return $this->configVars[$hash];
-               }
-
+       private function getConfigSettings( $context ) {
                $conf = $this->getConfig();
 
                // We can't use Title::newMainPage() if 'mainpage' is in
@@ -136,8 +128,7 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
 
                Hooks::run( 'ResourceLoaderGetConfigVars', [ &$vars ] );
 
-               $this->configVars[$hash] = $vars;
-               return $this->configVars[$hash];
+               return $vars;
        }
 
        /**
@@ -222,9 +213,23 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
                $out = '';
                $states = [];
                $registryData = [];
+               $moduleNames = $resourceLoader->getModuleNames();
+
+               // Preload with a batch so that the below calls to getVersionHash() for each module
+               // don't require on-demand loading of more information.
+               try {
+                       $resourceLoader->preloadModuleInfo( $moduleNames, $context );
+               } catch ( Exception $e ) {
+                       // Don't fail the request (T152266)
+                       // Also print the error in the main output
+                       $resourceLoader->outputErrorAndLog( $e,
+                               'Preloading module info from startup failed: {exception}',
+                               [ 'exception' => $e ]
+                       );
+               }
 
                // Get registry data
-               foreach ( $resourceLoader->getModuleNames() as $name ) {
+               foreach ( $moduleNames as $name ) {
                        $module = $resourceLoader->getModule( $name );
                        $moduleTargets = $module->getTargets();
                        if (
@@ -235,18 +240,25 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
                        }
 
                        if ( $module->isRaw() ) {
-                               // Don't register "raw" modules (like 'jquery' and 'mediawiki') client-side because
-                               // depending on them is illegal anyway and would only lead to them being reloaded
-                               // causing any state to be lost (like jQuery plugins, mw.config etc.)
+                               // Don't register "raw" modules (like 'startup') client-side because depending on them
+                               // is illegal anyway and would only lead to them being loaded a second time,
+                               // causing any state to be lost.
+
+                               // ATTENTION: Because of the line below, this is not going to cause infinite recursion.
+                               // Think carefully before making changes to this code!
+                               // The below code is going to call ResourceLoaderModule::getVersionHash() for every module.
+                               // For StartUpModule (this module) the hash is computed based on the manifest content,
+                               // which is the very thing we are computing right here. As such, this must skip iterating
+                               // over 'startup' itself.
                                continue;
                        }
 
                        try {
                                $versionHash = $module->getVersionHash( $context );
                        } catch ( Exception $e ) {
-                               // See also T152266 and ResourceLoader::getCombinedVersion()
-                               MWExceptionHandler::logException( $e );
-                               $context->getLogger()->warning(
+                               // Don't fail the request (T152266)
+                               // Also print the error in the main output
+                               $resourceLoader->outputErrorAndLog( $e,
                                        'Calculating version for "{module}" failed: {exception}',
                                        [
                                                'module' => $name,
@@ -445,59 +457,12 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
        }
 
        /**
-        * Get the definition summary for this module.
-        *
-        * @param ResourceLoaderContext $context
-        * @return array
-        */
-       public function getDefinitionSummary( ResourceLoaderContext $context ) {
-               global $IP;
-               $summary = parent::getDefinitionSummary( $context );
-               $startup = [
-                       // getScript() exposes these variables to mw.config (T30899).
-                       'vars' => $this->getConfigSettings( $context ),
-                       // getScript() uses this to decide how configure mw.Map for mw.config.
-                       'wgLegacyJavaScriptGlobals' => $this->getConfig()->get( 'LegacyJavaScriptGlobals' ),
-                       // Detect changes to the module registrations output by getScript().
-                       'moduleHashes' => $this->getAllModuleHashes( $context ),
-                       // Detect changes to base modules listed by getScript().
-                       'baseModules' => $this->getBaseModules(),
-
-                       'fileHashes' => [
-                               $this->safeFileHash( "$IP/resources/src/startup/startup.js" ),
-                               $this->safeFileHash( "$IP/resources/src/startup/mediawiki.js" ),
-                               $this->safeFileHash( "$IP/resources/src/startup/mediawiki.requestIdleCallback.js" ),
-                       ],
-               ];
-               if ( $context->getDebug() ) {
-                       $startup['fileHashes'][] = $this->safeFileHash( "$IP/resources/src/startup/mediawiki.log.js" );
-               }
-               if ( $this->getConfig()->get( 'ResourceLoaderEnableJSProfiler' ) ) {
-                       $startup['fileHashes'][] = $this->safeFileHash( "$IP/resources/src/startup/profiling.js" );
-               }
-               $summary[] = $startup;
-               return $summary;
-       }
-
-       /**
-        * Helper method for getDefinitionSummary().
-        *
-        * @param ResourceLoaderContext $context
-        * @return string SHA-1
+        * @return bool
         */
-       protected function getAllModuleHashes( ResourceLoaderContext $context ) {
-               $rl = $context->getResourceLoader();
-               // Preload for getCombinedVersion()
-               $rl->preloadModuleInfo( $rl->getModuleNames(), $context );
-
-               // ATTENTION: Because of the line below, this is not going to cause infinite recursion.
-               // Think carefully before making changes to this code!
-               // Pre-populate versionHash with something because the loop over all modules below includes
-               // the startup module (this module).
-               // See ResourceLoaderModule::getVersionHash() for usage of this cache.
-               $this->versionHash[$context->getHash()] = null;
-
-               return $rl->getCombinedVersion( $context, $rl->getModuleNames() );
+       public function enableModuleContentVersion() {
+               // Enabling this means that ResourceLoader::getVersionHash will simply call getScript()
+               // and hash it to determine the version (as used by E-Tag HTTP response header).
+               return true;
        }
 
        /**

diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php
index 86956f2..aa3f820 100644 (file)
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php
@@ -572,6 +572,7 @@ mw.loader.register( [
                $version1 = $module->getVersionHash( $context );
                $module = new ResourceLoaderStartupModule();
                $version2 = $module->getVersionHash( $context );
+
                $this->setMwGlobals( 'wgArticlePath', '/w3' );
                $module = new ResourceLoaderStartupModule();
                $version3 = $module->getVersionHash( $context );
@@ -590,8 +591,7 @@ mw.loader.register( [
        }
 
        /**
-        * @covers ResourceLoaderStartupModule::getAllModuleHashes
-        * @covers ResourceLoaderStartupModule::getDefinitionSummary
+        * @covers ResourceLoaderStartupModule
         */
        public function testGetVersionHash_varyModule() {
                $context1 = $this->getResourceLoaderContext();
@@ -621,10 +621,11 @@ mw.loader.register( [
                $module = new ResourceLoaderStartupModule();
                $version3 = $module->getVersionHash( $context3 );
 
-               $this->assertEquals(
+               // Module name *is* significant (T201686)
+               $this->assertNotEquals(
                        $version1,
                        $version2,
-                       'Module name is insignificant'
+                       'Module name is significant'
                );
 
                $this->assertNotEquals(
@@ -634,4 +635,32 @@ mw.loader.register( [
                );
        }
 
+       /**
+        * @covers ResourceLoaderStartupModule
+        */
+       public function testGetVersionHash_varyDeps() {
+               $context = $this->getResourceLoaderContext();
+               $rl = $context->getResourceLoader();
+               $rl->register( [
+                       'test.a' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'x', 'y' ] ] ),
+               ] );
+               $module = new ResourceLoaderStartupModule();
+               $version1 = $module->getVersionHash( $context );
+
+               $context = $this->getResourceLoaderContext();
+               $rl = $context->getResourceLoader();
+               $rl->register( [
+                       'test.a' => new ResourceLoaderTestModule( [ 'dependencies' => [ 'x', 'z' ] ] ),
+               ] );
+               $module = new ResourceLoaderStartupModule();
+               $version2 = $module->getVersionHash( $context );
+
+               // Dependencies *are* significant (T201686)
+               $this->assertNotEquals(
+                       $version1,
+                       $version2,
+                       'Dependencies are significant'
+               );
+       }
+
 }