From cf99077b0b21613dbcdff4a0602684c96cd7f356 Mon Sep 17 00:00:00 2001
From: Timo Tijhof <krinklemail@gmail.com>
Date: Wed, 7 Sep 2016 14:39:22 -0700
Subject: [PATCH] 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
---
 includes/resourceloader/ResourceLoader.php              | 4 ++--
 includes/resourceloader/ResourceLoaderClientHtml.php    | 6 +++---
 includes/resourceloader/ResourceLoaderStartUpModule.php | 8 +++-----
 3 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index fa110e3ae4..3cb30bc185 100644
--- 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 dc70af458e..572921817e 100644
--- 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 c854fa26c9..8970620720 100644
--- 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 );
 	}
-- 
2.20.1