resourceloader: Make getVersionHash() final

This is in preparation for making all version hashes the empty string
in debug mode, which will make things faster to work with, but also
helps solve T235672 in an easy way client-side without having to
add additional complexity to the mw.loader client that is specific
to debug mode.

Bug: T235672
Change-Id: I43204f22dfbcf5d236b35adc5b35df3da8021bad

includes/resourceloader/ResourceLoaderModule.php

@@ -842,20 +842,21 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface {
 	 * Get a string identifying the current version of this module in a given context.
 	 *
 	 * Whenever anything happens that changes the module's response (e.g. scripts, styles, and
-	 * messages) this value must change. This value is used to store module responses in cache.
-	 * (Both client-side and server-side.)
+	 * messages) this value must change. This value is used to store module responses in caches,
+	 * both server-side (by a CDN, or other HTTP cache), and client-side (in `mw.loader.store`,
+	 * and in the browser's own HTTP cache).
 	 *
-	 * It is not recommended to override this directly. Use getDefinitionSummary() instead.
-	 * If overridden, one must call the parent getVersionHash(), append data and re-hash.
-	 *
-	 * This method should be quick because it is frequently run by ResourceLoaderStartUpModule to
-	 * propagate changes to the client and effectively invalidate cache.
+	 * The underlying methods called here for any given module should be quick because this
+	 * is called for potentially thousands of module bundles in the same request as part of the
+	 * ResourceLoaderStartUpModule, which is how we invalidate caches and propagate changes to
+	 * clients.
 	 *
 	 * @since 1.26
+	 * @see self::getDefinitionSummary for how to customize version computation.
 	 * @param ResourceLoaderContext $context
-	 * @return string Hash (should use ResourceLoader::makeHash)
+	 * @return string Hash formatted by ResourceLoader::makeHash
 	 */
-	public function getVersionHash( ResourceLoaderContext $context ) {
+	final public function getVersionHash( ResourceLoaderContext $context ) {
 		// 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();

includes/resourceloader/ResourceLoaderStartUpModule.php

@@ -204,6 +204,9 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
 			}
 
 			try {
+				// The version should be formatted by ResourceLoader::makeHash and be of
+				// length ResourceLoader::HASH_LENGTH.
+				// The getVersionHash method is final and is covered by tests, as is makeHash().
 				$versionHash = $module->getVersionHash( $context );
 			} catch ( Exception $e ) {
 				// Don't fail the request (T152266)

tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php

@@ -64,6 +64,18 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase {
 		);
 	}
 
+	/**
+	 * @covers ResourceLoaderModule::getVersionHash
+	 */
+	public function testGetVersionHash_length() {
+		$context = $this->getResourceLoaderContext();
+		$module = new ResourceLoaderTestModule( [
+			'script' => 'foo();'
+		] );
+		$version = $module->getVersionHash( $context );
+		$this->assertSame( ResourceLoader::HASH_LENGTH, strlen( $version ), 'Hash length' );
+	}
+
 	/**
 	 * @covers ResourceLoaderModule::getVersionHash
 	 */

tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php

@@ -227,13 +227,13 @@ mw.loader.register([
 ]);',
 			] ],
 			[ [
-				'msg' => 'Version falls back gracefully if getVersionHash throws',
+				'msg' => 'Version falls back gracefully if getModuleContent throws',
 				'modules' => [
 					'test.fail' => [
 						'factory' => function () {
 							$mock = $this->getMockBuilder( ResourceLoaderTestModule::class )
-								->onlyMethods( [ 'getVersionHash' ] )->getMock();
-							$mock->method( 'getVersionHash' )->will(
+								->onlyMethods( [ 'getModuleContent' ] )->getMock();
+							$mock->method( 'getModuleContent' )->will(
 								$this->throwException( new Exception )
 							);
 							return $mock;
@@ -255,13 +255,20 @@ mw.loader.state({
 });',
 			] ],
 			[ [
-				'msg' => 'Use version from getVersionHash',
+				'msg' => 'Version falls back gracefully if getDefinitionSummary throws',
 				'modules' => [
-					'test.version' => [
+					'test.fail' => [
 						'factory' => function () {
 							$mock = $this->getMockBuilder( ResourceLoaderTestModule::class )
-								->onlyMethods( [ 'getVersionHash' ] )->getMock();
-							$mock->method( 'getVersionHash' )->willReturn( '12345' );
+								->onlyMethods( [
+									'enableModuleContentVersion',
+									'getDefinitionSummary'
+								] )
+								->getMock();
+							$mock->method( 'enableModuleContentVersion' )->willReturn( false );
+							$mock->method( 'getDefinitionSummary' )->will(
+								$this->throwException( new Exception )
+							);
 							return $mock;
 						}
 					]
@@ -272,33 +279,13 @@ mw.loader.addSource({
 });
 mw.loader.register([
     [
-        "test.version",
-        "12345"
+        "test.fail",
+        ""
     ]
-]);',
-			] ],
-			[ [
-				'msg' => 'Re-hash version from getVersionHash if too long',
-				'modules' => [
-					'test.version' => [
-						'factory' => function () {
-							$mock = $this->getMockBuilder( ResourceLoaderTestModule::class )
-								->onlyMethods( [ 'getVersionHash' ] )->getMock();
-							$mock->method( 'getVersionHash' )->willReturn( '12345678' );
-							return $mock;
-						}
-					],
-				],
-				'out' => '
-mw.loader.addSource({
-    "local": "/w/load.php"
-});
-mw.loader.register([
-    [
-        "test.version",
-        "16es8"
-    ]
-]);',
+]);
+mw.loader.state({
+    "test.fail": "error"
+});',
 			] ],
 			[ [
 				'msg' => 'Group signature',

tests/phpunit/includes/resourceloader/ResourceLoaderTest.php

@@ -210,6 +210,17 @@ class ResourceLoaderTest extends ResourceLoaderTestCase {
 		);
 	}
 
+	/**
+	 * @covers ResourceLoader::makeHash
+	 */
+	public function testGetVersionHash_length() {
+		$hash = ResourceLoader::makeHash(
+			'Anything you do could have serious repercussions on future events.'
+		);
+		$this->assertSame( 'xhh1x', $hash, 'Hash' );
+		$this->assertSame( ResourceLoader::HASH_LENGTH, strlen( $hash ), 'Hash length' );
+	}
+
 	/**
 	 * @covers ResourceLoader::getLessCompiler
 	 */