From 5090af900c4b75f1fd27724a8c3b3ce8ef3e5dab Mon Sep 17 00:00:00 2001 From: Roan Kattouw Date: Fri, 26 Aug 2011 16:26:17 +0000 Subject: [PATCH] (bug 29246) Edit API occasionally throws "unknown error 231". Per comment #1 on the bug and a comment in ApiEditPage, I refactored internalAttemptSave() to return a status object. However, there are so many idiosyncracies wrt how EditPage handles various errors that I decided to put the AS_* error code in $status->value (thanks Chad for that tip) and use that to decide what to do (not part of Chad's tip). The resulting code is still a mess but at least Status objects from doEdit() are propagated now, and it'll be a little bit easier to migrate internalAttemptSave() to a proper Status-based architecture in the future. Or maybe we should just throw away EditPage and start with a blank screen, that sounds appealing to me :) --- includes/EditPage.php | 118 ++++++++++++++++++++++++----------- includes/api/ApiBase.php | 6 ++ includes/api/ApiEditPage.php | 17 +++-- 3 files changed, 96 insertions(+), 45 deletions(-) diff --git a/includes/EditPage.php b/includes/EditPage.php index 20a1f4834a..86193d7769 100644 --- a/includes/EditPage.php +++ b/includes/EditPage.php @@ -853,32 +853,42 @@ class EditPage { * @param $result * @param $bot bool * - * @return int one of the constants describing the result + * @return Status object, possibly with a message, but always with one of the AS_* constants in $status->value, + * + * FIXME: This interface is TERRIBLE, but hard to get rid of due to various error display idiosyncrasies. There are + * also lots of cases where error metadata is set in the object and retrieved later instead of being returned, e.g. + * AS_CONTENT_TOO_BIG and AS_BLOCKED_PAGE_FOR_USER. All that stuff needs to be cleaned up some time. */ function internalAttemptSave( &$result, $bot = false ) { global $wgFilterCallback, $wgUser, $wgRequest, $wgParser; global $wgMaxArticleSize; + + $status = Status::newGood(); wfProfileIn( __METHOD__ ); wfProfileIn( __METHOD__ . '-checks' ); if ( !wfRunHooks( 'EditPage::attemptSave', array( $this ) ) ) { wfDebug( "Hook 'EditPage::attemptSave' aborted article saving\n" ); + $status->fatal( 'hookaborted' ); + $status->value = self::AS_HOOK_ERROR; wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_HOOK_ERROR; + return $status; } # Check image redirect if ( $this->mTitle->getNamespace() == NS_FILE && Title::newFromRedirect( $this->textbox1 ) instanceof Title && !$wgUser->isAllowed( 'upload' ) ) { - $isAnon = $wgUser->isAnon(); + $code = $wgUser->isAnon() ? self::AS_IMAGE_REDIRECT_ANON : self::AS_IMAGE_REDIRECT_LOGGED; + $status->setResult( false, $code ); wfProfileOut( __METHOD__ . '-checks' ); + wfProfileOut( __METHOD__ ); - return $isAnon ? self::AS_IMAGE_REDIRECT_ANON : self::AS_IMAGE_REDIRECT_LOGGED; + return $status; } # Check for spam @@ -892,71 +902,88 @@ class EditPage { $pdbk = $this->mTitle->getPrefixedDBkey(); $match = str_replace( "\n", '', $match ); wfDebugLog( 'SpamRegex', "$ip spam regex hit [[$pdbk]]: \"$match\"" ); + $status->fatal( 'spamprotectionmatch', $match ); + $status->value = self::AS_SPAM_ERROR; wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_SPAM_ERROR; + return $status; } if ( $wgFilterCallback && $wgFilterCallback( $this->mTitle, $this->textbox1, $this->section, $this->hookError, $this->summary ) ) { # Error messages or other handling should be performed by the filter function + $status->setResult( false, self::AS_FILTERING ); wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_FILTERING; + return $status; } if ( !wfRunHooks( 'EditFilter', array( $this, $this->textbox1, $this->section, &$this->hookError, $this->summary ) ) ) { # Error messages etc. could be handled within the hook... + $status->fatal( 'hookaborted' ); + $status->value = self::AS_HOOK_ERROR; wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_HOOK_ERROR; + return $status; } elseif ( $this->hookError != '' ) { # ...or the hook could be expecting us to produce an error + $status->fatal( 'hookaborted' ); + $status->value = self::AS_HOOK_ERROR_EXPECTED; wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_HOOK_ERROR_EXPECTED; + return $status; } if ( $wgUser->isBlockedFrom( $this->mTitle, false ) ) { # Check block state against master, thus 'false'. + $status->setResult( false, self::AS_BLOCKED_PAGE_FOR_USER ); wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_BLOCKED_PAGE_FOR_USER; + return $status; } $this->kblength = (int)( strlen( $this->textbox1 ) / 1024 ); if ( $this->kblength > $wgMaxArticleSize ) { // Error will be displayed by showEditForm() $this->tooBig = true; + $status->setResult( false, self::AS_CONTENT_TOO_BIG ); wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_CONTENT_TOO_BIG; + return $status; } if ( !$wgUser->isAllowed( 'edit' ) ) { if ( $wgUser->isAnon() ) { + $status->setResult( false, self::AS_READ_ONLY_PAGE_ANON ); wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_READ_ONLY_PAGE_ANON; + return $status; } else { + $status->fatal( 'readonlytext' ); + $status->value = self::AS_READ_ONLY_PAGE_LOGGED; wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_READ_ONLY_PAGE_LOGGED; + return $status; } } if ( wfReadOnly() ) { + $status->fatal( 'readonlytext' ); + $status->value = self::AS_READ_ONLY_PAGE; wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_READ_ONLY_PAGE; + return $status; } if ( $wgUser->pingLimiter() ) { + $status->fatal( 'actionthrottledtext' ); + $status->value = self::AS_RATE_LIMITED; wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_RATE_LIMITED; + return $status; } # If the article has been deleted while editing, don't save it without # confirmation if ( $this->wasDeletedSinceLastEdit() && !$this->recreate ) { + $status->setResult( false, self::AS_ARTICLE_WAS_DELETED ); wfProfileOut( __METHOD__ . '-checks' ); wfProfileOut( __METHOD__ ); - return self::AS_ARTICLE_WAS_DELETED; + return $status; } wfProfileOut( __METHOD__ . '-checks' ); @@ -968,34 +995,43 @@ class EditPage { if ( $new ) { // Late check for create permission, just in case *PARANOIA* if ( !$this->mTitle->userCan( 'create' ) ) { + $status->fatal( 'nocreatetext' ); + $status->value = self::AS_NO_CREATE_PERMISSION; wfDebug( __METHOD__ . ": no create permission\n" ); wfProfileOut( __METHOD__ ); - return self::AS_NO_CREATE_PERMISSION; + return $status; } # Don't save a new article if it's blank. if ( $this->textbox1 == '' ) { + $status->setResult( false, self::AS_BLANK_ARTICLE ); wfProfileOut( __METHOD__ ); - return self::AS_BLANK_ARTICLE; + return $status; } // Run post-section-merge edit filter if ( !wfRunHooks( 'EditFilterMerged', array( $this, $this->textbox1, &$this->hookError, $this->summary ) ) ) { # Error messages etc. could be handled within the hook... + $status->fatal( 'hookaborted' ); + $status->value = self::AS_HOOK_ERROR; wfProfileOut( __METHOD__ ); - return self::AS_HOOK_ERROR; + return $status; } elseif ( $this->hookError != '' ) { # ...or the hook could be expecting us to produce an error + $status->fatal( 'hookaborted' ); + $status->value = self::AS_HOOK_ERROR_EXPECTED; wfProfileOut( __METHOD__ ); - return self::AS_HOOK_ERROR_EXPECTED; + return $status; } # Handle the user preference to force summaries here. Check if it's not a redirect. if ( !$this->allowBlankSummary && !Title::newFromRedirect( $this->textbox1 ) ) { if ( md5( $this->summary ) == $this->autoSumm ) { $this->missingSummary = true; + $status->fatal( 'missingsummary' ); // or 'missingcommentheader' if $section == 'new'. Blegh + $status->value = self::AS_SUMMARY_NEEDED; wfProfileOut( __METHOD__ ); - return self::AS_SUMMARY_NEEDED; + return $status; } } @@ -1004,7 +1040,7 @@ class EditPage { $text = wfMsgForContent( 'newsectionheaderdefaultlevel', $this->summary ) . "\n\n" . $text; } - $retval = self::AS_SUCCESS_NEW_ARTICLE; + $status->value = self::AS_SUCCESS_NEW_ARTICLE; } else { @@ -1061,19 +1097,24 @@ class EditPage { } if ( $this->isConflict ) { + $status->setResult( false, self::AS_CONFLICT_DETECTED ); wfProfileOut( __METHOD__ ); - return self::AS_CONFLICT_DETECTED; + return $status; } // Run post-section-merge edit filter if ( !wfRunHooks( 'EditFilterMerged', array( $this, $text, &$this->hookError, $this->summary ) ) ) { # Error messages etc. could be handled within the hook... + $status->fatal( 'hookaborted' ); + $status->value = self::AS_HOOK_ERROR; wfProfileOut( __METHOD__ ); - return self::AS_HOOK_ERROR; + return $status; } elseif ( $this->hookError != '' ) { # ...or the hook could be expecting us to produce an error + $status->fatal( 'hookaborted' ); + $status->value = self::AS_HOOK_ERROR_EXPECTED; wfProfileOut( __METHOD__ ); - return self::AS_HOOK_ERROR_EXPECTED; + return $status; } # Handle the user preference to force summaries here, but not for null edits @@ -1092,8 +1133,10 @@ class EditPage { if ( $this->section == 'new' && !$this->allowBlankSummary ) { if ( trim( $this->summary ) == '' ) { $this->missingSummary = true; + $status->fatal( 'missingsummary' ); // or 'missingcommentheader' if $section == 'new'. Blegh + $status->value = self::AS_SUMMARY_NEEDED; wfProfileOut( __METHOD__ ); - return self::AS_SUMMARY_NEEDED; + return $status; } } @@ -1103,9 +1146,11 @@ class EditPage { if ( $this->section == 'new' ) { if ( $this->textbox1 == '' ) { $this->missingComment = true; + $status->fatal( 'missingcommenttext' ); + $status->value = self::AS_TEXTBOX_EMPTY; wfProfileOut( __METHOD__ . '-sectionanchor' ); wfProfileOut( __METHOD__ ); - return self::AS_TEXTBOX_EMPTY; + return $status; } if ( $this->summary != '' ) { $sectionanchor = $wgParser->guessLegacySectionNameFromWikiText( $this->summary ); @@ -1135,15 +1180,16 @@ class EditPage { $this->textbox1 = $text; $this->section = ''; - $retval = self::AS_SUCCESS_UPDATE; + $status->value = self::AS_SUCCESS_UPDATE; } // Check for length errors again now that the section is merged in $this->kblength = (int)( strlen( $text ) / 1024 ); if ( $this->kblength > $wgMaxArticleSize ) { $this->tooBig = true; + $status->setResult( false, self::AS_MAX_ARTICLE_SIZE_EXCEEDED ); wfProfileOut( __METHOD__ ); - return self::AS_MAX_ARTICLE_SIZE_EXCEEDED; + return $status; } $flags = EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY | @@ -1151,17 +1197,18 @@ class EditPage { ( ( $this->minoredit && !$this->isNew ) ? EDIT_MINOR : 0 ) | ( $bot ? EDIT_FORCE_BOT : 0 ); - $status = $this->mArticle->doEdit( $text, $this->summary, $flags ); + $doEditStatus = $this->mArticle->doEdit( $text, $this->summary, $flags ); - if ( $status->isOK() ) { + if ( $doEditStatus->isOK() ) { $result['redirect'] = Title::newFromRedirect( $text ) !== null; $this->commitWatch(); wfProfileOut( __METHOD__ ); - return $retval; + return $status; } else { $this->isConflict = true; + $doEditStatus->value = self::AS_END; // Destroys data doEdit() put in $status->value but who cares wfProfileOut( __METHOD__ ); - return self::AS_END; + return $doEditStatus; } } @@ -2833,13 +2880,14 @@ HTML $resultDetails = false; # Allow bots to exempt some edits from bot flagging $bot = $wgUser->isAllowed( 'bot' ) && $this->bot; - $value = $this->internalAttemptSave( $resultDetails, $bot ); + $status = $this->internalAttemptSave( $resultDetails, $bot ); + // FIXME: once the interface for internalAttemptSave() is made nicer, this should use the message in $status - if ( $value == self::AS_SUCCESS_UPDATE || $value == self::AS_SUCCESS_NEW_ARTICLE ) { + if ( $status->value == self::AS_SUCCESS_UPDATE || $status->value == self::AS_SUCCESS_NEW_ARTICLE ) { $this->didSave = true; } - switch ( $value ) { + switch ( $status->value ) { case self::AS_HOOK_ERROR_EXPECTED: case self::AS_CONTENT_TOO_BIG: case self::AS_ARTICLE_WAS_DELETED: diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php index ff005ee545..5c21f83548 100644 --- a/includes/api/ApiBase.php +++ b/includes/api/ApiBase.php @@ -1164,6 +1164,12 @@ abstract class ApiBase { 'emptynewsection' => array( 'code' => 'emptynewsection', 'info' => 'Creating empty new sections is not possible.' ), 'revwrongpage' => array( 'code' => 'revwrongpage', 'info' => "r\$1 is not a revision of ``\$2''" ), 'undo-failure' => array( 'code' => 'undofailure', 'info' => 'Undo failed due to conflicting intermediate edits' ), + + // Messages from WikiPage::doEit() + 'edit-hook-aborted' => array( 'code' => 'edit-hook-aborted', 'info' => "Your edit was aborted by an ArticleSave hook" ), + 'edit-gone-missing' => array( 'code' => 'edit-gone-missing', 'info' => "The page you tried to edit doesn't seem to exist anymore" ), + 'edit-conflict' => array( 'code' => 'editconflict', 'info' => "Edit conflict detected" ), + 'edit-already-exists' => array( 'code' => 'edit-already-exists', 'info' => "It seems the page you tried to create already exist" ), // uploadMsgs 'invalid-session-key' => array( 'code' => 'invalid-session-key', 'info' => 'Not a valid session key' ), diff --git a/includes/api/ApiEditPage.php b/includes/api/ApiEditPage.php index b04323381e..cf64ede2e3 100644 --- a/includes/api/ApiEditPage.php +++ b/includes/api/ApiEditPage.php @@ -270,11 +270,11 @@ class ApiEditPage extends ApiBase { $oldRequest = $wgRequest; $wgRequest = $req; - $retval = $ep->internalAttemptSave( $result, $wgUser->isAllowed( 'bot' ) && $params['bot'] ); + $status = $ep->internalAttemptSave( $result, $wgUser->isAllowed( 'bot' ) && $params['bot'] ); $wgRequest = $oldRequest; global $wgMaxArticleSize; - switch( $retval ) { + switch( $status->value ) { case EditPage::AS_HOOK_ERROR: case EditPage::AS_HOOK_ERROR_EXPECTED: $this->dieUsageMsg( 'hookaborted' ); @@ -353,15 +353,12 @@ class ApiEditPage extends ApiBase { $this->dieUsageMsg( 'summaryrequired' ); case EditPage::AS_END: - // This usually means some kind of race condition - // or DB weirdness occurred. Fall through to throw an unknown - // error. - - // This needs fixing higher up, as EditPage::internalAttemptSave - // should return the Status object, so that specific error - // conditions can be returned + // $status came from WikiPage::doEdit() + $errors = $status->getErrorsArray(); + $this->dieUsageMsg( $errors[0] ); // TODO: Add new errors to message map + break; default: - $this->dieUsageMsg( array( 'unknownerror', $retval ) ); + $this->dieUsageMsg( array( 'unknownerror', $status->value ) ); } $apiResult->addValue( null, $this->getModuleName(), $r ); } -- 2.20.1