r42063 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r42062‎ | r42063 | r42064 >
Date:00:21, 14 October 2008
Author:demon
Status:old (Comments)
Tags:
Comment:
Basic sanity checking. No reason to throw an exception everytime someone passes a bogus revid. Now you can do Special:Code/MediaWiki/1000000 and it won't explode, it'll just show the main listing :)
Modified paths:
  • /trunk/extensions/CodeReview/CodeRepository.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CodeReview/CodeRepository.php
——@@ -132,6 +132,9 @@
133133 * Load a particular revision out of the DB
134134 */
135135 function getRevision( $id ) {
 136+ if ( !$this->isValidRev( $id ) ) {
 137+ return null;
 138+ }
136139 $dbr = wfGetDB( DB_SLAVE );
137140 $row = $dbr->selectRow(
138141 'code_rev',
——@@ -178,4 +181,17 @@
179182
180183 return $data;
181184 }
 185+
 186+ /**
 187+ * Is the requested revid a valid revision to show?
 188+ * @return bool
 189+ * @param $rev int Rev id to check
 190+ */
 191+ function isValidRev( $rev ) {
 192+ $rev = intval( $rev );
 193+ if ( $rev > 0 && $rev <= $this->getLastStoredRev() ) {
 194+ return true;
 195+ }
 196+ return false;
 197+ }
182198 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r42065Followup to r42063, need to fail properly here as well.demon00:32, 14 October 2008

Comments

#Comment by Brion VIBBER (talk | contribs)   00:46, 15 October 2008

It would probably be better to show a warning that the requested revision doesn't exist, with a link back to the revision list for that repo.

As is, if the updating from the SVN master gets a bit behind, and I follow a link to where I think the last revision should be, I'm going to be pretty confused by just seeing a list of repositories.

#Comment by Werdna (talk | contribs)   04:13, 15 October 2008

... However, the behaviour above should be kept for at least Special:Code/MediaWiki/, which throws a scary exception now, when it should just redirect to Special:Code/MediaWiki.

#Comment by 😂 (talk | contribs)   15:52, 15 October 2008

I don't think this is the place for a UI output for this...this was a lower-level validation so we don't spew exceptions simply because someone passed a bogus revid.

Instead, we'd probably need to tweak CodeRevisionView's execute() to validate $mRepo and $mRev separately so we can give conditional errors. Relevant bit of code:

if( !$this->mRepo || !$this->mRev ) { $view = new CodeRepoListView(); $view->execute(); return; }

Should probably become

if( !$this->mRepo ) { $view = new CodeRepoListView(); $view->execute(); return; } if( !$this->mRev ) { // Some nicer way of failing on bad revs return; }

#Comment by 😂 (talk | contribs)   16:04, 15 October 2008

Did some preliminary cleanup in r42099. At least we now return the correct repo listing on a bad revid, but we could still use a nice message, per Brion.

Status & tagging log