From 2581453facc8cf347fa9972626aeaf8383f791e1 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Tue, 2 Jun 2015 18:27:23 +0100 Subject: [PATCH] resourceloader: Enable module content version for data modules This greatly simplifies logic required to compute module versions. It also makes it significantly less error-prone. Since f37cee996e, we support hashes as versions (instead of timestamps). This means we can build a hash of the content directly, instead of compiling a large array with all values that may influence the module content somehow. Benefits: * Remove all methods and logic related to querying database and disk for timestamps, revision numbers, definition summaries, cache epochs, and more. * No longer needlessly invalidate cache as a result of no-op changes to implementation datails. Due to inclusion of absolute file paths in the definition summary, cache was always invalidated when moving wikis to newer MediaWiki branches; even if the module observed no actual changes. * When changes are reverted within a certain period of time, old caches can now be re-used. The module would produce the same version hash as before. Previously when a change was deployed and then reverted, all web clients (even those that never saw the bad version) would have re-fetch modules because the version increased. Updated unit tests to account for the change in version. New default version of empty test modules is: "mvgTPvXh". For the record, this comes from the base64 encoding of the SHA1 digest of the JSON serialised form of the module content: > $str = '{"scripts":"","styles":{"css":[]},"messagesBlob":"{}"}'; > echo base64_encode(sha1($str, true)); > FEb3+VuiUm/fOMfod1bjw/te+AQ= Enabled content versioning for the data modules in MediaWiki core: * EditToolbarModule * JqueryMsgModule * LanguageDataModule * LanguageNamesModule * SpecialCharacterDataModule * UserCSSPrefsModule * UserDefaultsModule * UserOptionsModule The FileModule and base class explicitly disable it for now and keep their current behaviour of using the definition summary. We may remove it later, but that requires more performance testing first. Explicitly disable it in the WikiModule class to avoid breakage when the default changes. Ref T98087. Change-Id: I782df43c50dfcfb7d7592f744e13a3a0430b0dc6 --- .../ResourceLoaderEditToolbarModule.php | 11 ++--- .../ResourceLoaderFileModule.php | 12 +++++ .../ResourceLoaderJqueryMsgModule.php | 11 ++--- .../ResourceLoaderLanguageDataModule.php | 7 ++- .../ResourceLoaderLanguageNamesModule.php | 8 ++-- .../resourceloader/ResourceLoaderModule.php | 47 ++++++++++++------ ...sourceLoaderSpecialCharacterDataModule.php | 7 ++- .../ResourceLoaderUserCSSPrefsModule.php | 7 ++- .../ResourceLoaderUserDefaultsModule.php | 11 ++--- .../ResourceLoaderUserOptionsModule.php | 7 ++- .../ResourceLoaderWikiModule.php | 14 ++++++ tests/phpunit/ResourceLoaderTestCase.php | 4 ++ .../ResourceLoaderStartUpModuleTest.php | 48 +++++++++---------- 13 files changed, 111 insertions(+), 83 deletions(-) diff --git a/includes/resourceloader/ResourceLoaderEditToolbarModule.php b/includes/resourceloader/ResourceLoaderEditToolbarModule.php index d0273c2cfe..f3fae0e607 100644 --- a/includes/resourceloader/ResourceLoaderEditToolbarModule.php +++ b/includes/resourceloader/ResourceLoaderEditToolbarModule.php @@ -65,15 +65,10 @@ class ResourceLoaderEditToolbarModule extends ResourceLoaderFileModule { } /** - * @param ResourceLoaderContext $context - * @return array + * @return bool */ - public function getDefinitionSummary( ResourceLoaderContext $context ) { - $summary = parent::getDefinitionSummary( $context ); - $summary[] = array( - 'lessVars' => $this->getLessVars( $context ), - ); - return $summary; + public function enableModuleContentVersion() { + return true; } /** diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php index e6c9bd0439..b734defe3a 100644 --- a/includes/resourceloader/ResourceLoaderFileModule.php +++ b/includes/resourceloader/ResourceLoaderFileModule.php @@ -513,6 +513,18 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { return $this->raw; } + /** + * Disable module content versioning. + * + * This class uses getDefinitionSummary() instead, to avoid filesystem overhead + * involved with building the full module content inside a startup request. + * + * @return bool + */ + public function enableModuleContentVersion() { + return false; + } + /** * Helper method to gather file mtimes for getDefinitionSummary. * diff --git a/includes/resourceloader/ResourceLoaderJqueryMsgModule.php b/includes/resourceloader/ResourceLoaderJqueryMsgModule.php index b90578120f..d15928497a 100644 --- a/includes/resourceloader/ResourceLoaderJqueryMsgModule.php +++ b/includes/resourceloader/ResourceLoaderJqueryMsgModule.php @@ -49,14 +49,9 @@ class ResourceLoaderJqueryMsgModule extends ResourceLoaderFileModule { } /** - * @param ResourceLoaderContext $context - * @return array|null + * @return bool */ - public function getDefinitionSummary( ResourceLoaderContext $context ) { - $summary = parent::getDefinitionSummary( $context ); - $summary[] = array( - 'sanitizerData' => Sanitizer::getRecognizedTagData() - ); - return $summary; + public function enableModuleContentVersion() { + return true; } } diff --git a/includes/resourceloader/ResourceLoaderLanguageDataModule.php b/includes/resourceloader/ResourceLoaderLanguageDataModule.php index be15008ef9..27c74d7457 100644 --- a/includes/resourceloader/ResourceLoaderLanguageDataModule.php +++ b/includes/resourceloader/ResourceLoaderLanguageDataModule.php @@ -63,11 +63,10 @@ class ResourceLoaderLanguageDataModule extends ResourceLoaderModule { } /** - * @param ResourceLoaderContext $context - * @return string Hash + * @return bool */ - public function getModifiedHash( ResourceLoaderContext $context ) { - return md5( serialize( $this->getData( $context ) ) ); + public function enableModuleContentVersion() { + return true; } /** diff --git a/includes/resourceloader/ResourceLoaderLanguageNamesModule.php b/includes/resourceloader/ResourceLoaderLanguageNamesModule.php index 827a573feb..081c728cb4 100644 --- a/includes/resourceloader/ResourceLoaderLanguageNamesModule.php +++ b/includes/resourceloader/ResourceLoaderLanguageNamesModule.php @@ -32,7 +32,6 @@ class ResourceLoaderLanguageNamesModule extends ResourceLoaderModule { protected $targets = array( 'desktop', 'mobile' ); - /** * @param ResourceLoaderContext $context * @return array @@ -69,11 +68,10 @@ class ResourceLoaderLanguageNamesModule extends ResourceLoaderModule { } /** - * @param ResourceLoaderContext $context - * @return string Hash + * @return bool */ - public function getModifiedHash( ResourceLoaderContext $context ) { - return md5( serialize( $this->getData( $context ) ) ); + public function enableModuleContentVersion() { + return true; } } diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index d58c01d000..3322eff4e4 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -600,22 +600,28 @@ abstract class ResourceLoaderModule { $contextHash = $context->getHash(); if ( !array_key_exists( $contextHash, $this->versionHash ) ) { - $summary = $this->getDefinitionSummary( $context ); - if ( !isset( $summary['_cacheEpoch'] ) ) { - throw new Exception( 'getDefinitionSummary must call parent method' ); - } - $str = json_encode( $summary ); + if ( $this->enableModuleContentVersion() ) { + // Detect changes directly + $str = json_encode( $this->getModuleContent( $context ) ); + } else { + // Infer changes based on definition and other metrics + $summary = $this->getDefinitionSummary( $context ); + if ( !isset( $summary['_cacheEpoch'] ) ) { + throw new Exception( 'getDefinitionSummary must call parent method' ); + } + $str = json_encode( $summary ); - $mtime = $this->getModifiedTime( $context ); - if ( $mtime !== null ) { - // Support: MediaWiki 1.25 and earlier - $str .= strval( $mtime ); - } + $mtime = $this->getModifiedTime( $context ); + if ( $mtime !== null ) { + // Support: MediaWiki 1.25 and earlier + $str .= strval( $mtime ); + } - $mhash = $this->getModifiedHash( $context ); - if ( $mhash !== null ) { - // Support: MediaWiki 1.25 and earlier - $str .= strval( $mhash ); + $mhash = $this->getModifiedHash( $context ); + if ( $mhash !== null ) { + // Support: MediaWiki 1.25 and earlier + $str .= strval( $mhash ); + } } $this->versionHash[$contextHash] = ResourceLoader::makeHash( $str ); @@ -623,6 +629,19 @@ abstract class ResourceLoaderModule { return $this->versionHash[$contextHash]; } + /** + * Whether to generate version hash based on module content. + * + * If a module requires database or file system access to build the module + * content, consider disabling this in favour of manually tracking relevant + * aspects in getDefinitionSummary(). See getVersionHash() for how this is used. + * + * @return bool + */ + public function enableModuleContentVersion() { + return false; + } + /** * Get the definition summary for this module. * diff --git a/includes/resourceloader/ResourceLoaderSpecialCharacterDataModule.php b/includes/resourceloader/ResourceLoaderSpecialCharacterDataModule.php index 03f2124537..8170cb1c98 100644 --- a/includes/resourceloader/ResourceLoaderSpecialCharacterDataModule.php +++ b/includes/resourceloader/ResourceLoaderSpecialCharacterDataModule.php @@ -54,11 +54,10 @@ class ResourceLoaderSpecialCharacterDataModule extends ResourceLoaderModule { } /** - * @param ResourceLoaderContext $context - * @return string Hash + * @return bool */ - public function getModifiedHash( ResourceLoaderContext $context ) { - return md5( serialize( $this->getData() ) ); + public function enableModuleContentVersion() { + return true; } /** diff --git a/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php b/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php index d0f7d4476b..65d770e23f 100644 --- a/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php +++ b/includes/resourceloader/ResourceLoaderUserCSSPrefsModule.php @@ -30,11 +30,10 @@ class ResourceLoaderUserCSSPrefsModule extends ResourceLoaderModule { protected $origin = self::ORIGIN_CORE_INDIVIDUAL; /** - * @param ResourceLoaderContext $context - * @return array|int|mixed + * @return bool */ - public function getModifiedTime( ResourceLoaderContext $context ) { - return wfTimestamp( TS_UNIX, $context->getUserObj()->getTouched() ); + public function enableModuleContentVersion() { + return true; } /** diff --git a/includes/resourceloader/ResourceLoaderUserDefaultsModule.php b/includes/resourceloader/ResourceLoaderUserDefaultsModule.php index 2fd35adcd1..eba61edcfb 100644 --- a/includes/resourceloader/ResourceLoaderUserDefaultsModule.php +++ b/includes/resourceloader/ResourceLoaderUserDefaultsModule.php @@ -26,18 +26,13 @@ */ class ResourceLoaderUserDefaultsModule extends ResourceLoaderModule { - /* Protected Members */ - protected $targets = array( 'desktop', 'mobile' ); - /* Methods */ - /** - * @param ResourceLoaderContext $context - * @return string Hash + * @return bool */ - public function getModifiedHash( ResourceLoaderContext $context ) { - return md5( serialize( User::getDefaultOptions() ) ); + public function enableModuleContentVersion() { + return true; } /** diff --git a/includes/resourceloader/ResourceLoaderUserOptionsModule.php b/includes/resourceloader/ResourceLoaderUserOptionsModule.php index aba0fa6bc6..0847109cf5 100644 --- a/includes/resourceloader/ResourceLoaderUserOptionsModule.php +++ b/includes/resourceloader/ResourceLoaderUserOptionsModule.php @@ -40,11 +40,10 @@ class ResourceLoaderUserOptionsModule extends ResourceLoaderModule { } /** - * @param ResourceLoaderContext $context - * @return int + * @return bool */ - public function getModifiedTime( ResourceLoaderContext $context ) { - return wfTimestamp( TS_UNIX, $context->getUserObj()->getTouched() ); + public function enableModuleContentVersion() { + return true; } /** diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php index 264af5bafe..1561daecd6 100644 --- a/includes/resourceloader/ResourceLoaderWikiModule.php +++ b/includes/resourceloader/ResourceLoaderWikiModule.php @@ -224,6 +224,20 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule { return $styles; } + /** + * Disable module content versioning. + * + * This class does not support generating content outside of a module + * request due to foreign database support. + * + * See getDefinitionSummary() for meta-data versioning. + * + * @return bool + */ + public function enableModuleContentVersion() { + return false; + } + /** * @param ResourceLoaderContext $context * @return array diff --git a/tests/phpunit/ResourceLoaderTestCase.php b/tests/phpunit/ResourceLoaderTestCase.php index 3165bb88ad..325b20eec1 100644 --- a/tests/phpunit/ResourceLoaderTestCase.php +++ b/tests/phpunit/ResourceLoaderTestCase.php @@ -102,6 +102,10 @@ class ResourceLoaderTestModule extends ResourceLoaderModule { public function isRaw() { return $this->isRaw; } + + public function enableModuleContentVersion() { + return true; + } } class ResourceLoaderFileModuleTestModule extends ResourceLoaderFileModule { diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php index 72a2d6af04..490f5c650e 100644 --- a/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php +++ b/tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php @@ -23,7 +23,7 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "XyCC+PSK" + "wvTifjse" ] ] );', ) ), @@ -40,17 +40,17 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "XyCC+PSK" + "wvTifjse" ], [ "test.group.foo", - "XyCC+PSK", + "wvTifjse", [], "x-foo" ], [ "test.group.bar", - "XyCC+PSK", + "wvTifjse", [], "x-bar" ] @@ -68,7 +68,7 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "XyCC+PSK" + "wvTifjse" ] ] );' ) ), @@ -90,7 +90,7 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "XyCC+PSK", + "wvTifjse", [], null, "example" @@ -126,11 +126,11 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.x.core", - "XyCC+PSK" + "wvTifjse" ], [ "test.x.polyfill", - "XyCC+PSK", + "wvTifjse", [], null, null, @@ -138,7 +138,7 @@ mw.loader.addSource( { ], [ "test.y.polyfill", - "XyCC+PSK", + "wvTifjse", [], null, null, @@ -146,7 +146,7 @@ mw.loader.addSource( { ], [ "test.z.foo", - "XyCC+PSK", + "wvTifjse", [ 0, 1, @@ -222,36 +222,36 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "XyCC+PSK" + "wvTifjse" ], [ "test.x.core", - "XyCC+PSK" + "wvTifjse" ], [ "test.x.util", - "XyCC+PSK", + "wvTifjse", [ 1 ] ], [ "test.x.foo", - "XyCC+PSK", + "wvTifjse", [ 1 ] ], [ "test.x.bar", - "XyCC+PSK", + "wvTifjse", [ 2 ] ], [ "test.x.quux", - "XyCC+PSK", + "wvTifjse", [ 3, 4, @@ -260,25 +260,25 @@ mw.loader.addSource( { ], [ "test.group.foo.1", - "XyCC+PSK", + "wvTifjse", [], "x-foo" ], [ "test.group.foo.2", - "XyCC+PSK", + "wvTifjse", [], "x-foo" ], [ "test.group.bar.1", - "XyCC+PSK", + "wvTifjse", [], "x-bar" ], [ "test.group.bar.2", - "XyCC+PSK", + "wvTifjse", [], "x-bar", "example" @@ -344,8 +344,8 @@ mw.loader.addSource( { $this->assertEquals( 'mw.loader.addSource({"local":"/w/load.php"});' . 'mw.loader.register([' -. '["test.blank","XyCC+PSK"],' -. '["test.min","XyCC+PSK",[0],null,null,' +. '["test.blank","wvTifjse"],' +. '["test.min","wvTifjse",[0],null,null,' . '"return!!(window.JSON\u0026\u0026JSON.parse\u0026\u0026JSON.stringify);"' . ']]);', $module->getModuleRegistrations( $context ), @@ -367,11 +367,11 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "XyCC+PSK" + "wvTifjse" ], [ "test.min", - "XyCC+PSK", + "wvTifjse", [ 0 ], -- 2.20.1