r62933 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r62932‎ | r62933 | r62934 >
Date:17:47, 24 February 2010
Author:happydog
Status:ok (Comments)
Tags:
Comment:
CodeReview: Added some code to redirect to the repository list with an error message if the requested repository doesn't exist. Previously this would lead to PHP errors in most cases, as mRepo was not defined.

I also needed to add a check that mRepo is defined to authorWikiUser() as this is called even with the above change in place, resulting in a PHP error if the repository doesn't exist.
Modified paths:
  • /trunk/extensions/CodeReview/CodeReview.i18n.php (modified) (history)
  • /trunk/extensions/CodeReview/ui/SpecialCode.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CodeReview/CodeReview.i18n.php
——@@ -22,6 +22,7 @@
2323 'code-prop-changes' => 'Status & tagging log',
2424 'code-desc' => '[[Special:Code|Code review tool]] with [[Special:RepoAdmin|Subversion support]]',
2525 'code-no-repo' => 'No repository configured!',
 26+ 'code-repo-not-found' => 'Repository <b>$1</b> does not exist!',
2627 'code-load-diff' => 'Loading diff…',
2728 'code-notes' => 'recent comments',
2829 'code-statuschanges' => 'status changes',
Index: trunk/extensions/CodeReview/ui/SpecialCode.php
——@@ -91,6 +91,15 @@
9292 $wgOut->returnToMain( null, SpecialPage::getTitleFor( 'Code' ) );
9393 return;
9494 }
 95+
 96+ // If a repository was specified, but it does not exist, redirect to the
 97+ // repository list with an appropriate message.
 98+ if ( !$view->mRepo ) {
 99+ $view = new CodeRepoListView();
 100+ $wgOut->addHTML( "<p>"
 101+ . wfMsg( 'code-repo-not-found', $params[0] )
 102+ . "</p>" );
 103+ }
95104 }
96105 $view->execute();
97106
——@@ -129,7 +138,9 @@
130139 * of false
131140 */
132141 function authorWikiUser( $author ) {
133 - return $this->mRepo->authorWikiUser( $author );
 142+ if ( $this->mRepo )
 143+ return $this->mRepo->authorWikiUser( $author );
 144+ return false;
134145 }
135146
136147 function authorLink( $author, $extraParams = array() ) {

Follow-up revisions

RevisionCommit summaryAuthorDate
r63738CodeReview: Fixed up r62933 as per NikeRabbit's suggestions (http://www.media......happydog13:18, 14 March 2010

Comments

#Comment by Jack Phoenix (talk | contribs)   17:57, 24 February 2010 
+		if ( $this->mRepo )
+			return $this->mRepo->authorWikiUser( $author );

Please use braces here, as per our coding conventions.

#Comment by HappyDog (talk | contribs)   18:57, 24 February 2010

This code was copied from getRepo() in the same class, which doesn't use braces.

#Comment by Nikerabbit (talk | contribs)   19:46, 24 February 2010
  • Please check indentation.
  • Don't add new raw messages. $wgOut->addWikiMsg() seems to suit here well and it adds the <p>'s too. The way you currently pass $params[0] looks dangerous.
  • Optionally add documentation to qqq language for message code-repo-not-found which states what $1 is (and that the message is wikitext if you change that).

I'm not going to details here why addHtml + wfMsg is bad and addWikiMsg is good...

#Comment by HappyDog (talk | contribs)   11:12, 25 February 2010
  • Can you give further details about the indentation problems?
  • OK - wasn't aware of that function. Will fix it. You're right about params[0] - I will fix that too. What is the best method? htmlspecialchars(), or is there some internal function that should be used for this?
  • Where do I add this documentation?
#Comment by Nikerabbit (talk | contribs)   11:46, 25 February 2010
  • There is four more tabs on the lines after $wgOut->addHtml, which is quite much. If you are tying to align them to the <p>, I would suggest using spaces after you've reached the same indendation level as $wgOut->addHtml. Or just use one tab.
  • $wgOut->addWikiMsg( 'code-repo-not-found', $param[0] ); is safe, but you might want to still escape $param[0] for wikitext. Either wfEscapeWikiText() or &ltnowiki>&lt/nowiki> in the message. Latter solution is probably better if someone wants to play around the value.
  • In CodeReview.i18n.php there is $messages['qqq'] = array( after the definitions. It has the same syntax, and the qqq is just stupid name for documentation.
#Comment by HappyDog (talk | contribs)   13:26, 14 March 2010

All these issues addressed in r63738.

  • I used wfEscapeWikiText() for $param[0] - I'm not sure what you meant by the other alternative, or how it would help.
  • The new code is now all on one line, so no extra indentation is required any more. Just for reference, our standard house style is to use tabs for indentation, and old habits die hard! Our tab size fixed at 4, so this obviously works for us, but I can see how this breaks things for people with other tab sizes! I will try and make sure to avoid this, but this kind of thing may slip through occasionally as it is very easy to overlook. Feel free to pick me up on it again in the future (or just fix it yourself if that's easier).
  • Have added message to the 'qqq' array, but was pretty daunted by all the {{}} syntax going on in other messages - I hope I've done it right!

Status & tagging log