r87497 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r87496‎ | r87497 | r87498 >
Date:13:46, 5 May 2011
Author:catrope
Status:resolved (Comments)
Tags:
Comment:
Per bug 28738 comment 4, pack ResourceLoader URLs by encoding foo.bar|foo.baz|bar.baz|bar.quux as foo.bar,baz|bar.baz,quux

* Expand these URLs in ResourceLoaderContext
* Build and emit these URLs in OutputPage::makeResourceLoaderLink() and in mw.loader
* Throw an exception in ResourceLoader::register() for module names that contain pipe characters or commas. Commas need to be forbidden for this packing feature to work. Pipes were already forbidden but weren't checked for
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoader.php (modified) (history)
  • /trunk/phase3/includes/resourceloader/ResourceLoaderContext.php (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
——@@ -2396,7 +2396,6 @@
23972397 $wgResourceLoaderInlinePrivateModules;
23982398 // Lazy-load ResourceLoader
23992399 // TODO: Should this be a static function of ResourceLoader instead?
2400 - // TODO: Divide off modules starting with "user", and add the user parameter to them
24012400 $baseQuery = array(
24022401 'lang' => $this->getContext()->getLang()->getCode(),
24032402 'debug' => ResourceLoader::inDebugMode() ? 'true' : 'false',
——@@ -2475,7 +2474,7 @@
24762475 continue;
24772476 }
24782477
2479 - $query['modules'] = implode( '|', array_keys( $modules ) );
 2478+ $query['modules'] = ResourceLoader::makePackedModulesString( array_keys( $modules ) );
24802479
24812480 // Support inlining of private modules if configured as such
24822481 if ( $group === 'private' && $wgResourceLoaderInlinePrivateModules ) {
Index: trunk/phase3/includes/resourceloader/ResourceLoader.php
——@@ -199,6 +199,7 @@
200200 * this may also be a ResourceLoaderModule object. Optional when using
201201 * multiple-registration calling style.
202202 * @throws MWException: If a duplicate module registration is attempted
 203+ * @throws MWException: If a module name contains illegal characters (pipes or commas)
203204 * @throws MWException: If something other than a ResourceLoaderModule is being registered
204205 * @return Boolean: False if there were any errors, in which case one or more modules were not
205206 * registered
——@@ -223,6 +224,11 @@
224225 'Another module has already been registered as ' . $name
225226 );
226227 }
 228+
 229+ // Check $name for illegal characters
 230+ if ( preg_match( '/[|,]/', $name ) ) {
 231+ throw new MWException( "ResourceLoader module name '$name' is invalid. Names may not contain pipes (|) or commas (,)" );
 232+ }
227233
228234 // Attach module
229235 if ( is_object( $info ) ) {
——@@ -699,6 +705,31 @@
700706 }
701707
702708 /**
 709+ * Convert an array of module names to a packed query string.
 710+ *
 711+ * For example, array( 'foo.bar', 'foo.baz', 'bar.baz', 'bar.quux' )
 712+ * becomes 'foo.bar,baz|bar.baz,quux'
 713+ * @param $modules array of module names (strings)
 714+ * @return string Packed query string
 715+ */
 716+ public static function makePackedModulesString( $modules ) {
 717+ $groups = array(); // array( prefix => array( suffixes ) )
 718+ foreach ( $modules as $module ) {
 719+ $pos = strrpos( $module, '.' );
 720+ $prefix = $pos === false ? '' : substr( $module, 0, $pos );
 721+ $suffix = $pos === false ? $module : substr( $module, $pos + 1 );
 722+ $groups[$prefix][] = $suffix;
 723+ }
 724+
 725+ $arr = array();
 726+ foreach ( $groups as $prefix => $suffixes ) {
 727+ $p = $prefix === '' ? '' : $prefix . '.';
 728+ $arr[] = $p . implode( ',', $suffixes );
 729+ }
 730+ return implode( '|', $arr );
 731+ }
 732+
 733+ /**
703734 * Determine whether debug mode was requested
704735 * Order of priority is 1) request param, 2) cookie, 3) $wg setting
705736 * @return bool
Index: trunk/phase3/includes/resourceloader/ResourceLoaderContext.php
——@@ -51,7 +51,7 @@
5252 // Interpret request
5353 // List of modules
5454 $modules = $request->getVal( 'modules' );
55 - $this->modules = $modules ? explode( '|', $modules ) : array();
 55+ $this->modules = $modules ? self::expandModuleNames( $modules ) : array();
5656 // Various parameters
5757 $this->skin = $request->getVal( 'skin' );
5858 $this->user = $request->getVal( 'user' );
——@@ -63,6 +63,34 @@
6464 $this->skin = $wgDefaultSkin;
6565 }
6666 }
 67+
 68+ /**
 69+ * Expand a string of the form jquery.foo,bar|jquery.ui.baz,quux to
 70+ * an array of module names like array( 'jquery.foo', 'jquery.bar',
 71+ * 'jquery.ui.baz', 'jquery.ui.quux' )
 72+ * @param $modules String Packed module name list
 73+ * @return array of module names
 74+ */
 75+ public static function expandModuleNames( $modules ) {
 76+ $retval = array();
 77+ $exploded = explode( '|', $modules );
 78+ foreach ( $exploded as $group ) {
 79+ if ( strpos( $group, ',' ) === false ) {
 80+ // This is not a set of modules in foo.bar,baz notation
 81+ // but a single module
 82+ $retval[] = $group;
 83+ } else {
 84+ // This is a set of modules in foo.bar,baz notation
 85+ $pos = strrpos( $group, '.' );
 86+ $prefix = substr( $group, 0, $pos ); // 'foo'
 87+ $suffixes = explode( ',', substr( $group, $pos + 1 ) ); // array( 'bar', 'baz' )
 88+ foreach ( $suffixes as $suffix ) {
 89+ $retval[] = "$prefix.$suffix";
 90+ }
 91+ }
 92+ }
 93+ return $retval;
 94+ }
6795
6896 public function getResourceLoader() {
6997 return $this->resourceLoader;
Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -863,6 +863,20 @@
864864 }
865865 return sorted;
866866 }
 867+
 868+ /**
 869+ * Converts a module map of the form { foo: [ 'bar', 'baz' ], bar: [ 'baz, 'quux' ] }
 870+ * to a query string of the form foo.bar,baz|bar.baz,quux
 871+ */
 872+ function buildModulesString( moduleMap ) {
 873+ var arr = [];
 874+ for ( var prefix in moduleMap ) {
 875+ var p = prefix === '' ? '' : prefix + '.';
 876+ arr.push( p + moduleMap[prefix].join( ',' ) );
 877+ }
 878+ return arr.join( '|' );
 879+ }
 880+
867881
868882 /* Public Methods */
869883
——@@ -920,32 +934,38 @@
921935 var reqBaseLength = $.param( reqBase ).length;
922936 var reqs = [];
923937 var limit = mw.config.get( 'wgResourceLoaderMaxQueryLength', -1 );
924 - if ( limit > 0 ) {
925 - // We may need to split up the request to honor the query string length limit
926 - // So build it piece by piece
927 - var l = reqBaseLength + 9; // '&modules='.length == 9
928 - var r = 0;
929 - reqs[0] = [];
930 - for ( var i = 0; i < groups[group].length; i++ ) {
931 - // If the request would become too long, create a new one,
932 - // but don't create empty requests
933 - // '%7C'.length == 3
934 - if ( reqs[r].length > 0 && l + 3 + groups[group][i].length > limit ) {
935 - // This request would become too long, create a new one
936 - r++;
937 - reqs[r] = [];
938 - l = reqBaseLength + 9;
939 - }
940 - reqs[r][reqs[r].length] = groups[group][i];
941 - l += groups[group][i].length + 3;
 938+ // We may need to split up the request to honor the query string length limit
 939+ // So build it piece by piece
 940+ var l = reqBaseLength + 9; // '&modules='.length == 9
 941+ var r = 0;
 942+ reqs[0] = {}; // { prefix: [ suffixes ] }
 943+ for ( var i = 0; i < groups[group].length; i++ ) {
 944+ // Determine how many bytes this module would add to the query string
 945+ var lastDotIndex = groups[group][i].lastIndexOf( '.' );
 946+ // Note that these substr() calls work even if lastDotIndex == -1
 947+ var prefix = groups[group][i].substr( 0, lastDotIndex );
 948+ var suffix = groups[group][i].substr( lastDotIndex + 1 );
 949+ var bytesAdded = prefix in reqs[r] ?
 950+ suffix.length + 3 : // '%2C'.length == 3
 951+ groups[group][i].length + 3; // '%7C'.length == 3
 952+
 953+ // If the request would become too long, create a new one,
 954+ // but don't create empty requests
 955+ if ( limit > 0 && reqs[r] != {} && l + bytesAdded > limit ) {
 956+ // This request would become too long, create a new one
 957+ r++;
 958+ reqs[r] = {};
 959+ l = reqBaseLength + 9;
942960 }
943 - } else {
944 - // No splitting needed
945 - reqs = [ groups[group] ];
 961+ if ( !( prefix in reqs[r] ) ) {
 962+ reqs[r][prefix] = [];
 963+ }
 964+ reqs[r][prefix].push( suffix );
 965+ l += bytesAdded;
946966 }
947967 for ( var r = 0; r < reqs.length; r++ ) {
948968 requests[requests.length] = $.extend(
949 - { 'modules': reqs[r].join( '|' ) }, reqBase
 969+ { 'modules': buildModulesString( reqs[r] ) }, reqBase
950970 );
951971 }
952972 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r88076Fix bug in r87497, reported in CR: &modules=foo,bar,baz would look for '.foo'...catrope13:34, 14 May 2011
r89666Added phpunit test cases for r87497, r88076: ResourceLoader module name prefi...brion17:56, 7 June 2011
r896761.17: MFT r82247, r87203, r87265, r87494, r87497, r87711, r87840, r88076, r89615catrope19:09, 7 June 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r87203(bug 28738) Implement request splitting in mw.loader so ResourceLoader reques...catrope18:41, 1 May 2011
r87494For bug 28738, have the installer check for the Suhosin GET variable length l...catrope11:52, 5 May 2011

Comments

#Comment by Mdale (talk | contribs)   21:16, 9 May 2011

I think combination of buildModulesString and expandModuleNames is broken in the case of un-prefixed modules names. For example if you have a module name "foo" not "mw.foo". in requesting foo|bar|cat, buildModulesString will give you: foo,bar|cat And expandModuleNames will expand into 

Array
(
    [0] => .oo
    [1] => .bar
    [2] => cat
)

Giving you {".oo":"missing" , ".bar":"missing" } ... This was a little tricky to track down, since it missing module names don't throw an exception and fails silently in a loader script. ( maybe we should throw an exception when in debug mode for missing classes? )

If non-prefixed class names are not allowed, then an exception should be thrown if your module name is not prefixed by mw, jquery or ext prefix.

#Comment by Catrope (talk | contribs)   21:17, 9 May 2011

Hmm, I thought I had my bases covered for prefix-less modules, guess not. Will look at this tomorrow.

#Comment by Catrope (talk | contribs)   12:51, 20 May 2011

Tagging this and its followup r88076 for 1.17. The feature itself isn't direly needed, but the fix for the IE6 403 issue (either the current encode-dot-as-exclamation-mark patch or something fixing the actual problem) will build on this code and will be much easier to merge if this is merged.

#Comment by Brion VIBBER (talk | contribs)   17:57, 7 June 2011

phpunit test case added in r89666.

Status & tagging log