r64726 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r64725‎ | r64726 | r64727 >
Date:00:29, 8 April 2010
Author:conrad
Status:ok (Comments)
Tags:
Comment:
Bug 22474, urlencode now takes a parameter to change escaping style
Modified paths:
  • /trunk/phase3/RELEASE-NOTES (modified) (history)
  • /trunk/phase3/includes/parser/CoreParserFunctions.php (modified) (history)
  • /trunk/phase3/languages/messages/MessagesEn.php (modified) (history)
  • /trunk/phase3/maintenance/parserTests.txt (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/parser/CoreParserFunctions.php
@@ -124,8 +124,37 @@
125125 return wfUrlencode( str_replace( ' ', '_', self::ns( $parser, $part1 ) ) );
126126 }
127127
128 - static function urlencode( $parser, $s = '' ) {
129 - return urlencode( $s );
 128+ /**
 129+ * urlencodes a string according to one of three patterns: (bug 22474)
 130+ *
 131+ * By default (for HTTP "query" strings), spaces are encoded as '+'.
 132+ * Or to encode a value for the HTTP "path", spaces are encoded as '%20'.
 133+ * For links to "wiki"s, or similar software, spaces are encoded as '_',
 134+ *
 135+ * @param $parser.
 136+ * @param $s String: The text to encode.
 137+ * @param $arg String (optional): The type of encoding.
 138+ */
 139+ static function urlencode( $parser, $s = '', $arg = null ) {
 140+ static $magicWords = null;
 141+ if ( is_null( $magicWords ) ) {
 142+ $magicWords = new MagicWordArray( array( 'url_path', 'url_query', 'url_wiki' ) );
 143+ }
 144+ switch( $magicWords->matchStartToEnd( $arg ) ) {
 145+
 146+ // Encode as though it's a wiki page, '_' for ' '.
 147+ case 'url_wiki':
 148+ return wfUrlencode( str_replace( ' ', '_', $s ) );
 149+
 150+ // Encode for an HTTP Path, '%20' for ' '.
 151+ case 'url_path':
 152+ return rawurlencode( $s );
 153+
 154+ // Encode for HTTP query, '+' for ' '.
 155+ case 'url_query':
 156+ default:
 157+ return urlencode( $s );
 158+ }
130159 }
131160
132161 static function lcfirst( $parser, $s = '' ) {
Index: trunk/phase3/languages/messages/MessagesEn.php
@@ -352,6 +352,9 @@
353353 'staticredirect' => array( 1, '__STATICREDIRECT__' ),
354354 'protectionlevel' => array( 1, 'PROTECTIONLEVEL' ),
355355 'formatdate' => array( 0, 'formatdate', 'dateformat' ),
 356+ 'url_path' => array( 0, 'PATH' ),
 357+ 'url_wiki' => array( 0, 'WIKI' ),
 358+ 'url_query' => array( 0, 'QUERY' ),
356359 );
357360
358361 /**
Index: trunk/phase3/RELEASE-NOTES
@@ -41,6 +41,8 @@
4242 changes list
4343 * (bug 22925) "sp-contributions-blocked-notice-anon" message now displayed when
4444 viewing contributions of a blocked IP address
 45+* (bug 22474) {{urlencode:}} now takes an optional second paramter for type of
 46+ escaping.
4547
4648 === Bug fixes in 1.17 ===
4749 * (bug 17560) Half-broken deletion moved image files to deletion archive
Index: trunk/phase3/maintenance/parserTests.txt
@@ -2177,6 +2177,21 @@
21782178 !! end
21792179
21802180
 2181+!! test
 2182+Urlencode
 2183+!! input
 2184+{{urlencode:hi world?!}}
 2185+{{urlencode:hi world?!|WIKI}}
 2186+{{urlencode:hi world?!|PATH}}
 2187+{{urlencode:hi world?!|QUERY}}
 2188+!! result
 2189+<p>hi+world%3F%21
 2190+hi_world%3F!
 2191+hi%20world%3F%21
 2192+hi+world%3F%21
 2193+</p>
 2194+!! end
 2195+
21812196 ###
21822197 ### Magic links
21832198 ###

Comments

#Comment by Werdna (talk | contribs)   03:44, 9 December 2010

Seems OK to me, but I'm not familiar enough with magic words to mark as ok.

#Comment by MarkAHershberger (talk | contribs)   17:25, 10 December 2010

+ case 'url_query':

+			default:

Looks good, but I would add another magic word test, say |UNKNOWN so that this could be tested in cases where it is changed.

#Comment by Conrad.Irwin (talk | contribs)   20:01, 10 December 2010

The point of the default is not to provide behaviour for "UNKNOWN" — just to replicate the behaviour when there is no parameter specified, which is tested. If you think that passing an unknown parameter should always work as though no parameter is passed, then maybe it needs testing too. I would prefer to leave that behaviour undefined until it's clear that showing an error message is the wrong thing to do.

#Comment by MarkAHershberger (talk | contribs)   20:25, 10 December 2010

If an unknown parameter is specified, and you think the proper thing to do is return an error, then you should do that and test for this behavior.

#Comment by Conrad.Irwin (talk | contribs)   20:36, 10 December 2010

I don't know what the proper behaviour is — that's why I didn't test for it :p. I'm not too fussed either way, on the one hand I like to encourage people to use the right syntax, on the other, it's pretty unfriendly if it "just breaks".

#Comment by MarkAHershberger (talk | contribs)   23:10, 10 December 2010

I understand, My intention was to suggest that the tests reflect the current behavior (error or no) so that the software does not change behavior unexpectedly.

Status & tagging log