r83610 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r83609‎ | r83610 | r83611 >
Date:21:50, 9 March 2011
Author:krinkle
Status:ok (Comments)
Tags:
Comment:
Escape minus in $.escapeRE
* Follow-up r83287: Removing slash before caret(^), apparently not needed.
* Escaping minus (-), adding the minus symbol (added in r83284), causes all uppercase characters and numbers to be escaped:
(bug 27960) $.escapeRE shouldn't escape all uppercase and numbers
* Added more compelete tets to the JS Test Suite for $.escapeRE to avoid problems in the future
Modified paths:
  • /trunk/phase3/resources/mediawiki.util/mediawiki.util.test.js (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki.util/mediawiki.util.test.js
——@@ -138,9 +138,18 @@
139139 mw.test.addTest( 'typeof $.escapeRE',
140140 'function (string)' );
141141
142 - mw.test.addTest( '$.escapeRE( ".st{e}$st" )',
143 - '\\.st\\{e\\}\\$st (string)' );
 142+ mw.test.addTest( '$.escapeRE( "<!-- ([{+mW+}]) $^|?>" )',
 143+ '<!\\-\\- \\(\\[\\{\\+mW\\+\\}\\]\\) \\$\\^\\|\\?> (string)' ); // double escaped
144144
 145+ mw.test.addTest( '$.escapeRE( "ABCDEFGHIJKLMNOPQRSTUVWXYZ" )',
 146+ 'ABCDEFGHIJKLMNOPQRSTUVWXYZ (string)' );
 147+
 148+ mw.test.addTest( '$.escapeRE( "abcdefghijklmnopqrstuvwxyz" )',
 149+ 'abcdefghijklmnopqrstuvwxyz (string)' );
 150+
 151+ mw.test.addTest( '$.escapeRE( "0123456789" )',
 152+ '0123456789 (string)' );
 153+
145154 mw.test.addTest( 'typeof $.isEmpty',
146155 'function (string)' );
147156
Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -14,7 +14,7 @@
1515 return str.substr( 0, 1 ).toUpperCase() + str.substr( 1, str.length );
1616 },
1717 escapeRE : function( str ) {
18 - return str.replace ( /([\\{}()|.?*+-\^$\[\]])/g, "\\$1" );
 18+ return str.replace ( /([\\{}()|.?*+\-^$\[\]])/g, "\\$1" );
1919 },
2020 // $.isDomElement( document.getElementById('content') ) === true
2121 // $.isDomElement( document.getElementsByClassName('portal') ) === false (array)

Follow-up revisions

RevisionCommit summaryAuthorDate
r839321.17wmf1: MFT r78990, r79844, r81548, r82022, r82193, r83061, r83067, r83583,...catrope17:59, 14 March 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r83284Fixed (bug 27848) Escape '-' in $.escapeRE()krinkle14:46, 5 March 2011
r83287Follow-up r83284. Fixing JSLint warnings: No need to escape a pipe (|) but a ...krinkle14:56, 5 March 2011

Comments

#Comment by Krinkle (talk | contribs)   21:55, 9 March 2011

The test suite has changed too much since the 1.17 branch to be backported.

The regex fix should be backported to 1.17 and 1.17wmf1 though, especially because the previous commit (r83284) that fixed a bug was also backported and caused a regression.

#Comment by He7d3r (talk | contribs)   22:23, 9 March 2011

Thanks for fixing it.

Another reason to backport is that the function is used to define mw.util.getParamValue, which is used by various scripts in use in some wikis. (this is how the bug was noticed on ptwiki)

Status & tagging log