From c2a52446e269bf4ec6184b6961ae9c39e47d5ad6 Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Wed, 7 Oct 2015 11:48:23 -0700 Subject: [PATCH] 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 --- RELEASE-NOTES-1.27 | 2 + includes/FileDeleteForm.php | 7 ++- includes/filerepo/file/File.php | 2 +- includes/page/Article.php | 10 ++-- includes/page/WikiPage.php | 84 ++++++++++++++++----------------- 5 files changed, 55 insertions(+), 50 deletions(-) diff --git a/RELEASE-NOTES-1.27 b/RELEASE-NOTES-1.27 index f8293b975b..b3add69744 100644 --- 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 5e7f5b2635..8b41ad4c73 100644 --- 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 588ae6b2fd..5eda550f9f 100644 --- 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 43b1a476a3..5d6435ee5c 100644 --- 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 cdaab1a6eb..244e469edc 100644 --- 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; } -- 2.20.1