From 6a1ec17e7990808952dd1388015563b605aaaeea Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 5 Dec 2014 20:28:54 +0000 Subject: [PATCH] resourceloader: Make timestamp handling more consistent * Use time() instead of: - wfTimestamp() - wfTimestamp( TS_UNIX ) - wfTimestamp( TS_UNIX, 0 ) - wfTimestamp( TS_UNIX, time() ) - intval( wfTimestamp( TS_UNIX, time() ) ) * Consistently use 1 as default instead of 0. Previously the unwritten convention was that anything "final" max()'ed with 1, and any internal method would use 0, but this wasn't applied consistently made it fragile. There doesn't seem to be any value in returning 0 only to have it maxed up to 1 (because if the 0 would ever make it out alive, we'd be in trouble). * wfTimestamp returns a string for TS_UNIX. In PHP this doesn't matter much. In fact, max() takes number-like integers so transparently, it even preserves it: > max( 1, 3, '2' ); < 3 > max( 1, '3', 2 ); < "3" Just cast it in one place at the very end (StartupModule) instead of doing intval( wfTimestamp() ). * Fix weird documentation claiming getModifiedTime can return an array, or mixed. * Remove 'version > 1 ? version : 1' logic in ResourceLoader::makeLoaderRegisterScript. The client doesn't have "0 means now" behaviour so this isn't needed. And the method was only doing it for variadic argument calls. Removal of quotes around timestamps reduced the size of the startup module from 26.8KB to 25.9KB before gzip. After gzip the size was and still is 5.7KB, though. (From 5456 bytes to 5415 bytes.) Change-Id: If92ca3e7511e78fa779f2f2701e2ab24db78c8a8 --- includes/resourceloader/ResourceLoader.php | 4 +- .../resourceloader/ResourceLoaderModule.php | 29 +++++------ .../ResourceLoaderStartUpModule.php | 18 ++++--- .../ResourceLoaderUserDefaultsModule.php | 2 +- .../ResourceLoaderUserOptionsModule.php | 2 +- .../ResourceLoaderWikiModule.php | 8 ++-- resources/src/mediawiki/mediawiki.js | 21 ++++---- .../ResourceLoaderStartupModuleTest.php | 48 +++++++++---------- 8 files changed, 66 insertions(+), 66 deletions(-) diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index 2c54f69fa6..93cc24548e 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -140,7 +140,7 @@ class ResourceLoader { foreach ( array_keys( $modulesWithoutMessages ) as $name ) { $module = $this->getModule( $name ); if ( $module ) { - $module->setMsgBlobMtime( $lang, 0 ); + $module->setMsgBlobMtime( $lang, 1 ); } } } @@ -1225,7 +1225,7 @@ class ResourceLoader { ResourceLoader::inDebugMode() ); } else { - $version = (int)$version > 1 ? (int)$version : 1; + $version = (int) $version; return Xml::encodeJsCall( 'mw.loader.register', array( $name, $version, $dependencies, $group, $source, $skip ), diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 4c49fae5cb..3f95ce6eef 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -388,12 +388,12 @@ abstract class ResourceLoaderModule { * Get the last modification timestamp of the message blob for this * module in a given language. * @param string $lang Language code - * @return int UNIX timestamp, or 0 if the module doesn't have messages + * @return int UNIX timestamp */ public function getMsgBlobMtime( $lang ) { if ( !isset( $this->msgBlobMtime[$lang] ) ) { if ( !count( $this->getMessages() ) ) { - return 0; + return 1; } $dbr = wfGetDB( DB_SLAVE ); @@ -416,7 +416,7 @@ abstract class ResourceLoaderModule { * Set a preloaded message blob last modification timestamp. Used so we * can load this information for all modules at once. * @param string $lang Language code - * @param int $mtime UNIX timestamp or 0 if there is no such blob + * @param int $mtime UNIX timestamp */ public function setMsgBlobMtime( $lang, $mtime ) { $this->msgBlobMtime[$lang] = $mtime; @@ -443,7 +443,6 @@ abstract class ResourceLoaderModule { * @return int UNIX timestamp */ public function getModifiedTime( ResourceLoaderContext $context ) { - // 0 would mean now return 1; } @@ -451,13 +450,12 @@ abstract class ResourceLoaderModule { * Helper method for calculating when the module's hash (if it has one) changed. * * @param ResourceLoaderContext $context - * @return int UNIX timestamp or 0 if no hash was provided - * by getModifiedHash() + * @return int UNIX timestamp */ public function getHashMtime( ResourceLoaderContext $context ) { $hash = $this->getModifiedHash( $context ); if ( !is_string( $hash ) ) { - return 0; + return 1; } $cache = wfGetCache( CACHE_ANYTHING ); @@ -469,7 +467,7 @@ abstract class ResourceLoaderModule { return $data['timestamp']; } - $timestamp = wfTimestamp(); + $timestamp = time(); $cache->set( $key, array( 'hash' => $hash, 'timestamp' => $timestamp, @@ -497,15 +495,14 @@ abstract class ResourceLoaderModule { * @since 1.23 * * @param ResourceLoaderContext $context - * @return int UNIX timestamp or 0 if no definition summary was provided - * by getDefinitionSummary() + * @return int UNIX timestamp */ public function getDefinitionMtime( ResourceLoaderContext $context ) { wfProfileIn( __METHOD__ ); $summary = $this->getDefinitionSummary( $context ); if ( $summary === null ) { wfProfileOut( __METHOD__ ); - return 0; + return 1; } $hash = md5( json_encode( $summary ) ); @@ -640,16 +637,12 @@ abstract class ResourceLoaderModule { * Safe version of filemtime(), which doesn't throw a PHP warning if the file doesn't exist * but returns 1 instead. * @param string $filename File name - * @return int UNIX timestamp, or 1 if the file doesn't exist + * @return int UNIX timestamp */ protected static function safeFilemtime( $filename ) { - if ( file_exists( $filename ) ) { - return filemtime( $filename ); - } else { - // We only ever map this function on an array if we're gonna call max() after, - // so return our standard minimum timestamps here. This is 1, not 0, because - // wfTimestamp(0) == NOW + if ( !file_exists( $filename ) ) { return 1; } + return filemtime( $filename ); } } diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php index ee662888f9..5370424077 100644 --- a/includes/resourceloader/ResourceLoaderStartUpModule.php +++ b/includes/resourceloader/ResourceLoaderStartUpModule.php @@ -211,12 +211,10 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { continue; } - // getModifiedTime() is supposed to return a UNIX timestamp, but it doesn't always - // seem to do that, and custom implementations might forget. Coerce it to TS_UNIX + // Coerce module timestamp to UNIX timestamp. + // getModifiedTime() is supposed to return a UNIX timestamp, but custom implementations + // might forget. TODO: Maybe emit warning? $moduleMtime = wfTimestamp( TS_UNIX, $module->getModifiedTime( $context ) ); - $mtime = max( $moduleMtime, wfTimestamp( TS_UNIX, $this->getConfig()->get( 'CacheEpoch' ) ) ); - - // FIXME: Convert to numbers, wfTimestamp always gives us stings, even for TS_UNIX $skipFunction = $module->getSkipFunction(); if ( $skipFunction !== null && !ResourceLoader::inDebugMode() ) { @@ -229,8 +227,14 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { ); } + $mtime = max( + $moduleMtime, + wfTimestamp( TS_UNIX, $this->getConfig()->get( 'CacheEpoch' ) ) + ); + $registryData[$name] = array( - 'version' => $mtime, + // Convert to numbers as wfTimestamp always returns a string, even for TS_UNIX + 'version' => (int) $mtime, 'dependencies' => $module->getDependencies(), 'group' => $module->getGroup(), 'source' => $module->getSource(), @@ -352,7 +356,7 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { // Get the latest version $loader = $context->getResourceLoader(); - $version = 0; + $version = 1; foreach ( $moduleNames as $moduleName ) { $version = max( $version, $loader->getModule( $moduleName )->getModifiedTime( $context ) diff --git a/includes/resourceloader/ResourceLoaderUserDefaultsModule.php b/includes/resourceloader/ResourceLoaderUserDefaultsModule.php index 1249cf84d6..d78fa9da7d 100644 --- a/includes/resourceloader/ResourceLoaderUserDefaultsModule.php +++ b/includes/resourceloader/ResourceLoaderUserDefaultsModule.php @@ -42,7 +42,7 @@ class ResourceLoaderUserDefaultsModule extends ResourceLoaderModule { /** * @param ResourceLoaderContext $context - * @return array|int|mixed + * @return int */ public function getModifiedTime( ResourceLoaderContext $context ) { return $this->getHashMtime( $context ); diff --git a/includes/resourceloader/ResourceLoaderUserOptionsModule.php b/includes/resourceloader/ResourceLoaderUserOptionsModule.php index a1847fb8c4..84c1906d2d 100644 --- a/includes/resourceloader/ResourceLoaderUserOptionsModule.php +++ b/includes/resourceloader/ResourceLoaderUserOptionsModule.php @@ -46,7 +46,7 @@ class ResourceLoaderUserOptionsModule extends ResourceLoaderModule { /** * @param ResourceLoaderContext $context - * @return array|int|mixed + * @return int */ public function getModifiedTime( ResourceLoaderContext $context ) { $hash = $context->getHash(); diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php index e195cf2bc9..1a1a6d0008 100644 --- a/includes/resourceloader/ResourceLoaderWikiModule.php +++ b/includes/resourceloader/ResourceLoaderWikiModule.php @@ -164,10 +164,10 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule { /** * @param ResourceLoaderContext $context - * @return int|mixed + * @return int */ public function getModifiedTime( ResourceLoaderContext $context ) { - $modifiedTime = 1; // wfTimestamp() interprets 0 as "now" + $modifiedTime = 1; $titleInfo = $this->getTitleInfo( $context ); if ( count( $titleInfo ) ) { $mtimes = array_map( function ( $value ) { @@ -226,8 +226,8 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule { * Get the modification times of all titles that would be loaded for * a given context. * @param ResourceLoaderContext $context Context object - * @return array keyed by page dbkey, with value is an array with 'length' and 'timestamp' - * keys, where the timestamp is a unix one + * @return array Keyed by page dbkey. Value is an array with 'length' and 'timestamp' + * keys, where the timestamp is a UNIX timestamp */ protected function getTitleInfo( ResourceLoaderContext $context ) { $dbr = $this->getDB(); diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js index 9e118421fe..06d0f47796 100644 --- a/resources/src/mediawiki/mediawiki.js +++ b/resources/src/mediawiki/mediawiki.js @@ -807,7 +807,8 @@ } /** - * Generates an ISO8601 "basic" string from a UNIX timestamp + * Convert UNIX timestamp to ISO8601 format + * @param {number} timestamp UNIX timestamp * @private */ function formatVersionNumber( timestamp ) { @@ -1877,27 +1878,29 @@ /** * Get the version of a module. * - * @param {string} module Name of module to get version for + * @param {string} module Name of module * @return {string|null} The version, or null if the module (or its version) is not * in the registry. */ getVersion: function ( module ) { - if ( registry[module] !== undefined && registry[module].version !== undefined ) { - return formatVersionNumber( registry[module].version ); + if ( !registry[module] || registry[module].version === undefined ) { + return null; } - return null; + return formatVersionNumber( registry[module].version ); }, /** * Get the state of a module. * - * @param {string} module Name of module to get state for + * @param {string} module Name of module + * @return {string|null} The state, or null if the module (or its version) is not + * in the registry. */ getState: function ( module ) { - if ( registry[module] !== undefined && registry[module].state !== undefined ) { - return registry[module].state; + if ( !registry[module] || registry[module].state === undefined ) { + return null; } - return null; + return registry[module].state; }, /** diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderStartupModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderStartupModuleTest.php index 1c25211b38..3af1d1f208 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", - "1388534400" + 1388534400 ] ] );', ) ), @@ -40,17 +40,17 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "1388534400" + 1388534400 ], [ "test.group.foo", - "1388534400", + 1388534400, [], "x-foo" ], [ "test.group.bar", - "1388534400", + 1388534400, [], "x-bar" ] @@ -68,7 +68,7 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "1388534400" + 1388534400 ] ] );' ) ), @@ -90,7 +90,7 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "1388534400", + 1388534400, [], null, "example" @@ -126,11 +126,11 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.x.core", - "1388534400" + 1388534400 ], [ "test.x.polyfill", - "1388534400", + 1388534400, [], null, "local", @@ -138,7 +138,7 @@ mw.loader.addSource( { ], [ "test.y.polyfill", - "1388534400", + 1388534400, [], null, "local", @@ -146,7 +146,7 @@ mw.loader.addSource( { ], [ "test.z.foo", - "1388534400", + 1388534400, [ "test.x.core", "test.x.polyfil", @@ -222,36 +222,36 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "1388534400" + 1388534400 ], [ "test.x.core", - "1388534400" + 1388534400 ], [ "test.x.util", - "1388534400", + 1388534400, [ "test.x.core" ] ], [ "test.x.foo", - "1388534400", + 1388534400, [ "test.x.core" ] ], [ "test.x.bar", - "1388534400", + 1388534400, [ "test.x.util" ] ], [ "test.x.quux", - "1388534400", + 1388534400, [ "test.x.foo", "test.x.bar", @@ -260,25 +260,25 @@ mw.loader.addSource( { ], [ "test.group.foo.1", - "1388534400", + 1388534400, [], "x-foo" ], [ "test.group.foo.2", - "1388534400", + 1388534400, [], "x-foo" ], [ "test.group.bar.1", - "1388534400", + 1388534400, [], "x-bar" ], [ "test.group.bar.2", - "1388534400", + 1388534400, [], "x-bar", "example" @@ -344,8 +344,8 @@ mw.loader.addSource( { $this->assertEquals( 'mw.loader.addSource({"local":"/w/load.php"});' . 'mw.loader.register([' -. '["test.blank","1388534400"],' -. '["test.min","1388534400",["test.blank"],null,"local",' +. '["test.blank",1388534400],' +. '["test.min",1388534400,["test.blank"],null,"local",' . '"return!!(window.JSON\u0026\u0026JSON.parse\u0026\u0026JSON.stringify);"' . ']]);', $module->getModuleRegistrations( $context ), @@ -367,11 +367,11 @@ mw.loader.addSource( { } );mw.loader.register( [ [ "test.blank", - "1388534400" + 1388534400 ], [ "test.min", - "1388534400", + 1388534400, [ "test.blank" ], -- 2.20.1