r70760 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70759‎ | r70760 | r70761 >
Date:08:33, 9 August 2010
Author:tstarling
Status:deferred
Tags:
Comment:
* Revert r66878, completely misses the point of factoring out doEdit() in the first place, which was to separate the UI layer from the backend layer, to support callers with alternate UIs or no UIs.
* Reverted followup 66880.
* Reverted dependent changes r67752, r68606, r68608, r68609. The point of deprecating insertArticle()/updateArticle() was to allow the UI code to be moved to EditPage. If you move that exact EditPage-specific functionality back into Article::doEdit(), and call it from all sorts of non-EditPage places, then we'll hit the same sorts of bugs we had before r14834.
Modified paths:
  • /trunk/extensions/DataTransclusion/OpenLibrarySource.php (modified) (history)
  • /trunk/extensions/DataTransclusion/XmlDataTransclusionSource.php (modified) (history)
  • /trunk/extensions/DataTransclusion/tests/fetchRecord.php (modified) (history)
  • /trunk/extensions/DataTransclusion/tests/fetchWebData.php (modified) (history)
  • /trunk/extensions/DynamicPageList/DPL.php (modified) (history)
  • /trunk/extensions/JS2Support/ResourceLoaderOutputPage.php (modified) (history)
  • /trunk/extensions/JS2Support/mwResourceLoader.php (modified) (history)
  • /trunk/extensions/Postcomment/Postcomment.php (modified) (history)
  • /trunk/extensions/SpamBlacklist/cleanup.php (modified) (history)
  • /trunk/extensions/SpamDiffTool/SpamDiffTool_body.php (modified) (history)
  • /trunk/extensions/Translate/groups/MantisBT/MantisBT.yml (modified) (history)
  • /trunk/extensions/YouTubeAuthSub/YouTubeAuthSub_body.php (modified) (history)
  • /trunk/phase3/docs/hooks.txt (modified) (history)
  • /trunk/phase3/includes/Article.php (modified) (history)
  • /trunk/phase3/includes/EditPage.php (modified) (history)
  • /trunk/phase3/includes/Title.php (modified) (history)
  • /trunk/phase3/includes/api/ApiEditPage.php (modified) (history)
  • /trunk/phase3/maintenance/addwiki.php (modified) (history)
  • /trunk/phase3/maintenance/parserTests.inc (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/parserTests.inc
——@@ -1050,8 +1050,7 @@
10511051 }
10521052
10531053 $art = new Article( $title );
1054 - $art->doEdit( $text, '', EDIT_NEW | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY,
1055 - false, null, false, false, '', true );
 1054+ $art->insertNewArticle( $text, '', false, false );
10561055
10571056 $this->teardownGlobals();
10581057 }
Index: trunk/phase3/maintenance/addwiki.php
——@@ -132,8 +132,7 @@
133133 $wgArticle = new Article( $wgTitle );
134134 $ucsite = ucfirst( $site );
135135
136 - $wgArticle->doEdit( $this->getFirstArticle( $ucsite, $name ), '', EDIT_NEW | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY,
137 - false, null, false, false, '', true );
 136+ $wgArticle->insertNewArticle( $this->getFirstArticle( $ucsite, $name ), '', false, false );
138137
139138 $this->output( "Adding to dblists\n" );
140139
Index: trunk/phase3/docs/hooks.txt
——@@ -660,7 +660,7 @@
661661 $editPage: EditPage object
662662
663663 'EditPage::attemptSave': called before an article is
664 -saved, that is before Article::doEdit() is called
 664+saved, that is before insertNewArticle() is called
665665 $editpage_Obj: the current EditPage object
666666
667667 'EditPage::importFormData': allow extensions to read additional data
Index: trunk/phase3/includes/Article.php
——@@ -1945,33 +1945,71 @@
19461946 }
19471947
19481948 /**
1949 - * @deprecated use Article::doEdit()
 1949+ * This function is not deprecated until somebody fixes the core not to use
 1950+ * it. Nevertheless, use Article::doEdit() instead.
19501951 */
19511952 function insertNewArticle( $text, $summary, $isminor, $watchthis, $suppressRC = false, $comment = false, $bot = false ) {
1952 - wfDeprecated( __METHOD__ );
19531953 $flags = EDIT_NEW | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY |
19541954 ( $isminor ? EDIT_MINOR : 0 ) |
19551955 ( $suppressRC ? EDIT_SUPPRESS_RC : 0 ) |
19561956 ( $bot ? EDIT_FORCE_BOT : 0 );
19571957
1958 - $this->doEdit( $text, $summary, $flags, false, null, $watchthis, $comment, '', true );
 1958+ # If this is a comment, add the summary as headline
 1959+ if ( $comment && $summary != "" ) {
 1960+ $text = wfMsgForContent( 'newsectionheaderdefaultlevel', $summary ) . "\n\n" . $text;
 1961+ }
 1962+ $this->doEdit( $text, $summary, $flags );
 1963+
 1964+ $dbw = wfGetDB( DB_MASTER );
 1965+ if ( $watchthis ) {
 1966+ if ( !$this->mTitle->userIsWatching() ) {
 1967+ $dbw->begin();
 1968+ $this->doWatch();
 1969+ $dbw->commit();
 1970+ }
 1971+ } else {
 1972+ if ( $this->mTitle->userIsWatching() ) {
 1973+ $dbw->begin();
 1974+ $this->doUnwatch();
 1975+ $dbw->commit();
 1976+ }
 1977+ }
 1978+ $this->doRedirect( $this->isRedirect( $text ) );
19591979 }
19601980
19611981 /**
19621982 * @deprecated use Article::doEdit()
19631983 */
19641984 function updateArticle( $text, $summary, $minor, $watchthis, $forceBot = false, $sectionanchor = '' ) {
1965 - wfDeprecated( __METHOD__ );
19661985 $flags = EDIT_UPDATE | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY |
19671986 ( $minor ? EDIT_MINOR : 0 ) |
19681987 ( $forceBot ? EDIT_FORCE_BOT : 0 );
19691988
1970 - $status = $this->doEdit( $text, $summary, $flags, false, null, $watchthis, false, $sectionanchor, true );
 1989+ $status = $this->doEdit( $text, $summary, $flags );
19711990
19721991 if ( !$status->isOK() ) {
19731992 return false;
19741993 }
19751994
 1995+ $dbw = wfGetDB( DB_MASTER );
 1996+ if ( $watchthis ) {
 1997+ if ( !$this->mTitle->userIsWatching() ) {
 1998+ $dbw->begin();
 1999+ $this->doWatch();
 2000+ $dbw->commit();
 2001+ }
 2002+ } else {
 2003+ if ( $this->mTitle->userIsWatching() ) {
 2004+ $dbw->begin();
 2005+ $this->doUnwatch();
 2006+ $dbw->commit();
 2007+ }
 2008+ }
 2009+
 2010+ $extraQuery = ''; // Give extensions a chance to modify URL query on update
 2011+ wfRunHooks( 'ArticleUpdateBeforeRedirect', array( $this, &$sectionanchor, &$extraQuery ) );
 2012+
 2013+ $this->doRedirect( $this->isRedirect( $text ), $sectionanchor, $extraQuery );
19762014 return true;
19772015 }
19782016
——@@ -2026,11 +2064,6 @@
20272065 *
20282066 * @param $baseRevId the revision ID this edit was based off, if any
20292067 * @param $user Optional user object, $wgUser will be used if not passed
2030 - * @param $watchthis Watch the page if true, unwatch the page if false, do nothing if null
2031 - * @param $comment Boolean: whether the edit is a new section
2032 - * @param $sectionanchor The section anchor for the page; used for redirecting the user back to the page
2033 - * after the edit is successfully committed
2034 - * @param $redirect If true, redirect the user back to the page after the edit is successfully committed
20352068 *
20362069 * @return Status object. Possible errors:
20372070 * edit-hook-aborted: The ArticleSave hook aborted the edit but didn't set the fatal flag of $status
——@@ -2047,8 +2080,7 @@
20482081 *
20492082 * Compatibility note: this function previously returned a boolean value indicating success/failure
20502083 */
2051 - public function doEdit( $text, $summary, $flags = 0, $baseRevId = false, $user = null , $watchthis = null,
2052 - $comment = false, $sectionanchor = '', $redirect = false) {
 2084+ public function doEdit( $text, $summary, $flags = 0, $baseRevId = false, $user = null ) {
20532085 global $wgUser, $wgDBtransactions, $wgUseAutomaticEditSummaries;
20542086
20552087 # Low-level sanity check
——@@ -2066,13 +2098,8 @@
20672099
20682100 $flags = $this->checkFlags( $flags );
20692101
2070 - # If this is a comment, add the summary as headline
2071 - if ( $comment && $summary != "" ) {
2072 - $text = wfMsgForContent( 'newsectionheaderdefaultlevel', $summary ) . "\n\n" . $text;
2073 - }
2074 -
20752102 if ( !wfRunHooks( 'ArticleSave', array( &$this, &$user, &$text, &$summary,
2076 - $flags & EDIT_MINOR, &$watchthis, null, &$flags, &$status) ) )
 2103+ $flags & EDIT_MINOR, null, null, &$flags, &$status ) ) )
20772104 {
20782105 wfDebug( __METHOD__ . ": ArticleSave hook aborted save!\n" );
20792106 wfProfileOut( __METHOD__ );
——@@ -2277,7 +2304,7 @@
22782305 Article::onArticleCreate( $this->mTitle );
22792306
22802307 wfRunHooks( 'ArticleInsertComplete', array( &$this, &$user, $text, $summary,
2281 - $flags & EDIT_MINOR, &$watchthis, null, &$flags, $revision ) );
 2308+ $flags & EDIT_MINOR, null, null, &$flags, $revision ) );
22822309 }
22832310
22842311 # Do updates right now unless deferral was requested
——@@ -2289,39 +2316,9 @@
22902317 $status->value['revision'] = $revision;
22912318
22922319 wfRunHooks( 'ArticleSaveComplete', array( &$this, &$user, $text, $summary,
2293 - $flags & EDIT_MINOR, &$watchthis, null, &$flags, $revision, &$status, $baseRevId,
2294 - &$redirect) );
 2320+ $flags & EDIT_MINOR, null, null, &$flags, $revision, &$status, $baseRevId ) );
22952321
2296 - # Watch or unwatch the page
2297 - if ( $watchthis === true ) {
2298 - if ( !$this->mTitle->userIsWatching() ) {
2299 - $dbw->begin();
2300 - $this->doWatch();
2301 - $dbw->commit();
2302 - }
2303 - } elseif ( $watchthis === false ) {
2304 - if ( $this->mTitle->userIsWatching() ) {
2305 - $dbw->begin();
2306 - $this->doUnwatch();
2307 - $dbw->commit();
2308 - }
2309 - }
2310 -
2311 - # Give extensions a chance to modify URL query on update
2312 - $extraQuery = '';
2313 -
2314 - wfRunHooks( 'ArticleUpdateBeforeRedirect', array( $this, &$sectionanchor, &$extraQuery ) );
2315 -
2316 - if ( $redirect ) {
2317 - if ( $sectionanchor || $extraQuery ) {
2318 - $this->doRedirect( $this->isRedirect( $text ), $sectionanchor, $extraQuery );
2319 - } else {
2320 - $this->doRedirect( $this->isRedirect( $text ) );
2321 - }
2322 - }
2323 -
23242322 wfProfileOut( __METHOD__ );
2325 -
23262323 return $status;
23272324 }
23282325
Index: trunk/phase3/includes/EditPage.php
——@@ -904,20 +904,11 @@
905905
906906 $isComment = ( $this->section == 'new' );
907907
908 - $flags = EDIT_NEW | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY |
909 - ( $this->minoredit ? EDIT_MINOR : 0 ) |
910 - ( $bot ? EDIT_FORCE_BOT : 0 );
911 - $status = $this->mArticle->doEdit( $this->textbox1, $this->summary, $flags,
912 - false, null, $this->watchthis, $isComment, '', true );
 908+ $this->mArticle->insertNewArticle( $this->textbox1, $this->summary,
 909+ $this->minoredit, $this->watchthis, false, $isComment, $bot );
913910
914 - if ( $status->isOK() ) {
915 - wfProfileOut( __METHOD__ );
916 - return self::AS_SUCCESS_NEW_ARTICLE;
917 - } else {
918 - $result = $status->getErrorsArray();
919 - }
920911 wfProfileOut( __METHOD__ );
921 - return self::AS_END;
 912+ return self::AS_SUCCESS_NEW_ARTICLE;
922913 }
923914
924915 # Article exists. Check for edit conflict.
——@@ -1059,20 +1050,14 @@
10601051 return self::AS_MAX_ARTICLE_SIZE_EXCEEDED;
10611052 }
10621053
1063 - // Update the article here
1064 - $flags = EDIT_UPDATE | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY |
1065 - ( $this->minoredit ? EDIT_MINOR : 0 ) |
1066 - ( $bot ? EDIT_FORCE_BOT : 0 );
1067 - $status = $this->mArticle->doEdit( $text, $this->summary, $flags,
1068 - false, null, $this->watchthis, false, $sectionanchor, true );
1069 -
1070 - if ( $status->isOK() )
 1054+ # update the article here
 1055+ if ( $this->mArticle->updateArticle( $text, $this->summary, $this->minoredit,
 1056+ $this->watchthis, $bot, $sectionanchor ) )
10711057 {
10721058 wfProfileOut( __METHOD__ );
10731059 return self::AS_SUCCESS_UPDATE;
10741060 } else {
10751061 $this->isConflict = true;
1076 - $result = $status->getErrorsArray();
10771062 }
10781063 wfProfileOut( __METHOD__ );
10791064 return self::AS_END;
Index: trunk/phase3/includes/api/ApiEditPage.php
——@@ -333,13 +333,12 @@
334334
335335 case EditPage::AS_END:
336336 // This usually means some kind of race condition
337 - // or DB weirdness occurred.
338 - if ( is_array( $result ) && count( $result ) > 0 ) {
339 - $this->dieUsageMsg( array( 'unknownerror', $result[0][0] ) );
340 - }
 337+ // or DB weirdness occurred. Fall through to throw an unknown
 338+ // error.
341339
342 - // Unknown error, but no specific error message
343 - // Fall through
 340+ // This needs fixing higher up, as Article::doEdit should be
 341+ // used rather than Article::updateArticle, so that specific
 342+ // error conditions can be returned
344343 default:
345344 $this->dieUsageMsg( array( 'unknownerror', $retval ) );
346345 }
Index: trunk/phase3/includes/Title.php
——@@ -2413,6 +2413,10 @@
24142414 * This clears some fields in this object, and clears any associated
24152415 * keys in the "bad links" section of the link cache.
24162416 *
 2417+ * - This is called from Article::insertNewArticle() to allow
 2418+ * loading of the new page_id. It's also called from
 2419+ * Article::doDeleteArticle()
 2420+ *
24172421 * @param $newid \type{\int} the new Article ID
24182422 */
24192423 public function resetArticleID( $newid ) {
Index: trunk/extensions/YouTubeAuthSub/YouTubeAuthSub_body.php
——@@ -56,8 +56,10 @@
5757 }
5858 }
5959 $content = "{{YoutubeVideo|{$wgRequest->getVal('id')}|{$title}|{$keywords}|{$description}|{$category}}}";
60 - $a->doEdit( $content, wfMsg('youtubeauthsub_summary'), EDIT_NEW
61 - | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY, false, null, false, false, '', true );
 60+ $a->insertNewArticle($content,
 61+ wfMsg('youtubeauthsub_summary'),
 62+ false,
 63+ false);
6264 $wgOut->redirect('');
6365 }
6466 $wgOut->addWikiText(wfMsg('youtubeauthsub_viewpage', $descTitle->getFullText()) );
Property changes on: trunk/extensions/DataTransclusion/tests/fetchWebData.php
___________________________________________________________________
Deleted: svn:mergeinfo
Property changes on: trunk/extensions/DataTransclusion/tests/fetchRecord.php
___________________________________________________________________
Deleted: svn:mergeinfo
Property changes on: trunk/extensions/DataTransclusion/XmlDataTransclusionSource.php
___________________________________________________________________
Deleted: svn:mergeinfo
Property changes on: trunk/extensions/DataTransclusion/OpenLibrarySource.php
___________________________________________________________________
Deleted: svn:mergeinfo
Index: trunk/extensions/SpamDiffTool/SpamDiffTool_body.php
——@@ -55,15 +55,13 @@
5656 if ( $wgUser->getID() > 0 )
5757 $watch = $wgUser->isWatched( $sb );
5858 if ( $insert ) {
59 - $a->doEdit( $text, wfMsgForContent( 'spamdifftool_summary' ), EDIT_NEW
60 - | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY, false, null, $watch, false, '', true );
 59+ $a->insertNewArticle( $text, wfMsgForContent( 'spamdifftool_summary' ), false, $watch );
6160 } else {
62 - $a->doEdit( $text, wfMsgForContent( 'spamdifftool_summary' ), EDIT_UPDATE
63 - | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY, false, null, $watch, false, '', true );
 61+ $a->updateArticle( $text, wfMsgForContent( 'spamdifftool_summary' ), false, $watch );
6462 }
6563 $returnto = $wgRequest->getVal( 'returnto' );
6664 if ( $returnto != null && $returnto != '' )
67 - $wgOut->redirect( $wgScript . "?" . urldecode( $returnto ) ); // clear the redirect set by doEdit
 65+ $wgOut->redirect( $wgScript . "?" . urldecode( $returnto ) ); // clear the redirect set by updateArticle
6866 return;
6967 }
7068 $vals = $wgRequest->getValues();
Property changes on: trunk/extensions/JS2Support/ResourceLoaderOutputPage.php
___________________________________________________________________
Deleted: svn:mergeinfo
Property changes on: trunk/extensions/JS2Support/mwResourceLoader.php
___________________________________________________________________
Deleted: svn:mergeinfo
Index: trunk/extensions/SpamBlacklist/cleanup.php
——@@ -35,7 +35,6 @@
3636 }
3737 $dbw = wfGetDB( DB_MASTER );
3838 $dbw->begin();
39 - $article = new Article( $title );
4039 if ( !$rev ) {
4140 // Didn't find a non-spammy revision, delete the page
4241 /*
——@@ -45,14 +44,14 @@
4645 */
4746 // Too scary, blank instead
4847 print "All revisions are spam, blanking...\n";
49 - $article->doEdit( '', "All revisions matched the spam blacklist ($match), blanking",
50 - EDIT_UPDATE | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY, false, null, false, false,
51 - '', true );
 48+ $article = new Article( $title );
 49+ $article->updateArticle( '', "All revisions matched the spam blacklist ($match), blanking",
 50+ false, false );
5251
5352 } else {
5453 // Revert to this revision
55 - $article->doEdit( $rev->getText(), "Cleaning up links to $match", EDIT_UPDATE
56 - | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY, false, null, false, false, '', true );
 54+ $article = new Article( $title );
 55+ $article->updateArticle( $rev->getText(), "Cleaning up links to $match", false, false );
5756 }
5857 $dbw->commit();
5958 wfDoUpdates();
Property changes on: trunk/extensions/Translate/groups/MantisBT/MantisBT.yml
___________________________________________________________________
Deleted: svn:mergeinfo
Index: trunk/extensions/Postcomment/Postcomment.php
——@@ -194,11 +194,9 @@
195195
196196 if ($update) {
197197 //echo "trying to update article";
198 - $article->doEdit( $text, '', EDIT_UPDATE | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY
199 - | EDIT_MINOR, false, null, $watch, false, '', true );
 198+ $article->updateArticle($text, "", true, $watch);
200199 } else {
201200 //echo "inserting new article";
202 - $article->doEdit( $text, $summary, EDIT_NEW | EDIT_DEFER_UPDATES | EDIT_AUTOSUMMARY
203 - | EDIT_MINOR | EDIT_FORCE_BOT, false, null, $watch, false, '', true );
 201+ $article->insertNewArticle($text, "", true, $watch, false, false, true);
204202 }
205203 }
Index: trunk/extensions/DynamicPageList/DPL.php
——@@ -1137,8 +1137,7 @@
11381138 $wgArticle = $articleX = new Article( $titleX );
11391139 $permission_errors = $articleX->mTitle->getUserPermissionsErrors( 'edit', $wgUser );
11401140 if ( count( $permission_errors ) == 0 ) {
1141 - $articleX->doEdit( $text, $summary, EDIT_UPDATE | EDIT_DEFER_UPDATES
1142 - | EDIT_AUTOSUMMARY, false, null, $titleX->userIsWatching(), false, '', true );
 1141+ $articleX->updateArticle( $text, $summary, false, $titleX->userIsWatching() );
11431142 return '';
11441143 } else {
11451144 $wgOut->showPermissionsErrorPage( $permission_errors );

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r14834Did some refactoring in Article.php:...tstarling09:50, 20 June 2006
r66878(bug 23641) refactor code around Article::doEdit(), and restored access to wa...happy-melon16:40, 25 May 2010
r67752Return some more verbose error messages when editing fails using the API. Add...btongminh19:52, 9 June 2010
r68606(bug 18891) Deprecated Article::insertNewArticle still used in coremgrabovsky13:25, 26 June 2010
r68608Follow-up r68606, per comments:...mgrabovsky19:52, 26 June 2010
r68609Follow-up r68606, per comments:...mgrabovsky19:52, 26 June 2010

Status & tagging log