resourceloader: Use makeVersionQuery for 'version' query parameter

makeVersionQuery and getCombinedVersion return the same string in
most cases, but there are exceptions, and it could diverge further
in the future. Use the semantically correct method.

Before 6fa1e56 it didn't matter how 'version' was computed as long
as it's deterministic and sufficiently unique. Now that we validate
the hashes it's important all methods use the same logic.

Rename method to makeVersionQuery since it's no longer used just
for comparison against the expected value.

Change-Id: I19f5818e27c8a0920d3d1374b40aeb0b6ba0b614

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index fa110e3..3cb30bc 100644 (file)
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -637,7 +637,7 @@ class ResourceLoader implements LoggerAwareInterface {
         * @param string[] $modules List of module names
         * @return string Hash
         */
-       public function getExpectedVersionQuery( ResourceLoaderContext $context ) {
+       public function makeVersionQuery( ResourceLoaderContext $context ) {
                // As of MediaWiki 1.28, the server and client use the same algorithm for combining
                // version hashes. There is no technical reason for this to be same, and for years the
                // implementations differed. If getCombinedVersion in PHP (used for StartupModule and
@@ -797,7 +797,7 @@ class ResourceLoader implements LoggerAwareInterface {
                // - Version mismatch (T117587, T47877)
                if ( is_null( $context->getVersion() )
                        || $errors
-                       || $context->getVersion() !== $this->getExpectedVersionQuery( $context )
+                       || $context->getVersion() !== $this->makeVersionQuery( $context )
                ) {
                        $maxage = $rlMaxage['unversioned']['client'];
                        $smaxage = $rlMaxage['unversioned']['server'];

diff --git a/includes/resourceloader/ResourceLoaderClientHtml.php b/includes/resourceloader/ResourceLoaderClientHtml.php
index dc70af4..5729218 100644 (file)
--- a/includes/resourceloader/ResourceLoaderClientHtml.php
+++ b/includes/resourceloader/ResourceLoaderClientHtml.php
@@ -429,6 +429,7 @@ class ResourceLoaderClientHtml {
                foreach ( $sortedModules as $source => $groups ) {
                        foreach ( $groups as $group => $grpModules ) {
                                $context = self::makeContext( $mainContext, $group, $only, $extraQuery );
+                               $context->setModules( array_keys( $grpModules ) );
 
                                if ( $group === 'private' ) {
                                        // Decide whether to use style or script element
@@ -456,11 +457,10 @@ class ResourceLoaderClientHtml {
                                // This should NOT be done for the site group (bug 27564) because anons get that too
                                // and we shouldn't be putting timestamps in CDN-cached HTML
                                if ( $group === 'user' ) {
-                                       $version = $rl->getCombinedVersion( $context, array_keys( $grpModules ) );
-                                       $context->setVersion( $version );
+                                       // Must setModules() before makeVersionQuery()
+                                       $context->setVersion( $rl->makeVersionQuery( $context ) );
                                }
 
-                               $context->setModules( array_keys( $grpModules ) );
                                $url = $rl->createLoaderURL( $source, $context, $extraQuery );
 
                                // Decide whether to use 'style' or 'script' element

diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php
index c854fa2..8970620 100644 (file)
--- a/includes/resourceloader/ResourceLoaderStartUpModule.php
+++ b/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -311,14 +311,12 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
         */
        public static function getStartupModulesUrl( ResourceLoaderContext $context ) {
                $rl = $context->getResourceLoader();
-               $moduleNames = self::getStartupModules();
 
                $derivative = new DerivativeResourceLoaderContext( $context );
-               $derivative->setModules( $moduleNames );
+               $derivative->setModules( self::getStartupModules() );
                $derivative->setOnly( 'scripts' );
-               $derivative->setVersion(
-                       $rl->getCombinedVersion( $context, $moduleNames )
-               );
+               // Must setModules() before makeVersionQuery()
+               $derivative->setVersion( $rl->makeVersionQuery( $derivative ) );
 
                return $rl->createLoaderURL( 'local', $derivative );
        }