resourceloader: Clean up and better document module list (un)packing

* Move buildModulesString() call from doRequest() to batchRequest()

  This keeps all module string "packing" logic located to the same function,
  which is batchRequest(). It also means that the moduleMap object will not
  leave the function, which helps in maintenance given it's very internal.

* Add comments to all the methods referring to each other.

* Explain why buildModulesString() is only a partial port, and the rest is
  inlined in batchRequest().

* Minor changes to the JS and PHP implementation to better match each other.
  - '$groups' -> '$moduleMap'.
  - Remove redundant '$str'.

Bug: T188076
Change-Id: I7b0790606c456e492ab682faeb80f7e7fce7d9f8

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index f9b03c7..5ddb99b 100644 (file)
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -1532,27 +1532,31 @@ MESSAGE;
        /**
         * Convert an array of module names to a packed query string.
         *
-        * For example, [ 'foo.bar', 'foo.baz', 'bar.baz', 'bar.quux' ]
-        * becomes 'foo.bar,baz|bar.baz,quux'
+        * For example, `[ 'foo.bar', 'foo.baz', 'bar.baz', 'bar.quux' ]`
+        * becomes `'foo.bar,baz|bar.baz,quux'`.
+        *
+        * This process is reversed by ResourceLoaderContext::expandModuleNames().
+        * See also mw.loader#buildModulesString() which is a port of this, used
+        * on the client-side.
+        *
         * @param array $modules List of module names (strings)
         * @return string Packed query string
         */
        public static function makePackedModulesString( $modules ) {
-               $groups = []; // [ prefix => [ suffixes ] ]
+               $moduleMap = []; // [ prefix => [ suffixes ] ]
                foreach ( $modules as $module ) {
                        $pos = strrpos( $module, '.' );
                        $prefix = $pos === false ? '' : substr( $module, 0, $pos );
                        $suffix = $pos === false ? $module : substr( $module, $pos + 1 );
-                       $groups[$prefix][] = $suffix;
+                       $moduleMap[$prefix][] = $suffix;
                }
 
                $arr = [];
-               foreach ( $groups as $prefix => $suffixes ) {
+               foreach ( $moduleMap as $prefix => $suffixes ) {
                        $p = $prefix === '' ? '' : $prefix . '.';
                        $arr[] = $p . implode( ',', $suffixes );
                }
-               $str = implode( '|', $arr );
-               return $str;
+               return implode( '|', $arr );
        }
 
        /**

diff --git a/includes/resourceloader/ResourceLoaderContext.php b/includes/resourceloader/ResourceLoaderContext.php
index 7478266..370046a 100644 (file)
--- a/includes/resourceloader/ResourceLoaderContext.php
+++ b/includes/resourceloader/ResourceLoaderContext.php
@@ -98,9 +98,12 @@ class ResourceLoaderContext implements MessageLocalizer {
        }
 
        /**
-        * Expand a string of the form jquery.foo,bar|jquery.ui.baz,quux to
-        * an array of module names like [ 'jquery.foo', 'jquery.bar',
-        * 'jquery.ui.baz', 'jquery.ui.quux' ]
+        * Expand a string of the form `jquery.foo,bar|jquery.ui.baz,quux` to
+        * an array of module names like `[ 'jquery.foo', 'jquery.bar',
+        * 'jquery.ui.baz', 'jquery.ui.quux' ]`.
+        *
+        * This process is reversed by ResourceLoader::makePackedModulesString().
+        *
         * @param string $modules Packed module name list
         * @return array Array of module names
         */

diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js
index a2e071e..aa93ca2 100644 (file)
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1511,8 +1511,15 @@
                        }
 
                        /**
-                        * Converts a module map of the form { foo: [ 'bar', 'baz' ], bar: [ 'baz, 'quux' ] }
-                        * to a query string of the form foo.bar,baz|bar.baz,quux
+                        * Converts a module map of the form `{ foo: [ 'bar', 'baz' ], bar: [ 'baz, 'quux' ] }`
+                        * to a query string of the form `foo.bar,baz|bar.baz,quux`.
+                        *
+                        * See `ResourceLoader::makePackedModulesString()` in PHP, of which this is a port.
+                        * On the server, unpacking is done by `ResourceLoaderContext::expandModuleNames()`.
+                        *
+                        * Note: This is only half of the logic, the other half has to be in #batchRequest(),
+                        * because its implementation needs to keep track of potential string size in order
+                        * to decide when to split the requests due to url size.
                         *
                         * @private
                         * @param {Object} moduleMap Module map
@@ -1533,14 +1540,14 @@
                         * Make a network request to load modules from the server.
                         *
                         * @private
-                        * @param {Object} moduleMap Module map, see #buildModulesString
+                        * @param {string} moduleStr Module list for load.php `module` query parameter
                         * @param {Object} currReqBase Object with other parameters (other than 'modules') to use in the request
                         * @param {string} sourceLoadScript URL of load.php
                         */
-                       function doRequest( moduleMap, currReqBase, sourceLoadScript ) {
+                       function doRequest( moduleStr, currReqBase, sourceLoadScript ) {
                                // Optimisation: Inherit (Object.create), not copy ($.extend)
                                var query = Object.create( currReqBase );
-                               query.modules = buildModulesString( moduleMap );
+                               query.modules = moduleStr;
                                query = sortQuery( query );
                                addScript( sourceLoadScript + '?' + $.param( query ) );
                        }
@@ -1660,7 +1667,7 @@
                                                        // but don't create empty requests
                                                        if ( maxQueryLength > 0 && !$.isEmptyObject( moduleMap ) && l + bytesAdded > maxQueryLength ) {
                                                                // This url would become too long, create a new one, and start the old one
-                                                               doRequest( moduleMap, currReqBase, sourceLoadScript );
+                                                               doRequest( buildModulesString( moduleMap ), currReqBase, sourceLoadScript );
                                                                moduleMap = {};
                                                                l = currReqBaseLength + 9;
                                                                mw.track( 'resourceloader.splitRequest', { maxQueryLength: maxQueryLength } );
@@ -1673,7 +1680,7 @@
                                                }
                                                // If there's anything left in moduleMap, request that too
                                                if ( !$.isEmptyObject( moduleMap ) ) {
-                                                       doRequest( moduleMap, currReqBase, sourceLoadScript );
+                                                       doRequest( buildModulesString( moduleMap ), currReqBase, sourceLoadScript );
                                                }
                                        }
                                }