Convert doDeleteArticleReal to startAtomic()/endAtomic()

* They no longer commit the update, but just make sure
  it is part of a transaction. The BEGIN/COMMIT will
  happen at request start/end given DBO_TRX or in this
  method otherwise (like when in CLI mode). This avoids
  premature committing of other transactions.
* FileDeleteForm was the only caller using $commit=false
  for WikiPage::doDeleteArticleReal() and its proxies
  WikiPage::doDeleteArticle() and Article::doDeleteArticle().
  The ugly $commit flag is now removed.
* No caller was using $id for WikiPage::doDeleteArticleReal()
  and its proxies WikiPage::doDeleteArticle() and
  Article::doDeleteArticle(). It is now removed and we can
  be sure the lock() and CAS logic always hit in the method.
  The rollback() calls are not needed given the page row lock
  and having them there could break outer transactions.
* Updated FileDeleteForm to use startAtomic()/endAtomic() so
  the article and file delete are still wrapped in a
  transaction. Note that LocalFile::delete() does reference
  counting and trxLevel() checks so it will not try to begin()
  and break FileDeleteForm's transaction via startAtomic().
* Moved less important 'page-recent-delete' key update down
  for sanity in case it blows up.

Change-Id: Idb98510506c0edd02236c30badaec97d86e07696

diff --git a/RELEASE-NOTES-1.27 b/RELEASE-NOTES-1.27
index f8293b9..b3add69 100644 (file)
--- a/RELEASE-NOTES-1.27
+++ b/RELEASE-NOTES-1.27
@@ -90,6 +90,8 @@ changes to languages because of Bugzilla reports.
 
 === Other changes in 1.27 ===
 * ProfilerOutputUdp was removed. Note that there is a ProfilerOutputStats class.
+* WikiPage::doDeleteArticleReal() and WikiPage::doDeleteArticle() now
+  ignore the 2nd and 3rd arguments (formerly $id and $commit).
 
 == Compatibility ==
 

diff --git a/includes/FileDeleteForm.php b/includes/FileDeleteForm.php
index 5e7f5b2..8b41ad4 100644 (file)
--- a/includes/FileDeleteForm.php
+++ b/includes/FileDeleteForm.php
@@ -190,6 +190,7 @@ class FileDeleteForm {
                        $page = WikiPage::factory( $title );
                        $dbw = wfGetDB( DB_MASTER );
                        try {
+                               $dbw->startAtomic( __METHOD__ );
                                // delete the associated article first
                                $error = '';
                                $deleteStatus = $page->doDeleteArticleReal( $reason, $suppress, 0, false, $error, $user );
@@ -198,11 +199,15 @@ class FileDeleteForm {
                                if ( $deleteStatus->isOK() ) {
                                        $status = $file->delete( $reason, $suppress, $user );
                                        if ( $status->isOK() ) {
-                                               $dbw->commit( __METHOD__ );
                                                $status->value = $deleteStatus->value; // log id
+                                               $dbw->endAtomic( __METHOD__ );
                                        } else {
+                                               // Page deleted but file still there? rollback page delete
                                                $dbw->rollback( __METHOD__ );
                                        }
+                               } else {
+                                       // Done; nothing changed
+                                       $dbw->endAtomic( __METHOD__ );
                                }
                        } catch ( Exception $e ) {
                                // Rollback before returning to prevent UI from displaying

diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php
index 588ae6b..5eda550 100644 (file)
--- a/includes/filerepo/file/File.php
+++ b/includes/filerepo/file/File.php
@@ -1907,7 +1907,7 @@ abstract class File implements IDBAccessObject {
         * @param string $reason
         * @param bool $suppress Hide content from sysops?
         * @param User|null $user
-        * @return bool Boolean on success, false on some kind of failure
+        * @return FileRepoStatus
         * STUB
         * Overridden by LocalFile
         */

diff --git a/includes/page/Article.php b/includes/page/Article.php
index 43b1a47..5d6435e 100644 (file)
--- a/includes/page/Article.php
+++ b/includes/page/Article.php
@@ -2088,15 +2088,15 @@ class Article implements Page {
        /**
         * @param string $reason
         * @param bool $suppress
-        * @param int $id
-        * @param bool $commit
+        * @param int $u1 Unused
+        * @param bool $u2 Unused
         * @param string $error
         * @return bool
         */
-       public function doDeleteArticle( $reason, $suppress = false, $id = 0,
-               $commit = true, &$error = ''
+       public function doDeleteArticle(
+               $reason, $suppress = false, $u1 = null, $u2 = null, &$error = ''
        ) {
-               return $this->mPage->doDeleteArticle( $reason, $suppress, $id, $commit, $error );
+               return $this->mPage->doDeleteArticle( $reason, $suppress, $u1, $u2, $error );
        }
 
        /**

diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php
index cdaab1a..244e469 100644 (file)
--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@@ -2736,16 +2736,16 @@ class WikiPage implements Page, IDBAccessObject {
         * @param string $reason Delete reason for deletion log
         * @param bool $suppress Suppress all revisions and log the deletion in
         *        the suppression log instead of the deletion log
-        * @param int $id Article ID
-        * @param bool $commit Defaults to true, triggers transaction end
-        * @param array &$error Array of errors to append to
+        * @param int $u1 Unused
+        * @param bool $u2 Unused
+        * @param array|string &$error Array of errors to append to
         * @param User $user The deleting user
         * @return bool True if successful
         */
        public function doDeleteArticle(
-               $reason, $suppress = false, $id = 0, $commit = true, &$error = '', User $user = null
+               $reason, $suppress = false, $u1 = null, $u2 = null, &$error = '', User $user = null
        ) {
-               $status = $this->doDeleteArticleReal( $reason, $suppress, $id, $commit, $error, $user );
+               $status = $this->doDeleteArticleReal( $reason, $suppress, $u1, $u2, $error, $user );
                return $status->isGood();
        }
 
@@ -2758,16 +2758,16 @@ class WikiPage implements Page, IDBAccessObject {
         * @param string $reason Delete reason for deletion log
         * @param bool $suppress Suppress all revisions and log the deletion in
         *   the suppression log instead of the deletion log
-        * @param int $id Article ID
-        * @param bool $commit Defaults to true, triggers transaction end
-        * @param array &$error Array of errors to append to
+        * @param int $u1 Unused
+        * @param bool $u2 Unused
+        * @param array|string &$error Array of errors to append to
         * @param User $user The deleting user
         * @return Status Status object; if successful, $status->value is the log_id of the
         *   deletion log entry. If the page couldn't be deleted because it wasn't
         *   found, $status is a non-fatal 'cannotdelete' error
         */
        public function doDeleteArticleReal(
-               $reason, $suppress = false, $id = 0, $commit = true, &$error = '', User $user = null
+               $reason, $suppress = false, $u1 = null, $u2 = null, &$error = '', User $user = null
        ) {
                global $wgUser, $wgContentHandlerUseDB;
 
@@ -2793,25 +2793,28 @@ class WikiPage implements Page, IDBAccessObject {
                }
 
                $dbw = wfGetDB( DB_MASTER );
-               $dbw->begin( __METHOD__ );
-
-               if ( $id == 0 ) {
-                       $this->loadPageData( self::READ_LATEST );
-                       $id = $this->getID();
-                       // T98706: lock the page from various other updates but avoid using
-                       // WikiPage::READ_LOCKING as that will carry over the FOR UPDATE to
-                       // the revisions queries (which also JOIN on user). Only lock the page
-                       // row and CAS check on page_latest to see if the trx snapshot matches.
-                       $lockedLatest = $this->lock();
-                       if ( $id == 0 || $this->getLatest() != $lockedLatest ) {
-                               // Page not there or trx snapshot is stale
-                               $dbw->rollback( __METHOD__ );
-                               $status->error( 'cannotdelete',
-                                       wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) );
-                               return $status;
-                       }
+               $dbw->startAtomic( __METHOD__ );
+
+               $this->loadPageData( self::READ_LATEST );
+               $id = $this->getID();
+               // T98706: lock the page from various other updates but avoid using
+               // WikiPage::READ_LOCKING as that will carry over the FOR UPDATE to
+               // the revisions queries (which also JOIN on user). Only lock the page
+               // row and CAS check on page_latest to see if the trx snapshot matches.
+               $lockedLatest = $this->lock();
+               if ( $id == 0 || $this->getLatest() != $lockedLatest ) {
+                       $dbw->endAtomic( __METHOD__ );
+                       // Page not there or trx snapshot is stale
+                       $status->error( 'cannotdelete',
+                               wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) );
+                       return $status;
                }
 
+               // At this point we are now comitted to returning an OK
+               // status unless some DB query error or other exception comes up.
+               // This way callers don't have to call rollback() if $status is bad
+               // unless they actually try to catch exceptions (which is rare).
+
                // we need to remember the old content so we can use it to generate all deletion updates.
                $content = $this->getContent( Revision::RAW );
 
@@ -2864,24 +2867,20 @@ class WikiPage implements Page, IDBAccessObject {
                        $row['ar_content_format'] = 'rev_content_format';
                }
 
-               $dbw->insertSelect( 'archive', array( 'page', 'revision' ),
+               // Copy all the page revisions into the archive table
+               $dbw->insertSelect(
+                       'archive',
+                       array( 'page', 'revision' ),
                        $row,
                        array(
                                'page_id' => $id,
                                'page_id = rev_page'
-                       ), __METHOD__
+                       ),
+                       __METHOD__
                );
 
                // Now that it's safely backed up, delete it
                $dbw->delete( 'page', array( 'page_id' => $id ), __METHOD__ );
-               $ok = ( $dbw->affectedRows() > 0 ); // $id could be laggy
-
-               if ( !$ok ) {
-                       $dbw->rollback( __METHOD__ );
-                       $status->error( 'cannotdelete',
-                               wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) );
-                       return $status;
-               }
 
                if ( !$dbw->cascadingDeletes() ) {
                        $dbw->delete( 'revision', array( 'rev_page' => $id ), __METHOD__ );
@@ -2904,19 +2903,18 @@ class WikiPage implements Page, IDBAccessObject {
                        $logEntry->publish( $logid );
                } );
 
-               if ( $commit ) {
-                       $dbw->commit( __METHOD__ );
-               }
-
-               // Show log excerpt on 404 pages rather than just a link
-               $key = wfMemcKey( 'page-recent-delete', md5( $logTitle->getPrefixedText() ) );
-               ObjectCache::getMainStashInstance()->set( $key, 1, 86400 );
+               $dbw->endAtomic( __METHOD__ );
 
                $this->doDeleteUpdates( $id, $content );
 
                Hooks::run( 'ArticleDeleteComplete',
                        array( &$this, &$user, $reason, $id, $content, $logEntry ) );
                $status->value = $logid;
+
+               // Show log excerpt on 404 pages rather than just a link
+               $key = wfMemcKey( 'page-recent-delete', md5( $logTitle->getPrefixedText() ) );
+               ObjectCache::getMainStashInstance()->set( $key, 1, 86400 );
+
                return $status;
        }