resourceloader: Make timestamp handling more consistent
authorTimo Tijhof <krinklemail@gmail.com>
Fri, 5 Dec 2014 20:28:54 +0000 (20:28 +0000)
committerTimo Tijhof <krinklemail@gmail.com>
Fri, 5 Dec 2014 21:29:57 +0000 (21:29 +0000)
* 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
includes/resourceloader/ResourceLoaderModule.php
includes/resourceloader/ResourceLoaderStartUpModule.php
includes/resourceloader/ResourceLoaderUserDefaultsModule.php
includes/resourceloader/ResourceLoaderUserOptionsModule.php
includes/resourceloader/ResourceLoaderWikiModule.php
resources/src/mediawiki/mediawiki.js
tests/phpunit/includes/resourceloader/ResourceLoaderStartupModuleTest.php

index 2c54f69..93cc245 100644 (file)
@@ -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 ),
index 4c49fae..3f95ce6 100644 (file)
@@ -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 );
        }
 }
index ee66288..5370424 100644 (file)
@@ -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 )
index 1249cf8..d78fa9d 100644 (file)
@@ -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 );
index a1847fb..84c1906 100644 (file)
@@ -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();
index e195cf2..1a1a6d0 100644 (file)
@@ -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();
index 9e11842..06d0f47 100644 (file)
                        }
 
                        /**
-                        * Generates an ISO8601 "basic" string from a UNIX timestamp
+                        * Convert UNIX timestamp to ISO8601 format
+                        * @param {number} timestamp UNIX timestamp
                         * @private
                         */
                        function formatVersionNumber( timestamp ) {
                                /**
                                 * 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;
                                },
 
                                /**
index 1c25211..3af1d1f 100644 (file)
@@ -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"
         ],