r95332 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r95331‎ | r95332 | r95333 >
Date:20:17, 23 August 2011
Author:brion
Status:resolved (Comments)
Tags:
Comment:
* (bug 30441) getParamValue must understand "+" encoding of space

$.param() produces query string form encoding using the traditional '+' encoding for space; mediawiki.util.getParamValue() was using only decodeURIComponent() to do unescaping, which is not required by spec to handle '+'. Explicitly replacing '+' with '%20' before the decode nicely resolves this.

Added a test case to qunit tests for mediawiki.util module.
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.util.js (modified) (history)
  • /trunk/phase3/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js (modified) (history)

Diff [purge]

Index: trunk/phase3/tests/qunit/suites/resources/mediawiki/mediawiki.util.test.js
——@@ -107,7 +107,7 @@
108108 });
109109
110110 test( 'getParamValue', function() {
111 - expect(3);
 111+ expect(4);
112112
113113 var url1 = 'http://mediawiki.org/?foo=wrong&foo=right#&foo=bad';
114114
——@@ -116,6 +116,9 @@
117117
118118 var url2 = 'http://mediawiki.org/#&foo=bad';
119119 strictEqual( mw.util.getParamValue( 'foo', url2 ), null, 'Ignore hash if param is not in querystring but in hash (bug 27427)' );
 120+
 121+ var url3 = 'example.com?' + $.param({ 'TEST': 'a b+c' });
 122+ strictEqual( mw.util.getParamValue( 'TEST', url3 ), 'a b+c', 'Bug 30441: getParamValue must understand "+" encoding of space' );
120123 });
121124
122125 test( 'tooltipAccessKey', function() {
Index: trunk/phase3/resources/mediawiki/mediawiki.util.js
——@@ -226,7 +226,9 @@
227227 var re = new RegExp( '^[^#]*[&?]' + $.escapeRE( param ) + '=([^&#]*)' );
228228 var m = re.exec( url );
229229 if ( m && m.length > 1 ) {
230 - return decodeURIComponent( m[1] );
 230+ // Beware that decodeURIComponent is not required to understand '+'
 231+ // by spec, as encodeURIComponent does not produce it.
 232+ return decodeURIComponent( m[1].replace( '+', '%20' ) );
231233 }
232234 return null;
233235 },

Follow-up revisions

RevisionCommit summaryAuthorDate
r95451Followup r95332 (bug 30441 fix) -- Roan pointed out that I forgot to do a glo...brion22:09, 24 August 2011
r96088MFT to REL1_18...hashar10:53, 2 September 2011
r964661.17wmf1: MFT r95332, r95451catrope18:39, 7 September 2011
r96559MFT r92422, r93520, r93563, r94107, r94433, r95042, r95332, r95451, r96386...reedy12:49, 8 September 2011

Comments

#Comment by Hashar (talk | contribs)   10:00, 24 August 2011

jQuery.param() internally uses encodeURIComponent() and then replace any '%20' occurrences with '+' : 

// global jQuery definition:
var r20 = /%20/g;

// jQuery.param() returns:
return s.join( "&" ).replace( r20, "+" );

Comparisons of escape() encodeURI() and encodeURIComponent() can be found with examples at:

http://xkr.us/articles/javascript/encode-compare/

#Comment by Catrope (talk | contribs)   12:43, 24 August 2011 
+				return decodeURIComponent( m[1].replace( '+', '%20' ) );

This doesn't work the way you expect. In JavaScript, .replace() only replaces the first occurrence, so: 

>>> url3 = 'example.com?' + $.param({ 'TEST': 'a b+c d+e' });
"example.com?TEST=a+b%2Bc+d%2Be"
>>> mw.util.getParamValue( 'TEST', url3 )
"a b+c+d+e"

You'll need to use a regex with the g (global) modifier.

#Comment by Brion VIBBER (talk | contribs)   22:02, 24 August 2011

Good catch! I'll add another test case and fix it up.

#Comment by Brion VIBBER (talk | contribs)   22:10, 24 August 2011

Fixed in r95451. Reset this rev to new for re-check.

Status & tagging log