r92605 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r92604‎ | r92605 | r92606 >
Date:00:15, 20 July 2011
Author:owen
Status:ok (Comments)
Tags:
Comment:
added missing null checks in callers of Revision::newNullRevision
Modified paths:
  • /trunk/phase3/includes/filerepo/LocalFile.php (modified) (history)
  • /trunk/phase3/includes/specials/SpecialImport.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/filerepo/LocalFile.php
@@ -991,11 +991,12 @@
992992 $log->getRcComment(),
993993 false
994994 );
995 - $nullRevision->insertOn( $dbw );
 995+ if (!is_null($nullRevision)) {
 996+ $nullRevision->insertOn( $dbw );
996997
997 - wfRunHooks( 'NewRevisionFromEditComplete', array( $article, $nullRevision, $latest, $user ) );
998 - $article->updateRevisionOn( $dbw, $nullRevision );
999 -
 998+ wfRunHooks( 'NewRevisionFromEditComplete', array( $article, $nullRevision, $latest, $user ) );
 999+ $article->updateRevisionOn( $dbw, $nullRevision );
 1000+ }
10001001 # Invalidate the cache for the description page
10011002 $descTitle->invalidateCache();
10021003 $descTitle->purgeSquid();
Index: trunk/phase3/includes/specials/SpecialImport.php
@@ -384,11 +384,13 @@
385385 $dbw = wfGetDB( DB_MASTER );
386386 $latest = $title->getLatestRevID();
387387 $nullRevision = Revision::newNullRevision( $dbw, $title->getArticleId(), $comment, true );
388 - $nullRevision->insertOn( $dbw );
389 - $article = new Article( $title );
390 - # Update page record
391 - $article->updateRevisionOn( $dbw, $nullRevision );
392 - wfRunHooks( 'NewRevisionFromEditComplete', array($article, $nullRevision, $latest, $wgUser) );
 388+ if (!is_null($nullRevision)) {
 389+ $nullRevision->insertOn( $dbw );
 390+ $article = new Article( $title );
 391+ # Update page record
 392+ $article->updateRevisionOn( $dbw, $nullRevision );
 393+ wfRunHooks( 'NewRevisionFromEditComplete', array($article, $nullRevision, $latest, $wgUser) );
 394+ }
393395 } else {
394396 $wgOut->addHTML( "<li>" . Linker::linkKnown( $title ) . " " .
395397 wfMsgHtml( 'import-nonewrevisions' ) . "</li>\n" );

Comments

#Comment by Owyn (talk | contribs)   00:15, 20 July 2011

One question: should the call to wfRunHooks be inside or outside of this null check?

#Comment by Aaron Schulz (talk | contribs)   17:54, 9 September 2011

Inside, as it is now.

#Comment by Nikerabbit (talk | contribs)   09:30, 20 July 2011

What are the cases when it can return null?

#Comment by Owyn (talk | contribs)   17:10, 20 July 2011

It returns null when the underlying database query finds no rows. It could happen if the page id does not exist or the current page revision is missing. I'm not sure what the root cause of THAT is though, possibly slave lag? Also, SpecialImport calls Article->updateRevisionOn() before the hook and localFile calls it after the hook . I suspect it should probably run the hook after the article is updated in both cases.

#Comment by Platonides (talk | contribs)   17:19, 20 July 2011

It seems to me that if the page doesn't exist, it should throw a MWEception, since that should never happen.

#Comment by Aaron Schulz (talk | contribs)   19:02, 2 August 2011

The callers or the Revision function itself?

#Comment by Owyn (talk | contribs)   19:49, 2 August 2011

The callers in this case... The revision function can return null but nothing was checking for that return value. It's only used in a few places so it seemed easier to fix with a check for null rather than rewriting that function to throw an exception.

Status & tagging log