r95562 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r95561‎ | r95562 | r95563 >
Date:16:26, 26 August 2011
Author:catrope
Status:resolved (Comments)
Tags:
Comment:
(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 :)
Modified paths:
  • /trunk/phase3/includes/EditPage.php (modified) (history)
  • /trunk/phase3/includes/api/ApiBase.php (modified) (history)
  • /trunk/phase3/includes/api/ApiEditPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/api/ApiEditPage.php
——@@ -270,11 +270,11 @@
271271 $oldRequest = $wgRequest;
272272 $wgRequest = $req;
273273
274 - $retval = $ep->internalAttemptSave( $result, $wgUser->isAllowed( 'bot' ) && $params['bot'] );
 274+ $status = $ep->internalAttemptSave( $result, $wgUser->isAllowed( 'bot' ) && $params['bot'] );
275275 $wgRequest = $oldRequest;
276276 global $wgMaxArticleSize;
277277
278 - switch( $retval ) {
 278+ switch( $status->value ) {
279279 case EditPage::AS_HOOK_ERROR:
280280 case EditPage::AS_HOOK_ERROR_EXPECTED:
281281 $this->dieUsageMsg( 'hookaborted' );
——@@ -353,15 +353,12 @@
354354 $this->dieUsageMsg( 'summaryrequired' );
355355
356356 case EditPage::AS_END:
357 - // This usually means some kind of race condition
358 - // or DB weirdness occurred. Fall through to throw an unknown
359 - // error.
360 -
361 - // This needs fixing higher up, as EditPage::internalAttemptSave
362 - // should return the Status object, so that specific error
363 - // conditions can be returned
 357+ // $status came from WikiPage::doEdit()
 358+ $errors = $status->getErrorsArray();
 359+ $this->dieUsageMsg( $errors[0] ); // TODO: Add new errors to message map
 360+ break;
364361 default:
365 - $this->dieUsageMsg( array( 'unknownerror', $retval ) );
 362+ $this->dieUsageMsg( array( 'unknownerror', $status->value ) );
366363 }
367364 $apiResult->addValue( null, $this->getModuleName(), $r );
368365 }
Index: trunk/phase3/includes/api/ApiBase.php
——@@ -1164,6 +1164,12 @@
11651165 'emptynewsection' => array( 'code' => 'emptynewsection', 'info' => 'Creating empty new sections is not possible.' ),
11661166 'revwrongpage' => array( 'code' => 'revwrongpage', 'info' => "r\$1 is not a revision of ``\$2''" ),
11671167 'undo-failure' => array( 'code' => 'undofailure', 'info' => 'Undo failed due to conflicting intermediate edits' ),
 1168+
 1169+ // Messages from WikiPage::doEit()
 1170+ 'edit-hook-aborted' => array( 'code' => 'edit-hook-aborted', 'info' => "Your edit was aborted by an ArticleSave hook" ),
 1171+ 'edit-gone-missing' => array( 'code' => 'edit-gone-missing', 'info' => "The page you tried to edit doesn't seem to exist anymore" ),
 1172+ 'edit-conflict' => array( 'code' => 'editconflict', 'info' => "Edit conflict detected" ),
 1173+ 'edit-already-exists' => array( 'code' => 'edit-already-exists', 'info' => "It seems the page you tried to create already exist" ),
11681174
11691175 // uploadMsgs
11701176 'invalid-session-key' => array( 'code' => 'invalid-session-key', 'info' => 'Not a valid session key' ),
Index: trunk/phase3/includes/EditPage.php
——@@ -853,32 +853,42 @@
854854 * @param $result
855855 * @param $bot bool
856856 *
857 - * @return int one of the constants describing the result
 857+ * @return Status object, possibly with a message, but always with one of the AS_* constants in $status->value,
 858+ *
 859+ * FIXME: This interface is TERRIBLE, but hard to get rid of due to various error display idiosyncrasies. There are
 860+ * also lots of cases where error metadata is set in the object and retrieved later instead of being returned, e.g.
 861+ * AS_CONTENT_TOO_BIG and AS_BLOCKED_PAGE_FOR_USER. All that stuff needs to be cleaned up some time.
858862 */
859863 function internalAttemptSave( &$result, $bot = false ) {
860864 global $wgFilterCallback, $wgUser, $wgRequest, $wgParser;
861865 global $wgMaxArticleSize;
 866+
 867+ $status = Status::newGood();
862868
863869 wfProfileIn( __METHOD__ );
864870 wfProfileIn( __METHOD__ . '-checks' );
865871
866872 if ( !wfRunHooks( 'EditPage::attemptSave', array( $this ) ) ) {
867873 wfDebug( "Hook 'EditPage::attemptSave' aborted article saving\n" );
 874+ $status->fatal( 'hookaborted' );
 875+ $status->value = self::AS_HOOK_ERROR;
868876 wfProfileOut( __METHOD__ . '-checks' );
869877 wfProfileOut( __METHOD__ );
870 - return self::AS_HOOK_ERROR;
 878+ return $status;
871879 }
872880
873881 # Check image redirect
874882 if ( $this->mTitle->getNamespace() == NS_FILE &&
875883 Title::newFromRedirect( $this->textbox1 ) instanceof Title &&
876884 !$wgUser->isAllowed( 'upload' ) ) {
877 - $isAnon = $wgUser->isAnon();
 885+ $code = $wgUser->isAnon() ? self::AS_IMAGE_REDIRECT_ANON : self::AS_IMAGE_REDIRECT_LOGGED;
 886+ $status->setResult( false, $code );
878887
879888 wfProfileOut( __METHOD__ . '-checks' );
 889+
880890 wfProfileOut( __METHOD__ );
881891
882 - return $isAnon ? self::AS_IMAGE_REDIRECT_ANON : self::AS_IMAGE_REDIRECT_LOGGED;
 892+ return $status;
883893 }
884894
885895 # Check for spam
——@@ -892,71 +902,88 @@
893903 $pdbk = $this->mTitle->getPrefixedDBkey();
894904 $match = str_replace( "\n", '', $match );
895905 wfDebugLog( 'SpamRegex', "$ip spam regex hit [[$pdbk]]: \"$match\"" );
 906+ $status->fatal( 'spamprotectionmatch', $match );
 907+ $status->value = self::AS_SPAM_ERROR;
896908 wfProfileOut( __METHOD__ . '-checks' );
897909 wfProfileOut( __METHOD__ );
898 - return self::AS_SPAM_ERROR;
 910+ return $status;
899911 }
900912 if ( $wgFilterCallback && $wgFilterCallback( $this->mTitle, $this->textbox1, $this->section, $this->hookError, $this->summary ) ) {
901913 # Error messages or other handling should be performed by the filter function
 914+ $status->setResult( false, self::AS_FILTERING );
902915 wfProfileOut( __METHOD__ . '-checks' );
903916 wfProfileOut( __METHOD__ );
904 - return self::AS_FILTERING;
 917+ return $status;
905918 }
906919 if ( !wfRunHooks( 'EditFilter', array( $this, $this->textbox1, $this->section, &$this->hookError, $this->summary ) ) ) {
907920 # Error messages etc. could be handled within the hook...
 921+ $status->fatal( 'hookaborted' );
 922+ $status->value = self::AS_HOOK_ERROR;
908923 wfProfileOut( __METHOD__ . '-checks' );
909924 wfProfileOut( __METHOD__ );
910 - return self::AS_HOOK_ERROR;
 925+ return $status;
911926 } elseif ( $this->hookError != '' ) {
912927 # ...or the hook could be expecting us to produce an error
 928+ $status->fatal( 'hookaborted' );
 929+ $status->value = self::AS_HOOK_ERROR_EXPECTED;
913930 wfProfileOut( __METHOD__ . '-checks' );
914931 wfProfileOut( __METHOD__ );
915 - return self::AS_HOOK_ERROR_EXPECTED;
 932+ return $status;
916933 }
917934 if ( $wgUser->isBlockedFrom( $this->mTitle, false ) ) {
918935 # Check block state against master, thus 'false'.
 936+ $status->setResult( false, self::AS_BLOCKED_PAGE_FOR_USER );
919937 wfProfileOut( __METHOD__ . '-checks' );
920938 wfProfileOut( __METHOD__ );
921 - return self::AS_BLOCKED_PAGE_FOR_USER;
 939+ return $status;
922940 }
923941 $this->kblength = (int)( strlen( $this->textbox1 ) / 1024 );
924942 if ( $this->kblength > $wgMaxArticleSize ) {
925943 // Error will be displayed by showEditForm()
926944 $this->tooBig = true;
 945+ $status->setResult( false, self::AS_CONTENT_TOO_BIG );
927946 wfProfileOut( __METHOD__ . '-checks' );
928947 wfProfileOut( __METHOD__ );
929 - return self::AS_CONTENT_TOO_BIG;
 948+ return $status;
930949 }
931950
932951 if ( !$wgUser->isAllowed( 'edit' ) ) {
933952 if ( $wgUser->isAnon() ) {
 953+ $status->setResult( false, self::AS_READ_ONLY_PAGE_ANON );
934954 wfProfileOut( __METHOD__ . '-checks' );
935955 wfProfileOut( __METHOD__ );
936 - return self::AS_READ_ONLY_PAGE_ANON;
 956+ return $status;
937957 } else {
 958+ $status->fatal( 'readonlytext' );
 959+ $status->value = self::AS_READ_ONLY_PAGE_LOGGED;
938960 wfProfileOut( __METHOD__ . '-checks' );
939961 wfProfileOut( __METHOD__ );
940 - return self::AS_READ_ONLY_PAGE_LOGGED;
 962+ return $status;
941963 }
942964 }
943965
944966 if ( wfReadOnly() ) {
 967+ $status->fatal( 'readonlytext' );
 968+ $status->value = self::AS_READ_ONLY_PAGE;
945969 wfProfileOut( __METHOD__ . '-checks' );
946970 wfProfileOut( __METHOD__ );
947 - return self::AS_READ_ONLY_PAGE;
 971+ return $status;
948972 }
949973 if ( $wgUser->pingLimiter() ) {
 974+ $status->fatal( 'actionthrottledtext' );
 975+ $status->value = self::AS_RATE_LIMITED;
950976 wfProfileOut( __METHOD__ . '-checks' );
951977 wfProfileOut( __METHOD__ );
952 - return self::AS_RATE_LIMITED;
 978+ return $status;
953979 }
954980
955981 # If the article has been deleted while editing, don't save it without
956982 # confirmation
957983 if ( $this->wasDeletedSinceLastEdit() && !$this->recreate ) {
 984+ $status->setResult( false, self::AS_ARTICLE_WAS_DELETED );
958985 wfProfileOut( __METHOD__ . '-checks' );
959986 wfProfileOut( __METHOD__ );
960 - return self::AS_ARTICLE_WAS_DELETED;
 987+ return $status;
961988 }
962989
963990 wfProfileOut( __METHOD__ . '-checks' );
——@@ -968,34 +995,43 @@
969996 if ( $new ) {
970997 // Late check for create permission, just in case *PARANOIA*
971998 if ( !$this->mTitle->userCan( 'create' ) ) {
 999+ $status->fatal( 'nocreatetext' );
 1000+ $status->value = self::AS_NO_CREATE_PERMISSION;
9721001 wfDebug( __METHOD__ . ": no create permission\n" );
9731002 wfProfileOut( __METHOD__ );
974 - return self::AS_NO_CREATE_PERMISSION;
 1003+ return $status;
9751004 }
9761005
9771006 # Don't save a new article if it's blank.
9781007 if ( $this->textbox1 == '' ) {
 1008+ $status->setResult( false, self::AS_BLANK_ARTICLE );
9791009 wfProfileOut( __METHOD__ );
980 - return self::AS_BLANK_ARTICLE;
 1010+ return $status;
9811011 }
9821012
9831013 // Run post-section-merge edit filter
9841014 if ( !wfRunHooks( 'EditFilterMerged', array( $this, $this->textbox1, &$this->hookError, $this->summary ) ) ) {
9851015 # Error messages etc. could be handled within the hook...
 1016+ $status->fatal( 'hookaborted' );
 1017+ $status->value = self::AS_HOOK_ERROR;
9861018 wfProfileOut( __METHOD__ );
987 - return self::AS_HOOK_ERROR;
 1019+ return $status;
9881020 } elseif ( $this->hookError != '' ) {
9891021 # ...or the hook could be expecting us to produce an error
 1022+ $status->fatal( 'hookaborted' );
 1023+ $status->value = self::AS_HOOK_ERROR_EXPECTED;
9901024 wfProfileOut( __METHOD__ );
991 - return self::AS_HOOK_ERROR_EXPECTED;
 1025+ return $status;
9921026 }
9931027
9941028 # Handle the user preference to force summaries here. Check if it's not a redirect.
9951029 if ( !$this->allowBlankSummary && !Title::newFromRedirect( $this->textbox1 ) ) {
9961030 if ( md5( $this->summary ) == $this->autoSumm ) {
9971031 $this->missingSummary = true;
 1032+ $status->fatal( 'missingsummary' ); // or 'missingcommentheader' if $section == 'new'. Blegh
 1033+ $status->value = self::AS_SUMMARY_NEEDED;
9981034 wfProfileOut( __METHOD__ );
999 - return self::AS_SUMMARY_NEEDED;
 1035+ return $status;
10001036 }
10011037 }
10021038
——@@ -1004,7 +1040,7 @@
10051041 $text = wfMsgForContent( 'newsectionheaderdefaultlevel', $this->summary ) . "\n\n" . $text;
10061042 }
10071043
1008 - $retval = self::AS_SUCCESS_NEW_ARTICLE;
 1044+ $status->value = self::AS_SUCCESS_NEW_ARTICLE;
10091045
10101046 } else {
10111047
——@@ -1061,19 +1097,24 @@
10621098 }
10631099
10641100 if ( $this->isConflict ) {
 1101+ $status->setResult( false, self::AS_CONFLICT_DETECTED );
10651102 wfProfileOut( __METHOD__ );
1066 - return self::AS_CONFLICT_DETECTED;
 1103+ return $status;
10671104 }
10681105
10691106 // Run post-section-merge edit filter
10701107 if ( !wfRunHooks( 'EditFilterMerged', array( $this, $text, &$this->hookError, $this->summary ) ) ) {
10711108 # Error messages etc. could be handled within the hook...
 1109+ $status->fatal( 'hookaborted' );
 1110+ $status->value = self::AS_HOOK_ERROR;
10721111 wfProfileOut( __METHOD__ );
1073 - return self::AS_HOOK_ERROR;
 1112+ return $status;
10741113 } elseif ( $this->hookError != '' ) {
10751114 # ...or the hook could be expecting us to produce an error
 1115+ $status->fatal( 'hookaborted' );
 1116+ $status->value = self::AS_HOOK_ERROR_EXPECTED;
10761117 wfProfileOut( __METHOD__ );
1077 - return self::AS_HOOK_ERROR_EXPECTED;
 1118+ return $status;
10781119 }
10791120
10801121 # Handle the user preference to force summaries here, but not for null edits
——@@ -1092,8 +1133,10 @@
10931134 if ( $this->section == 'new' && !$this->allowBlankSummary ) {
10941135 if ( trim( $this->summary ) == '' ) {
10951136 $this->missingSummary = true;
 1137+ $status->fatal( 'missingsummary' ); // or 'missingcommentheader' if $section == 'new'. Blegh
 1138+ $status->value = self::AS_SUMMARY_NEEDED;
10961139 wfProfileOut( __METHOD__ );
1097 - return self::AS_SUMMARY_NEEDED;
 1140+ return $status;
10981141 }
10991142 }
11001143
——@@ -1103,9 +1146,11 @@
11041147 if ( $this->section == 'new' ) {
11051148 if ( $this->textbox1 == '' ) {
11061149 $this->missingComment = true;
 1150+ $status->fatal( 'missingcommenttext' );
 1151+ $status->value = self::AS_TEXTBOX_EMPTY;
11071152 wfProfileOut( __METHOD__ . '-sectionanchor' );
11081153 wfProfileOut( __METHOD__ );
1109 - return self::AS_TEXTBOX_EMPTY;
 1154+ return $status;
11101155 }
11111156 if ( $this->summary != '' ) {
11121157 $sectionanchor = $wgParser->guessLegacySectionNameFromWikiText( $this->summary );
——@@ -1135,15 +1180,16 @@
11361181 $this->textbox1 = $text;
11371182 $this->section = '';
11381183
1139 - $retval = self::AS_SUCCESS_UPDATE;
 1184+ $status->value = self::AS_SUCCESS_UPDATE;
11401185 }
11411186
11421187 // Check for length errors again now that the section is merged in
11431188 $this->kblength = (int)( strlen( $text ) / 1024 );
11441189 if ( $this->kblength > $wgMaxArticleSize ) {
11451190 $this->tooBig = true;
 1191+ $status->setResult( false, self::AS_MAX_ARTICLE_SIZE_EXCEEDED );
11461192 wfProfileOut( __METHOD__ );
1147 - return self::AS_MAX_ARTICLE_SIZE_EXCEEDED;
 1193+ return $status;
11481194 }
11491195
11501196 $flags = EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY |
——@@ -1151,17 +1197,18 @@
11521198 ( ( $this->minoredit && !$this->isNew ) ? EDIT_MINOR : 0 ) |
11531199 ( $bot ? EDIT_FORCE_BOT : 0 );
11541200
1155 - $status = $this->mArticle->doEdit( $text, $this->summary, $flags );
 1201+ $doEditStatus = $this->mArticle->doEdit( $text, $this->summary, $flags );
11561202
1157 - if ( $status->isOK() ) {
 1203+ if ( $doEditStatus->isOK() ) {
11581204 $result['redirect'] = Title::newFromRedirect( $text ) !== null;
11591205 $this->commitWatch();
11601206 wfProfileOut( __METHOD__ );
1161 - return $retval;
 1207+ return $status;
11621208 } else {
11631209 $this->isConflict = true;
 1210+ $doEditStatus->value = self::AS_END; // Destroys data doEdit() put in $status->value but who cares
11641211 wfProfileOut( __METHOD__ );
1165 - return self::AS_END;
 1212+ return $doEditStatus;
11661213 }
11671214 }
11681215
——@@ -2833,13 +2880,14 @@
28342881 $resultDetails = false;
28352882 # Allow bots to exempt some edits from bot flagging
28362883 $bot = $wgUser->isAllowed( 'bot' ) && $this->bot;
2837 - $value = $this->internalAttemptSave( $resultDetails, $bot );
 2884+ $status = $this->internalAttemptSave( $resultDetails, $bot );
 2885+ // FIXME: once the interface for internalAttemptSave() is made nicer, this should use the message in $status
28382886
2839 - if ( $value == self::AS_SUCCESS_UPDATE || $value == self::AS_SUCCESS_NEW_ARTICLE ) {
 2887+ if ( $status->value == self::AS_SUCCESS_UPDATE || $status->value == self::AS_SUCCESS_NEW_ARTICLE ) {
28402888 $this->didSave = true;
28412889 }
28422890
2843 - switch ( $value ) {
 2891+ switch ( $status->value ) {
28442892 case self::AS_HOOK_ERROR_EXPECTED:
28452893 case self::AS_CONTENT_TOO_BIG:
28462894 case self::AS_ARTICLE_WAS_DELETED:

Follow-up revisions

RevisionCommit summaryAuthorDate
r95674Followup r95562: update callers to internalAttemptSave() in extensions. Thank...catrope16:06, 29 August 2011
r965081.18: MFT r95562, r95570, r95597, r95608, r95647, r95648, r95674, r95790, r95...catrope21:56, 7 September 2011
r97377Followup r95562: forgot to convert one case. Thanks to Nikerabbit for providi...catrope15:50, 17 September 2011

Comments

#Comment by Nikerabbit (talk | contribs)   13:16, 17 September 2011

This seems to miss one case, which results in warnings like: 

[17-Sep-2011 11:34:40] PHP Notice:  Trying to get property of non-object in /www/w/includes/EditPage.php on line 2893
[17-Sep-2011 11:34:40] PHP Notice:  Trying to get property of non-object in /www/w/includes/EditPage.php on line 2897

In trunk the offending line is 1135.

#Comment by Catrope (talk | contribs)   15:53, 17 September 2011

Fixed in r97377. Thanks for providing the line number, that made it easy to fix.

Status & tagging log