From 3068742546e3d9912a24a8b474f1726e4b24fb23 Mon Sep 17 00:00:00 2001 From: Catrope Date: Sun, 13 May 2012 18:20:05 -0700 Subject: [PATCH] Expose the log_id of the deletion log entry in the action=delete API This entails some refactoring to actually surface the log_id all the way up: * Made doDeleteArticleReal() return a Status object rather than a constant, and put the log_id in $status->value. This Status object is also passed to the ArticleDelete hook. * Kept doDeleteArticle() the same for extension compatibility. * Switched all core callers of doDeleteArticle() to doDeleteArticleReal() and surfaced the error message from the Status if appropriate, rather than hardcoding 'cannotdelete' all over the place. * Exposed the log_id in ApiDelete * Add 'delete-hook-aborted' message for when a hook aborts the deletion but does not provide an error message. Previously this just caused the 'cannotdelete' message to appear. Change-Id: Ia6415b390d5d4172ce96667f46ccdba2be02461f --- docs/hooks.txt | 2 + includes/Article.php | 6 ++- includes/FileDeleteForm.php | 5 ++- includes/Title.php | 2 +- includes/WikiPage.php | 59 ++++++++++++--------------- includes/api/ApiDelete.php | 32 +++++++-------- includes/specials/SpecialMovepage.php | 5 ++- languages/messages/MessagesEn.php | 2 + languages/messages/MessagesQqq.php | 1 + maintenance/language/messages.inc | 1 + 10 files changed, 57 insertions(+), 58 deletions(-) diff --git a/docs/hooks.txt b/docs/hooks.txt index 33db1c53b4..c927db4654 100644 --- a/docs/hooks.txt +++ b/docs/hooks.txt @@ -422,6 +422,8 @@ $user: the user (object) deleting the article $reason: the reason (string) the article is being deleted $error: if the deletion was prohibited, the (raw HTML) error message to display (added in 1.13) +$status: Status object, modify this to throw an error. Overridden by $error + (added in 1.20) 'ArticleDeleteComplete': after an article is deleted $article: the WikiPage that was deleted diff --git a/includes/Article.php b/includes/Article.php index 6a3e41cb3c..d5324ec2de 100644 --- a/includes/Article.php +++ b/includes/Article.php @@ -1479,7 +1479,8 @@ class Article extends Page { public function doDelete( $reason, $suppress = false ) { $error = ''; $outputPage = $this->getContext()->getOutput(); - if ( $this->mPage->doDeleteArticle( $reason, $suppress, 0, true, $error ) ) { + $status = $this->mPage->doDeleteArticleReal( $reason, $suppress, 0, true, $error ); + if ( $status->isGood() ) { $deleted = $this->getTitle()->getPrefixedText(); $outputPage->setPageTitle( wfMessage( 'actioncomplete' ) ); @@ -1492,8 +1493,9 @@ class Article extends Page { } else { $outputPage->setPageTitle( wfMessage( 'cannotdelete-title', $this->getTitle()->getPrefixedText() ) ); if ( $error == '' ) { + $errors = $status->getErrorsArray(); $outputPage->wrapWikiMsg( "
\n$1\n
", - array( 'cannotdelete', wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) ) + $errors[0] ); $outputPage->addHTML( Xml::element( 'h2', null, LogPage::logName( 'delete' ) ) ); diff --git a/includes/FileDeleteForm.php b/includes/FileDeleteForm.php index d86c8d82f3..b794770310 100644 --- a/includes/FileDeleteForm.php +++ b/includes/FileDeleteForm.php @@ -149,7 +149,10 @@ class FileDeleteForm { try { // delete the associated article first $error = ''; - if ( $page->doDeleteArticleReal( $reason, $suppress, 0, false, $error, $user ) >= WikiPage::DELETE_SUCCESS ) { + $deleteStatus = $page->doDeleteArticleReal( $reason, $suppress, 0, false, $error, $user ); + // doDeleteArticleReal() returns a non-fatal error status if the page + // or revision is missing, so check for isOK() rather than isGood() + if ( $deleteStatus->isOK() ) { $status = $file->delete( $reason, $suppress ); if( $status->isOK() ) { $dbw->commit( __METHOD__ ); diff --git a/includes/Title.php b/includes/Title.php index 4f0a897433..6e3f16cb75 100644 --- a/includes/Title.php +++ b/includes/Title.php @@ -2866,7 +2866,7 @@ class Title { * * - This is called from WikiPage::doEdit() and WikiPage::insertOn() to allow * loading of the new page_id. It's also called from - * WikiPage::doDeleteArticle() + * WikiPage::doDeleteArticleReal() * * @param $newid Int the new Article ID */ diff --git a/includes/WikiPage.php b/includes/WikiPage.php index 8e9b0a0462..cd300debe8 100644 --- a/includes/WikiPage.php +++ b/includes/WikiPage.php @@ -34,30 +34,6 @@ abstract class Page {} * @internal documentation reviewed 15 Mar 2010 */ class WikiPage extends Page { - // doDeleteArticleReal() return values. Values less than zero indicate fatal errors, - // values greater than zero indicate that there were problems not resulting in page - // not being deleted - - /** - * Delete operation aborted by hook - */ - const DELETE_HOOK_ABORTED = -1; - - /** - * Deletion successful - */ - const DELETE_SUCCESS = 0; - - /** - * Page not found - */ - const DELETE_NO_PAGE = 1; - - /** - * No revisions found to delete - */ - const DELETE_NO_REVISIONS = 2; - // Constants for $mDataLoadedFrom and related /** @@ -2106,7 +2082,10 @@ class WikiPage extends Page { } /** - * Same as doDeleteArticleReal(), but returns more detailed success/failure status + * Same as doDeleteArticleReal(), but returns a simple boolean. This is kept around for + * backwards compatibility, if you care about error reporting you should use + * doDeleteArticleReal() instead. + * * Deletes the article with database consistency, writes logs, purges caches * * @param $reason string delete reason for deletion log @@ -2124,8 +2103,8 @@ class WikiPage extends Page { public function doDeleteArticle( $reason, $suppress = false, $id = 0, $commit = true, &$error = '', User $user = null ) { - return $this->doDeleteArticleReal( $reason, $suppress, $id, $commit, $error, $user ) - == WikiPage::DELETE_SUCCESS; + $status = $this->doDeleteArticleReal( $reason, $suppress, $id, $commit, $error, $user ); + return $status->isGood(); } /** @@ -2142,7 +2121,9 @@ class WikiPage extends Page { * @param $commit boolean defaults to true, triggers transaction end * @param &$error Array of errors to append to * @param $user User The deleting user - * @return int: One of WikiPage::DELETE_* constants + * @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 @@ -2151,20 +2132,28 @@ class WikiPage extends Page { wfDebug( __METHOD__ . "\n" ); + $status = Status::newGood(); + if ( $this->mTitle->getDBkey() === '' ) { - return WikiPage::DELETE_NO_PAGE; + $status->error( 'cannotdelete', wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) ); + return $status; } $user = is_null( $user ) ? $wgUser : $user; - if ( ! wfRunHooks( 'ArticleDelete', array( &$this, &$user, &$reason, &$error ) ) ) { - return WikiPage::DELETE_HOOK_ABORTED; + if ( ! wfRunHooks( 'ArticleDelete', array( &$this, &$user, &$reason, &$error, &$status ) ) ) { + if ( $status->isOK() ) { + // Hook aborted but didn't set a fatal status + $status->fatal( 'delete-hook-aborted' ); + } + return $status; } if ( $id == 0 ) { $this->loadPageData( 'forupdate' ); $id = $this->getID(); if ( $id == 0 ) { - return WikiPage::DELETE_NO_PAGE; + $status->error( 'cannotdelete', wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) ); + return $status; } } @@ -2222,7 +2211,8 @@ class WikiPage extends Page { if ( !$ok ) { $dbw->rollback( __METHOD__ ); - return WikiPage::DELETE_NO_REVISIONS; + $status->error( 'cannotdelete', wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) ); + return $status; } $this->doDeleteUpdates( $id ); @@ -2242,7 +2232,8 @@ class WikiPage extends Page { } wfRunHooks( 'ArticleDeleteComplete', array( &$this, &$user, $reason, $id ) ); - return WikiPage::DELETE_SUCCESS; + $status->value = $logid; + return $status; } /** diff --git a/includes/api/ApiDelete.php b/includes/api/ApiDelete.php index cefdaac783..72ddc37844 100644 --- a/includes/api/ApiDelete.php +++ b/includes/api/ApiDelete.php @@ -56,13 +56,14 @@ class ApiDelete extends ApiBase { $user = $this->getUser(); if ( $titleObj->getNamespace() == NS_FILE ) { - $retval = self::deleteFile( $pageObj, $user, $params['token'], $params['oldimage'], $reason, false ); + $status = self::deleteFile( $pageObj, $user, $params['token'], $params['oldimage'], $reason, false ); } else { - $retval = self::delete( $pageObj, $user, $params['token'], $reason ); + $status = self::delete( $pageObj, $user, $params['token'], $reason ); } - if ( count( $retval ) ) { - $this->dieUsageMsg( reset( $retval ) ); // We don't care about multiple errors, just report one of them + if ( !$status->isGood() ) { + $errors = $this->getErrorsArray(); + $this->dieUsageMsg( $errors[0] ); // We don't care about multiple errors, just report one of them } // Deprecated parameters @@ -75,7 +76,11 @@ class ApiDelete extends ApiBase { } $this->setWatch( $watch, $titleObj, 'watchdeletion' ); - $r = array( 'title' => $titleObj->getPrefixedText(), 'reason' => $reason ); + $r = array( + 'title' => $titleObj->getPrefixedText(), + 'reason' => $reason, + 'logid' => $status->value + ); $this->getResult()->addValue( null, $this->getModuleName(), $r ); } @@ -97,7 +102,7 @@ class ApiDelete extends ApiBase { * @param $user User doing the action * @param $token String: delete token (same as edit token) * @param $reason String: reason for the deletion. Autogenerated if NULL - * @return Title::getUserPermissionsErrors()-like array + * @return Status */ public static function delete( Page $page, User $user, $token, &$reason = null ) { $title = $page->getTitle(); @@ -119,11 +124,7 @@ class ApiDelete extends ApiBase { $error = ''; // Luckily, Article.php provides a reusable delete function that does the hard work for us - if ( $page->doDeleteArticle( $reason, false, 0, true, $error ) ) { - return array(); - } else { - return array( array( 'cannotdelete', $title->getPrefixedText() ) ); - } + return $page->doDeleteArticleReal( $reason, false, 0, true, $error ); } /** @@ -133,7 +134,7 @@ class ApiDelete extends ApiBase { * @param $oldimage * @param $reason * @param $suppress bool - * @return array|Title + * @return Status */ public static function deleteFile( Page $page, User $user, $token, $oldimage, &$reason = null, $suppress = false ) { $title = $page->getTitle(); @@ -162,12 +163,7 @@ class ApiDelete extends ApiBase { if ( is_null( $reason ) ) { // Log and RC don't like null reasons $reason = ''; } - $status = FileDeleteForm::doDelete( $title, $file, $oldimage, $reason, $suppress ); - if ( !$status->isGood() ) { - return array( array( 'cannotdelete', $title->getPrefixedText() ) ); - } - - return array(); + return FileDeleteForm::doDelete( $title, $file, $oldimage, $reason, $suppress ); } public function mustBePosted() { diff --git a/includes/specials/SpecialMovepage.php b/includes/specials/SpecialMovepage.php index 6b817d2804..f7d13bb2d2 100644 --- a/includes/specials/SpecialMovepage.php +++ b/includes/specials/SpecialMovepage.php @@ -429,8 +429,9 @@ class MovePageForm extends UnlistedSpecialPage { $error = ''; // passed by ref $page = WikiPage::factory( $nt ); - if ( !$page->doDeleteArticle( $reason, false, 0, true, $error, $user ) ) { - $this->showForm( array( array( 'cannotdelete', wfEscapeWikiText( $nt->getPrefixedText() ) ) ) ); + $deleteStatus = $page->doDeleteArticleReal( $reason, false, 0, true, $error, $user ); + if ( !$deleteStatus->isGood() ) { + $this->showForm( $deleteStatus->getErrorsArray() ); return; } } diff --git a/languages/messages/MessagesEn.php b/languages/messages/MessagesEn.php index c13c9da623..f1c14d085c 100644 --- a/languages/messages/MessagesEn.php +++ b/languages/messages/MessagesEn.php @@ -1005,6 +1005,8 @@ Please report this to an [[Special:ListUsers/sysop|administrator]], making note 'cannotdelete' => 'The page or file "$1" could not be deleted. It may have already been deleted by someone else.', 'cannotdelete-title' => 'Cannot delete page "$1"', +'delete-hook-aborted' => 'Deletion aborted by hook. +It gave no explanation.', 'badtitle' => 'Bad title', 'badtitletext' => 'The requested page title was invalid, empty, or an incorrectly linked inter-language or inter-wiki title. It may contain one or more characters which cannot be used in titles.', diff --git a/languages/messages/MessagesQqq.php b/languages/messages/MessagesQqq.php index eb85c53a2d..651343be72 100644 --- a/languages/messages/MessagesQqq.php +++ b/languages/messages/MessagesQqq.php @@ -659,6 +659,7 @@ HTML markup cannot be used. $1 is a filename, I think.', 'cannotdelete-title' => 'Title of error page when the user cannot delete a page * $1 is the page name', +'delete-hook-aborted' => 'Error message shown when an extension hook prevents a page deletion, but does not provide an error message.', 'badtitle' => 'The page title when a user requested a page with invalid page name. The content will be {{msg-mw|badtitletext}}.', 'badtitletext' => 'The message shown when a user requested a page with invalid page name. The page title will be {{msg-mw|badtitle}}.', 'perfcached' => 'Like {{msg-mw|perfcachedts}} but used when we do not know how long ago page was cached (unlikely to happen). Parameters: diff --git a/maintenance/language/messages.inc b/maintenance/language/messages.inc index c55b83d64f..2ad558d37a 100644 --- a/maintenance/language/messages.inc +++ b/maintenance/language/messages.inc @@ -386,6 +386,7 @@ $wgMessageStructure = array( 'badarticleerror', 'cannotdelete', 'cannotdelete-title', + 'delete-hook-aborted', 'badtitle', 'badtitletext', 'perfcached', -- 2.20.1