r94353 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r94352‎ | r94353 | r94354 >
Date:14:55, 12 August 2011
Author:dantman
Status:resolved (Comments)
Tags:
Comment:
Followup r94349; Interwiki::getURL used `$title != null` to test if the $title arg was passed and should be substituted. However `"" == null`, so as a result switching to using the argument broke [[mw:]] style interwiki links without an article title.
Update the Interwiki::getURL code to use isset(), and update the comment to tell pre-1.19 supporting extensions to do the entire urlencoding and $1 substitution on their own since Interwiki::getURL was essentially buggy and broken before now.
Modified paths:
  • /trunk/phase3/includes/interwiki/Interwiki.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/interwiki/Interwiki.php
——@@ -315,13 +315,13 @@
316316 *
317317 * @param $title String: what text to put for the article name
318318 * @return String: the URL
319 - * @note Prior to 1.19 getURL did not urlencode the $title, if you use this
320 - * arg in an extension that supports MW earlier than 1.19 please ensure
321 - * you wfUrlencode it when installed in earlier versions of MW.
 319+ * @note Prior to 1.19 The getURL with an argument was broken.
 320+ * If you if you use this arg in an extension that supports MW earlier
 321+ * than 1.19 please wfUrlencode and substitute $1 on your own.
322322 */
323323 public function getURL( $title = null ) {
324324 $url = $this->mURL;
325 - if( $title != null ) {
 325+ if( isset($title) ) {
326326 $url = str_replace( "$1", wfUrlencode( $title ), $url );
327327 }
328328 return $url;

Follow-up revisions

RevisionCommit summaryAuthorDate
r94460Followup r94353; Use !== null since that's what we use in core.dantman14:20, 14 August 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r94349Update Interwiki::getURL's first argument so that it's properly urlencoded wh...dantman14:10, 12 August 2011

Comments

#Comment by Nikerabbit (talk | contribs)   07:09, 13 August 2011

What is wrong with !== null?

#Comment by Dantman (talk | contribs)   07:43, 13 August 2011

I wasn't aware that we used !== null in core. Could have sworn my practice of isset'ing optional args came from MediaWiki. !== null also sorta feels like a practice that could easily make more != null mistakes pop up and cause more bugs.

#Comment by Nikerabbit (talk | contribs)   10:38, 13 August 2011

Use is_null() then. Using isset() could easily make more bugs when the variable is renamed only partially.

#Comment by Dantman (talk | contribs)   10:40, 13 August 2011

Interesting... think we should push for the practice of using is_null()/!is_null() instead of isset or !== null?

#Comment by Catrope (talk | contribs)   10:43, 13 August 2011

I don't care much between is_null() and === null, they're equivalent. What I do care about is not using isset() for this, because it masks any mistakes you might make, such as variable name typos.

#Comment by Nikerabbit (talk | contribs)   10:45, 13 August 2011

I prefer !== null, but like said they are equivalent.

Status & tagging log