From: Tim Starling Date: Thu, 25 Sep 2008 10:15:19 +0000 (+0000) Subject: Re r41198: if you're going to break the interface, you may as well do it properly... X-Git-Tag: 1.31.0-rc.0~45104 X-Git-Url: https://git.cyclocoop.org/%7B%24admin_url%7Dmembres/%7B%7B%20url_for%28%27vote%27%2C%20idvote=vote.voteid%29%20%7D%7D?a=commitdiff_plain;h=95e847e5b5028314cd8aaa5e500e239272b600df;p=lhc%2Fweb%2Fwiklou.git Re r41198: if you're going to break the interface, you may as well do it properly. Rather than Article::doEdit() returning some random fragment of data that your current application requires, how about we have it return a general, extensible object? Also: * Fixed EDIT_NEW on existing article to return an error status instead of throw a DB exception * Fixed a bug dating from MW 1.5 whereby there's a short interval where an edit conflict can be missed, and an edit overwritten. See comment before updateRevisionOn() call. * Reduced some indenting levels using early returns --- diff --git a/includes/Article.php b/includes/Article.php index 318cd6c335..ced850ac93 100644 --- a/includes/Article.php +++ b/includes/Article.php @@ -1125,7 +1125,7 @@ class Article { * Best if all done inside a transaction. * * @param Database $dbw - * @return int The newly created page_id key + * @return int The newly created page_id key, or false if the title already existed * @private */ function insertOn( $dbw ) { @@ -1144,13 +1144,15 @@ class Article { 'page_touched' => $dbw->timestamp(), 'page_latest' => 0, # Fill this in shortly... 'page_len' => 0, # Fill this in shortly... - ), __METHOD__ ); - $newid = $dbw->insertId(); - - $this->mTitle->resetArticleId( $newid ); + ), __METHOD__, 'IGNORE' ); + $affected = $dbw->affectedRows(); + if ( $affected ) { + $newid = $dbw->insertId(); + $this->mTitle->resetArticleId( $newid ); + } wfProfileOut( __METHOD__ ); - return $newid; + return $affected ? $newid : false; } /** @@ -1363,29 +1365,31 @@ class Article { ( $minor ? EDIT_MINOR : 0 ) | ( $forceBot ? EDIT_FORCE_BOT : 0 ); - $good = $this->doEdit( $text, $summary, $flags ); - if ( $good ) { - $dbw = wfGetDB( DB_MASTER ); - if ($watchthis) { - if (!$this->mTitle->userIsWatching()) { - $dbw->begin(); - $this->doWatch(); - $dbw->commit(); - } - } else { - if ( $this->mTitle->userIsWatching() ) { - $dbw->begin(); - $this->doUnwatch(); - $dbw->commit(); - } + $status = $this->doEdit( $text, $summary, $flags ); + if ( !$status->isOK() ) { + return false; + } + + $dbw = wfGetDB( DB_MASTER ); + if ($watchthis) { + if (!$this->mTitle->userIsWatching()) { + $dbw->begin(); + $this->doWatch(); + $dbw->commit(); + } + } else { + if ( $this->mTitle->userIsWatching() ) { + $dbw->begin(); + $this->doUnwatch(); + $dbw->commit(); } + } - $extraQuery = ''; // Give extensions a chance to modify URL query on update - wfRunHooks( 'ArticleUpdateBeforeRedirect', array( $this, &$sectionanchor, &$extraQuery ) ); + $extraQuery = ''; // Give extensions a chance to modify URL query on update + wfRunHooks( 'ArticleUpdateBeforeRedirect', array( $this, &$sectionanchor, &$extraQuery ) ); - $this->doRedirect( $this->isRedirect( $text ), $sectionanchor, $extraQuery ); - } - return $good; + $this->doRedirect( $this->isRedirect( $text ), $sectionanchor, $extraQuery ); + return true; } /** @@ -1415,13 +1419,27 @@ class Article { * Fill in blank summaries with generated text where possible * * If neither EDIT_NEW nor EDIT_UPDATE is specified, the status of the article will be detected. - * If EDIT_UPDATE is specified and the article doesn't exist, the function will return false. If - * EDIT_NEW is specified and the article does exist, a duplicate key error will cause an exception - * to be thrown from the Database. These two conditions are also possible with auto-detection due - * to MediaWiki's performance-optimised locking strategy. + * If EDIT_UPDATE is specified and the article doesn't exist, the function will an + * edit-gone-missing error. If EDIT_NEW is specified and the article does exist, an + * edit-already-exists error will be returned. These two conditions are also possible with + * auto-detection due to MediaWiki's performance-optimised locking strategy. + * * @param $baseRevId, the revision ID this edit was based off, if any * - * @return int Current revision ID after this edit + * @return Status object. Possible errors: + * edit-hook-aborted: The ArticleSave hook aborted the edit but didn't set the fatal flag of $status + * edit-gone-missing: In update mode, but the article didn't exist + * edit-conflict: In update mode, the article changed unexpectedly + * edit-no-change: Warning that the text was the same as before + * edit-already-exists: In creation mode, but the article already exists + * + * Extensions may define additional errors. + * + * $return->value will contain an associative array with members as follows: + * new: Boolean indicating if the function attempted to create a new article + * revision: The revision object for the inserted revision, or null + * + * Compatibility note: this function previously returned a boolean value indicating success/failure */ function doEdit( $text, $summary, $flags = 0, $baseRevId = false, $user = null ) { global $wgUser, $wgDBtransactions, $wgUseAutomaticEditSummaries; @@ -1436,10 +1454,13 @@ class Article { if ($user == null) { $user = $wgUser; } - $good = true; + $status = Status::newGood( array() ); + + # Load $this->mTitle->getArticleID() and $this->mLatest if it's not already + $this->loadPageData(); if ( !($flags & EDIT_NEW) && !($flags & EDIT_UPDATE) ) { - $aid = $this->mTitle->getArticleID( GAID_FOR_UPDATE ); + $aid = $this->mTitle->getArticleID(); if ( $aid ) { $flags |= EDIT_UPDATE; } else { @@ -1449,11 +1470,14 @@ class Article { if( !wfRunHooks( 'ArticleSave', array( &$this, &$user, &$text, &$summary, $flags & EDIT_MINOR, - null, null, &$flags ) ) ) + null, null, &$flags, &$status ) ) ) { wfDebug( __METHOD__ . ": ArticleSave hook aborted save!\n" ); wfProfileOut( __METHOD__ ); - return false; + if ( $status->isOK() ) { + $status->fatal( 'edit-hook-aborted'); + } + return $status; } # Silently ignore EDIT_MINOR if not allowed @@ -1477,13 +1501,13 @@ class Article { if ( $flags & EDIT_UPDATE ) { # Update article, but only if changed. + $status->value['new'] = false; # Make sure the revision is either completely inserted or not inserted at all if( !$wgDBtransactions ) { $userAbort = ignore_user_abort( true ); } - $lastRevision = 0; $revisionId = 0; $changed = ( strcmp( $text, $oldtext ) != 0 ); @@ -1493,14 +1517,12 @@ class Article { - (int)$this->isCountable( $oldtext ); $this->mTotalAdjustment = 0; - $lastRevision = $dbw->selectField( - 'page', 'page_latest', array( 'page_id' => $this->getId() ) ); - - if ( !$lastRevision ) { + if ( !$this->mLatest ) { # Article gone missing wfDebug( __METHOD__.": EDIT_UPDATE specified but article doesn't exist\n" ); + $status->fatal( 'edit-gone-missing' ); wfProfileOut( __METHOD__ ); - return false; + return $status; } $revision = new Revision( array( @@ -1508,7 +1530,7 @@ class Article { 'comment' => $summary, 'minor_edit' => $isminor, 'text' => $text, - 'parent_id' => $lastRevision, + 'parent_id' => $this->mLatest, 'user' => $user->getId(), 'user_text' => $user->getName(), ) ); @@ -1517,11 +1539,21 @@ class Article { $revisionId = $revision->insertOn( $dbw ); # Update page - $ok = $this->updateRevisionOn( $dbw, $revision, $lastRevision ); + # + # 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, $this->mLatest ); if( !$ok ) { /* Belated edit conflict! Run away!! */ - $good = false; + $status->fatal( 'edit-conflict' ); + # Delete the invalid revision if the DB is not transactional + if ( !$wgDBtransactions ) { + $dbw->delete( 'revision', array( 'rev_id' => $revisionId ), __METHOD__ ); + } $revisionId = 0; $dbw->rollback(); } else { @@ -1530,7 +1562,7 @@ class Article { # Update recentchanges if( !( $flags & EDIT_SUPPRESS_RC ) ) { $rc = RecentChange::notifyEdit( $now, $this->mTitle, $isminor, $user, $summary, - $lastRevision, $this->getTimestamp(), $bot, '', $oldsize, $newsize, + $this->mLatest, $this->getTimestamp(), $bot, '', $oldsize, $newsize, $revisionId ); # Mark as patrolled if the user can do so @@ -1542,6 +1574,7 @@ class Article { $dbw->commit(); } } else { + $status->warning( 'edit-no-change' ); $revision = null; // Keep the same revision ID, but do some updates on it $revisionId = $this->getRevIdFetched(); @@ -1553,16 +1586,20 @@ class Article { if( !$wgDBtransactions ) { ignore_user_abort( $userAbort ); } - - if ( $good ) { - # Invalidate cache of this article and all pages using this article - # as a template. Partly deferred. - Article::onArticleEdit( $this->mTitle, false ); // leave templatelinks for editUpdates() - # Update links tables, site stats, etc. - $this->editUpdates( $text, $summary, $isminor, $now, $revisionId, $changed ); + // Now that ignore_user_abort is restored, we can respond to fatal errors + if ( !$status->isOK() ) { + wfProfileOut( __METHOD__ ); + return $status; } + + # Invalidate cache of this article and all pages using this article + # as a template. Partly deferred. + Article::onArticleEdit( $this->mTitle, false ); // leave templatelinks for editUpdates() + # Update links tables, site stats, etc. + $this->editUpdates( $text, $summary, $isminor, $now, $revisionId, $changed ); } else { # Create new article + $status->value['new'] = true; # Set statistics members # We work out if it's countable after PST to avoid counter drift @@ -1571,10 +1608,18 @@ class Article { $this->mTotalAdjustment = 1; $dbw->begin(); + # Add the page record; stake our claim on this title! - # This will fail with a database query exception if the article already exists + # This will return false if the article already exists $newid = $this->insertOn( $dbw ); + if ( $newid === false ) { + $dbw->rollback(); + $status->fatal( 'edit-already-exists' ); + wfProfileOut( __METHOD__ ); + return $status; + } + # Save the revision text... $revision = new Revision( array( 'page' => $newid, @@ -1614,17 +1659,19 @@ class Article { $flags & EDIT_MINOR, null, null, &$flags, $revision ) ); } - if ( $good && !( $flags & EDIT_DEFER_UPDATES ) ) { + # Do updates right now unless deferral was requested + if ( !( $flags & EDIT_DEFER_UPDATES ) ) { wfDoUpdates(); } - if ( $good ) { - wfRunHooks( 'ArticleSaveComplete', array( &$this, &$user, $text, $summary, - $flags & EDIT_MINOR, null, null, &$flags, $revision ) ); - } + // Return the new revision (or null) to the caller + $status->value['revision'] = $revision; + + wfRunHooks( 'ArticleSaveComplete', array( &$this, &$user, $text, $summary, + $flags & EDIT_MINOR, null, null, &$flags, $revision, &$status ) ); wfProfileOut( __METHOD__ ); - return $revisionId; + return $status; } /** @@ -2590,7 +2637,12 @@ class Article { if( $bot && ($wgUser->isAllowed('markbotedits') || $wgUser->isAllowed('bot')) ) $flags |= EDIT_FORCE_BOT; # Actually store the edit - $revId = $this->doEdit( $target->getText(), $summary, $flags, $target->getId() ); + $status = $this->doEdit( $target->getText(), $summary, $flags, $target->getId() ); + if ( !empty( $status->value['revision'] ) ) { + $revId = $status->value['revision']->getId(); + } else { + $revId = false; + } wfRunHooks( 'ArticleRollbackComplete', array( $this, $wgUser, $target ) ); diff --git a/languages/messages/MessagesEn.php b/languages/messages/MessagesEn.php index e0f1d01dac..6c6ebd990d 100644 --- a/languages/messages/MessagesEn.php +++ b/languages/messages/MessagesEn.php @@ -1190,6 +1190,11 @@ The deletion log for this page is provided here for convenience:", 'deleted-notice' => 'This page has been deleted. The deletion log for the page is provided below for reference.', 'deletelog-fulllog' => 'View full log', +'edit-hook-aborted' => 'Edit aborted by hook, it gave no explanation.', +'edit-gone-missing' => 'Couldn\'t update the article, it appears to have been deleted.', +'edit-conflict' => 'Edit conflict.', +'edit-no-change' => 'Your edit was ignored, because no change was made to the text.', +'edit-already-exists' => 'Couldn\'t create a new article, it already exists.', # Parser/template warnings 'expensive-parserfunction-warning' => 'Warning: This page contains too many expensive parser function calls. diff --git a/maintenance/edit.php b/maintenance/edit.php index 037f9a9aea..6417804505 100644 --- a/maintenance/edit.php +++ b/maintenance/edit.php @@ -58,15 +58,20 @@ $text = file_get_contents( 'php://stdin' ); # Do the edit print "Saving... "; -$success = $wgArticle->doEdit( $text, $summary, +$status = $wgArticle->doEdit( $text, $summary, ( $minor ? EDIT_MINOR : 0 ) | ( $bot ? EDIT_FORCE_BOT : 0 ) | ( $autoSummary ? EDIT_AUTOSUMMARY : 0 ) | ( $noRC ? EDIT_SUPPRESS_RC : 0 ) ); -if ( $success ) { +if ( $status->isOK() ) { print "done\n"; + $exit = 0; } else { print "failed\n"; - exit( 1 ); + $exit = 1; +} +if ( !$status->isGood() ) { + print $status->getWikiText() . "\n"; } +exit( $exit );