From d24641a2a0aab6ee83063454ff458a67775a753c Mon Sep 17 00:00:00 2001 From: Daimona Eaytoy Date: Mon, 30 Aug 2021 20:31:39 +0200 Subject: [PATCH] Change return format of UndeletePage::undelete() - Make it consistently return a StatusValue, as is expected by this kind of methods. - Omit the comment in the returned status, as it matches the one that was passed as method parameter to undelete(). - Return a good status if there was nothing to undelete. Callers can check the status value to determine if something was deleted. The new format will make getRevisionStatus and getFileStatus unnecessary, and will also make it impossible to distinguish between file-related and revision-related errors. I don't think this is really important (only SpecialUndelete uses the respective methods). PageArchive is responsible for maintaining backwards compatibility with the old format. Bug: T290021 Change-Id: I51179fa7707b5fc44d2257cc927edc0bd798a4d9 --- includes/page/PageArchive.php | 14 ++++++++++++- includes/page/UndeletePage.php | 36 ++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/includes/page/PageArchive.php b/includes/page/PageArchive.php index 37cb8decdd4..baa63a63cf2 100644 --- a/includes/page/PageArchive.php +++ b/includes/page/PageArchive.php @@ -411,7 +411,19 @@ class PageArchive { $page = MediaWikiServices::getInstance()->getWikiPageFactory()->newFromTitle( $this->title ); $user = MediaWikiServices::getInstance()->getUserFactory()->newFromUserIdentity( $user ); $up = new UndeletePage( $page, $user ); - $ret = $up->undelete( $timestamps, $comment, $fileVersions, $unsuppress, $tags ); + $status = $up->undelete( $timestamps, $comment, $fileVersions, $unsuppress, $tags ); + // BC with old return format + if ( $status->isGood() ) { + $restoredRevs = $status->getValue()[UndeletePage::REVISIONS_RESTORED]; + $restoredFiles = $status->getValue()[UndeletePage::FILES_RESTORED]; + if ( $restoredRevs === 0 && $restoredFiles === 0 ) { + $ret = false; + } else { + $ret = [ $restoredRevs, $restoredFiles, $comment ]; + } + } else { + $ret = false; + } $this->fileStatus = $up->getFileStatus(); $this->revisionStatus = $up->getRevisionStatus(); return $ret; diff --git a/includes/page/UndeletePage.php b/includes/page/UndeletePage.php index 443bf32dd6f..6c3d319a72b 100644 --- a/includes/page/UndeletePage.php +++ b/includes/page/UndeletePage.php @@ -37,6 +37,7 @@ use ReadOnlyError; use ReadOnlyMode; use RepoGroup; use Status; +use StatusValue; use Wikimedia\Rdbms\IDatabase; use Wikimedia\Rdbms\ILoadBalancer; use WikiPage; @@ -47,6 +48,11 @@ use WikiPage; * @unstable */ class UndeletePage { + + // Constants used as keys in the StatusValue returned by undelete() + public const FILES_RESTORED = 'files'; + public const REVISIONS_RESTORED = 'revs'; + /** @var HookRunner */ private $hookRunner; /** @var JobQueueGroup */ @@ -72,7 +78,7 @@ class UndeletePage { private $performer; /** @var Status|null */ private $fileStatus; - /** @var Status|null */ + /** @var StatusValue|null */ private $revisionStatus; /** @@ -111,8 +117,12 @@ class UndeletePage { * @param bool $unsuppress * @param string|string[]|null $tags Change tags to add to log entry * ($user should be able to add the specified tags before this is called) - * @return array|bool [ number of file revisions restored, number of image revisions - * restored, log message ] on success, false on failure. + * @return StatusValue Good Status with the following value on success: + * [ + * self::REVISIONS_RESTORED => number of text revisions restored, + * self::FILES_RESTORED => number of file revisions restored + * ] + * Fatal Status on failure. */ public function undelete( $timestamps, @@ -120,7 +130,7 @@ class UndeletePage { $fileVersions = [], $unsuppress = false, $tags = null - ) { + ): StatusValue { // If both the set of text revisions and file revisions are empty, // restore everything. Otherwise, just restore the requested items. $restoreAll = empty( $timestamps ) && empty( $fileVersions ); @@ -134,7 +144,7 @@ class UndeletePage { $img->load( File::READ_LATEST ); $this->fileStatus = $img->restore( $fileVersions, $unsuppress ); if ( !$this->fileStatus->isOK() ) { - return false; + return $this->fileStatus; } $filesRestored = $this->fileStatus->successCount; } else { @@ -144,7 +154,7 @@ class UndeletePage { if ( $restoreText ) { $this->revisionStatus = $this->undeleteRevisions( $timestamps, $unsuppress, $comment ); if ( !$this->revisionStatus->isOK() ) { - return false; + return $this->revisionStatus; } $textRestored = $this->revisionStatus->getValue(); @@ -154,8 +164,7 @@ class UndeletePage { if ( !$textRestored && !$filesRestored ) { $this->logger->debug( "Undelete: nothing undeleted..." ); - - return false; + return StatusValue::newGood( [ self::REVISIONS_RESTORED => 0, self::FILES_RESTORED => 0 ] ); } $logEntry = new ManualLogEntry( 'delete', 'restore' ); @@ -173,7 +182,10 @@ class UndeletePage { $logid = $logEntry->insert(); $logEntry->publish( $logid ); - return [ $textRestored, $filesRestored, $comment ]; + return StatusValue::newGood( [ + self::REVISIONS_RESTORED => $textRestored, + self::FILES_RESTORED => $filesRestored + ] ); } /** @@ -185,7 +197,7 @@ class UndeletePage { * @param bool $unsuppress Remove all ar_deleted/fa_deleted restrictions of seletected revs * @param string $comment * @throws ReadOnlyError - * @return Status Status object containing the number of revisions restored on success + * @return StatusValue Status object containing the number of revisions restored on success */ private function undeleteRevisions( $timestamps, $unsuppress = false, $comment = '' ) { if ( $this->readOnlyMode->isReadOnly() ) { @@ -466,9 +478,9 @@ class UndeletePage { } /** - * @return Status|null + * @return StatusValue|null */ - public function getRevisionStatus() { + public function getRevisionStatus(): ?StatusValue { return $this->revisionStatus; } }