Make doEditContent call $dbw->rollback() if exception happens

doEditContent calls a variety of methods in the middle of its
database transaction that can throw exceptions. For example if
$revision->insertOn() throws an exception, it can cause
referential integrity issues unless the transaction is rolled
back by something further up the chain (Thus see I5807e, I8f1da5).

It seems like it would be more reliable if doEditContent rolled
back its own transaction in exceptional circumstances. So this
patch puts everything in a try block, and does rollback if
an exception happens.

Bug: 63145
Bug: 32551
Change-Id: Icc44687da4edb1a72f13d2b95bfee2eea3ad6808

diff --git a/includes/WikiPage.php b/includes/WikiPage.php
index 18409b0..a5d66df 100644 (file)
--- a/includes/WikiPage.php
+++ b/includes/WikiPage.php
@@ -1812,56 +1812,62 @@ class WikiPage implements Page, IDBAccessObject {
                                }
 
                                $dbw->begin( __METHOD__ );
+                               try {
 
-                               $prepStatus = $content->prepareSave( $this, $flags, $baseRevId, $user );
-                               $status->merge( $prepStatus );
+                                       $prepStatus = $content->prepareSave( $this, $flags, $baseRevId, $user );
+                                       $status->merge( $prepStatus );
 
-                               if ( !$status->isOK() ) {
-                                       $dbw->rollback( __METHOD__ );
+                                       if ( !$status->isOK() ) {
+                                               $dbw->rollback( __METHOD__ );
 
-                                       wfProfileOut( __METHOD__ );
-                                       return $status;
-                               }
+                                               wfProfileOut( __METHOD__ );
+                                               return $status;
+                                       }
+                                       $revisionId = $revision->insertOn( $dbw );
 
-                               $revisionId = $revision->insertOn( $dbw );
+                                       // Update page
+                                       //
+                                       // Note that we use $this->mLatest instead of fetching a value from the master DB
+                                       // during the course of this function. This makes sure that EditPage can detect
+                                       // edit conflicts reliably, either by $ok here, or by $article->getTimestamp()
+                                       // before this function is called. A previous function used a separate query, this
+                                       // creates a window where concurrent edits can cause an ignored edit conflict.
+                                       $ok = $this->updateRevisionOn( $dbw, $revision, $oldid, $oldIsRedirect );
 
-                               // Update page
-                               //
-                               // Note that we use $this->mLatest instead of fetching a value from the master DB
-                               // during the course of this function. This makes sure that EditPage can detect
-                               // edit conflicts reliably, either by $ok here, or by $article->getTimestamp()
-                               // before this function is called. A previous function used a separate query, this
-                               // creates a window where concurrent edits can cause an ignored edit conflict.
-                               $ok = $this->updateRevisionOn( $dbw, $revision, $oldid, $oldIsRedirect );
+                                       if ( !$ok ) {
+                                               // Belated edit conflict! Run away!!
+                                               $status->fatal( 'edit-conflict' );
 
-                               if ( !$ok ) {
-                                       // Belated edit conflict! Run away!!
-                                       $status->fatal( 'edit-conflict' );
+                                               $dbw->rollback( __METHOD__ );
 
-                                       $dbw->rollback( __METHOD__ );
-
-                                       wfProfileOut( __METHOD__ );
-                                       return $status;
-                               }
+                                               wfProfileOut( __METHOD__ );
+                                               return $status;
+                                       }
 
-                               wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, $baseRevId, $user ) );
-                               // Update recentchanges
-                               if ( !( $flags & EDIT_SUPPRESS_RC ) ) {
-                                       // Mark as patrolled if the user can do so
-                                       $patrolled = $wgUseRCPatrol && !count(
+                                       wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, $baseRevId, $user ) );
+                                       // Update recentchanges
+                                       if ( !( $flags & EDIT_SUPPRESS_RC ) ) {
+                                               // Mark as patrolled if the user can do so
+                                               $patrolled = $wgUseRCPatrol && !count(
                                                $this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) );
-                                       // Add RC row to the DB
-                                       $rc = RecentChange::notifyEdit( $now, $this->mTitle, $isminor, $user, $summary,
-                                               $oldid, $this->getTimestamp(), $bot, '', $oldsize, $newsize,
-                                               $revisionId, $patrolled
-                                       );
+                                               // Add RC row to the DB
+                                               $rc = RecentChange::notifyEdit( $now, $this->mTitle, $isminor, $user, $summary,
+                                                       $oldid, $this->getTimestamp(), $bot, '', $oldsize, $newsize,
+                                                       $revisionId, $patrolled
+                                               );
 
-                                       // Log auto-patrolled edits
-                                       if ( $patrolled ) {
-                                               PatrolLog::record( $rc, true, $user );
+                                               // Log auto-patrolled edits
+                                               if ( $patrolled ) {
+                                                       PatrolLog::record( $rc, true, $user );
+                                               }
                                        }
+                                       $user->incEditCount();
+                               } catch ( MWException $e ) {
+                                       $dbw->rollback( __METHOD__ );
+                                       // Question: Would it perhaps be better if this method turned all
+                                       // exceptions into $status's?
+                                       throw $e;
                                }
-                               $user->incEditCount();
                                $dbw->commit( __METHOD__ );
                        } else {
                                // Bug 32948: revision ID must be set to page {{REVISIONID}} and
@@ -1891,74 +1897,80 @@ class WikiPage implements Page, IDBAccessObject {
                        $status->value['new'] = true;
 
                        $dbw->begin( __METHOD__ );
+                       try {
 
-                       $prepStatus = $content->prepareSave( $this, $flags, $baseRevId, $user );
-                       $status->merge( $prepStatus );
+                               $prepStatus = $content->prepareSave( $this, $flags, $baseRevId, $user );
+                               $status->merge( $prepStatus );
 
-                       if ( !$status->isOK() ) {
-                               $dbw->rollback( __METHOD__ );
+                               if ( !$status->isOK() ) {
+                                       $dbw->rollback( __METHOD__ );
 
-                               wfProfileOut( __METHOD__ );
-                               return $status;
-                       }
+                                       wfProfileOut( __METHOD__ );
+                                       return $status;
+                               }
 
-                       $status->merge( $prepStatus );
+                               $status->merge( $prepStatus );
 
-                       // Add the page record; stake our claim on this title!
-                       // This will return false if the article already exists
-                       $newid = $this->insertOn( $dbw );
+                               // Add the page record; stake our claim on this title!
+                               // This will return false if the article already exists
+                               $newid = $this->insertOn( $dbw );
 
-                       if ( $newid === false ) {
-                               $dbw->rollback( __METHOD__ );
-                               $status->fatal( 'edit-already-exists' );
+                               if ( $newid === false ) {
+                                       $dbw->rollback( __METHOD__ );
+                                       $status->fatal( 'edit-already-exists' );
 
-                               wfProfileOut( __METHOD__ );
-                               return $status;
-                       }
+                                       wfProfileOut( __METHOD__ );
+                                       return $status;
+                               }
 
-                       // Save the revision text...
-                       $revision = new Revision( array(
-                               'page'       => $newid,
-                               'title'      => $this->getTitle(), // for determining the default content model
-                               'comment'    => $summary,
-                               'minor_edit' => $isminor,
-                               'text'       => $serialized,
-                               'len'        => $newsize,
-                               'user'       => $user->getId(),
-                               'user_text'  => $user->getName(),
-                               'timestamp'  => $now,
-                               'content_model' => $content->getModel(),
-                               'content_format' => $serialisation_format,
-                       ) );
-                       $revisionId = $revision->insertOn( $dbw );
+                               // Save the revision text...
+                               $revision = new Revision( array(
+                                       'page'       => $newid,
+                                       'title'      => $this->getTitle(), // for determining the default content model
+                                       'comment'    => $summary,
+                                       'minor_edit' => $isminor,
+                                       'text'       => $serialized,
+                                       'len'        => $newsize,
+                                       'user'       => $user->getId(),
+                                       'user_text'  => $user->getName(),
+                                       'timestamp'  => $now,
+                                       'content_model' => $content->getModel(),
+                                       'content_format' => $serialisation_format,
+                               ) );
+                               $revisionId = $revision->insertOn( $dbw );
 
-                       // Bug 37225: use accessor to get the text as Revision may trim it
-                       $content = $revision->getContent(); // sanity; get normalized version
+                               // Bug 37225: use accessor to get the text as Revision may trim it
+                               $content = $revision->getContent(); // sanity; get normalized version
 
-                       if ( $content ) {
-                               $newsize = $content->getSize();
-                       }
+                               if ( $content ) {
+                                       $newsize = $content->getSize();
+                               }
 
-                       // Update the page record with revision data
-                       $this->updateRevisionOn( $dbw, $revision, 0 );
+                               // Update the page record with revision data
+                               $this->updateRevisionOn( $dbw, $revision, 0 );
 
-                       wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, false, $user ) );
+                               wfRunHooks( 'NewRevisionFromEditComplete', array( $this, $revision, false, $user ) );
 
-                       // Update recentchanges
-                       if ( !( $flags & EDIT_SUPPRESS_RC ) ) {
-                               // Mark as patrolled if the user can do so
-                               $patrolled = ( $wgUseRCPatrol || $wgUseNPPatrol ) && !count(
-                                       $this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) );
-                               // Add RC row to the DB
-                               $rc = RecentChange::notifyNew( $now, $this->mTitle, $isminor, $user, $summary, $bot,
-                                       '', $newsize, $revisionId, $patrolled );
+                               // Update recentchanges
+                               if ( !( $flags & EDIT_SUPPRESS_RC ) ) {
+                                       // Mark as patrolled if the user can do so
+                                       $patrolled = ( $wgUseRCPatrol || $wgUseNPPatrol ) && !count(
+                                               $this->mTitle->getUserPermissionsErrors( 'autopatrol', $user ) );
+                                       // Add RC row to the DB
+                                       $rc = RecentChange::notifyNew( $now, $this->mTitle, $isminor, $user, $summary, $bot,
+                                               '', $newsize, $revisionId, $patrolled );
 
-                               // Log auto-patrolled edits
-                               if ( $patrolled ) {
-                                       PatrolLog::record( $rc, true, $user );
+                                       // Log auto-patrolled edits
+                                       if ( $patrolled ) {
+                                               PatrolLog::record( $rc, true, $user );
+                                       }
                                }
+                               $user->incEditCount();
+
+                       } catch ( MWException $e ) {
+                               $dbw->rollback( __METHOD__ );
+                               throw $e;
                        }
-                       $user->incEditCount();
                        $dbw->commit( __METHOD__ );
 
                        // Update links, etc.