r62436 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62435‎ | r62436 | r62437 >
Date:15:08, 13 February 2010
Author:platonides
Status:reverted (Comments)
Tags:
Comment:
(Bug 14779) {{#ifexist}} does not recognise URL encoded filenames
Solution provided by Umherirrender
Modified paths:
  • /trunk/extensions/ParserFunctions/ParserFunctions_body.php (modified) (history)
  • /trunk/extensions/ParserFunctions/funcsParserTests.txt (modified) (history)

Diff [purge]

Index: trunk/extensions/ParserFunctions/ParserFunctions_body.php
——@@ -321,6 +321,9 @@
322322 function ifexistCommon( &$parser, $frame, $titletext = '', $then = '', $else = '' ) {
323323 global $wgContLang;
324324 $title = Title::newFromText( $titletext );
 325+ if( is_null( $title ) ) # It my be urlencoded (bug 14779)
 326+ $title = Title::newFromUrl( urldecode( $titletext ) );
 327+
325328 $wgContLang->findVariantLink( $titletext, $title, true );
326329 if ( $title ) {
327330 if( $title->getNamespace() == NS_MEDIA ) {
Index: trunk/extensions/ParserFunctions/funcsParserTests.txt
——@@ -6,6 +6,12 @@
77
88 # fixme: #time seems to be accepting input as local time, which strikes me as wrong
99
 10+!! article
 11+File:Dionysos-Brunnen am Kölner Dom.jpg
 12+!! text
 13+blah blah
 14+!! endarticle
 15+
1016 !! test
1117 Input times should probably be UTC, not local time
1218 !! input
——@@ -52,3 +58,34 @@
5359 &lt;DEF&gt;
5460 </p>
5561 !! end
 62+
 63+!! test
 64+{{#ifexist}}
 65+!! input
 66+{{#ifexist:Media:Foobar.jpg|Found|Not found}}
 67+{{#ifexist:Main Page|Found|Not found}}
 68+{{#ifexist:Missing|Found|Not found}}
 69+!! result
 70+<p>Found
 71+Found
 72+Not found
 73+</p>
 74+!! end
 75+
 76+!! test
 77+Bug 14779 - {{#ifexist}} does not recognise URL encoded filenames
 78+Based on http://test.wikipedia.org/wiki/User:Raymond/ifexist
 79+!! input
 80+{{#ifexist:Media:F%6fobar.jpg|found|not found}}
 81+[[:File:Dionysos-Brunnen am Kölner Dom.jpg]]
 82+{{#ifexist:File:Dionysos-Brunnen am Kölner Dom.jpg|found|not found}}
 83+[[:File:Dionysos-Brunnen_am_K%C3%B6lner_Dom.jpg]]
 84+{{#ifexist:File:Dionysos-Brunnen_am_K%C3%B6lner_Dom.jpg|found|not found}}
 85+!! result
 86+<p>found
 87+<a href="https://www.mediawiki.org/wiki/File:Dionysos-Brunnen_am_K%C3%B6lner_Dom.jpg" title="File:Dionysos-Brunnen am Kölner Dom.jpg">File:Dionysos-Brunnen am Kölner Dom.jpg</a>
 88+found
 89+<a href="https://www.mediawiki.org/wiki/File:Dionysos-Brunnen_am_K%C3%B6lner_Dom.jpg" title="File:Dionysos-Brunnen am Kölner Dom.jpg">File:Dionysos-Brunnen_am_Kölner_Dom.jpg</a>
 90+found
 91+</p>
 92+!! end

Follow-up revisions

RevisionCommit summaryAuthorDate
r62452Follow up r62436 comments....platonides23:58, 13 February 2010
r62695Revert r62436 and associated parser test. If you really want to make {{#ifexi...tstarling07:32, 19 February 2010

Comments

#Comment by Nikerabbit (talk | contribs)   16:20, 13 February 2010 
+if( is_null( $title ) ) # It my be urlencoded (bug 14779)

Spelling mistake, I would use === null. In addition use of {...} is encouraged in coding conventions. 

+$title = Title::newFromUrl( urldecode( $titletext ) );

This does not make any sense to me. It is not url-encoded any longer if you decode it. From Title::newFromUrl: 

Create a new Title from URL-encoded text. Ensures that the given title's length does not exceed the maximum.


#Comment by Platonides (talk | contribs)   23:14, 13 February 2010

That piece (except the typo) is copied from CoreParserFunctions::urlFunction

The description of Title::newFromUrl seems wrong. It will never create a title being passed, since Title::getTitleInvalidRegex() forbids url escaping sequences from titles.

I think the description should be "Create a new Title from text which was present on a url (already urldecoded)".

I don't know which is the right way to call it, though. Since SpecialListfiles.php and SpecialNewimages.php use it from a $wgRequest->getText which is stated as the wrong use on the comment. importTextFile.php uses it but for no reason.

However, $wgTitle is initialised precisely doing a Title::newFromURL( $wgRequest->getVal( 'title' ) )

Doing Title::newFromText( $wgRequest->getVal( 'title' ) ) decodes references which probably isn't wanted for urls. Title::newFromURL() allows x-www-form-urlencoded spaces (+ instead of %20).

I will have to ask Tim about it. On r51326 comment he said "Never use this function", so we may want to ditch it.

#Comment by Tim Starling (talk | contribs)   07:10, 19 February 2010

Title::newFromURL() should just be an alias to Title::newFromText(), since whenever anyone uses it, that is what they mean.

#Comment by Tim Starling (talk | contribs)   07:27, 19 February 2010

Note that Parser::replaceInternalLinks() (the inelegant behaviour of which you are presumably trying to emulate) uses Title::newFromText(urldecode($text)).

#Comment by Conrad.Irwin (talk | contribs)   12:30, 14 February 2010

Perhaps a cleaner idea would be to create Title::newFromPlainOrEncodedText (or a better name) to stop the need for duplication of this code.

Status & tagging log