Deprecate WikiPage methods replaced by DeletePage

Also remove the "@unstable" annotation from DeletePage. Methods without known usages were hard-deprecated, the others soft-deprecated. Bug: T288758 Bug: T288759 Change-Id: I30c62572fd533526779a8ade3ab178f35bebb522
2021-08-23 17:38:52 +02:00 · 2021-08-23 17:38:52 +02:00 · a8200aa5a8
commit a8200aa5a8
parent 377342de86
6 changed files with 134 additions and 80 deletions
--- a/RELEASE-NOTES-1.37
+++ b/RELEASE-NOTES-1.37
@ -730,6 +730,16 @@ because of Phabricator reports.
  ::whereUserNamePrefix().
 * Manually constructing a MovePage object, deprecated in 1.34, now emits
  deprecation warnings. Use MovePageFactory instead.
+* The following deletion-related methods were deprecated:
+  - WikiPage::doDeleteArticleReal() (soft) - use DeletePage
+  - WikiPage::doDeleteArticleBatched() (soft) - no replacement
+  - WikiPage::isBatchedDelete() (soft) - use DeletePage
+  - WikiPage::doDeleteUpdates() (hard) - no replacement
+  - WikiPage::getDeletionUpdates() (hard) - no replacement
+  - Title::isBigDeletion (soft) - no replacement
+* Relying on PermissionManager or Authority to check for big deletions
+  was deprecated. This is now automatically checked if you use
+  DeletePage::deleteIfAllowed(). (T288759)
 * The userCan hook now emits deprecation warnings. Use the
  getUserPermissionsErrors or getUserPermissionsErrorsExpensive hooks instead.
 * Parser::$mUser public access, and the methods ParserOptions::getUser() and
--- a/includes/Permissions/PermissionManager.php
+++ b/includes/Permissions/PermissionManager.php
@ -1130,6 +1130,7 @@ class PermissionManager {
 				&& !$this->userCan( 'bigdelete', $user, $title )
 				&& $title->isBigDeletion()
 			) {
+				// NOTE: This check is deprecated since 1.37, see T288759
 				$errors[] = [
 					'delete-toobig',
 					$wgLang->formatNum( $this->options->get( 'DeleteRevisionsLimit' ) )
--- a/includes/Title.php
+++ b/includes/Title.php
@ -3426,6 +3426,7 @@ class Title implements LinkTarget, PageIdentity, IDBAccessObject {

 	/**
 	 * Check whether the number of revisions of this page surpasses $wgDeleteRevisionsLimit
+	 * @deprecated since 1.37 External callers shouldn't need to know about this.
 	 *
 	 * @return bool
 	 */
--- a/includes/page/DeletePage.php
+++ b/includes/page/DeletePage.php
@ -27,6 +27,7 @@ use MediaWiki\Revision\RevisionStore;
 use MediaWiki\Revision\SlotRecord;
 use MediaWiki\User\UserFactory;
 use Message;
+use RawMessage;
 use ResourceLoaderWikiModule;
 use SearchUpdate;
 use SiteStatsUpdate;
@ -40,7 +41,6 @@ use WikiPage;
 /**
 * @since 1.37
 * @package MediaWiki\Page
- * @unstable
 */
 class DeletePage {
 	/**
@ -94,6 +94,8 @@ class DeletePage {

 	/** @var string|array */
 	private $legacyHookErrors = '';
+	/** @var bool */
+	private $mergeLegacyHookErrors = true;

 	/** @var BacklinkCacheFactory */
 	private $backlinkCacheFactory;
@ -156,6 +158,15 @@ class DeletePage {
 		return $this->legacyHookErrors;
 	}

+	/**
+	 * @internal BC method for use by WikiPage::doDeleteArticleReal only.
+	 * @return self
+	 */
+	public function keepLegacyHookErrorsSeparate(): self {
+		$this->mergeLegacyHookErrors = false;
+		return $this;
+	}
+
 	/**
 	 * If true, suppress all revisions and log the deletion in the suppression log instead of
 	 * the deletion log.
@ -243,6 +254,9 @@ class DeletePage {
 		return $status;
 	}

+	/**
+	 * @return bool
+	 */
 	private function isBigDeletion(): bool {
 		$revLimit = $this->options->get( 'DeleteRevisionsLimit' );
 		if ( !$revLimit ) {
@ -297,6 +311,14 @@ class DeletePage {
 		if ( !$this->hookRunner->onArticleDelete(
 			$this->page, $legacyDeleter, $reason, $this->legacyHookErrors, $status, $this->suppress )
 		) {
+			if ( $this->mergeLegacyHookErrors ) {
+				if ( is_string( $this->legacyHookErrors ) ) {
+					$this->legacyHookErrors = [ $this->legacyHookErrors ];
+				}
+				foreach ( $this->legacyHookErrors as $legacyError ) {
+					$status->fatal( new RawMessage( $legacyError ) );
+				}
+			}
 			if ( $status->isOK() ) {
 				// Hook aborted but didn't set a fatal status
 				$status->fatal( 'delete-hook-aborted' );
@ -304,6 +326,8 @@ class DeletePage {
 			return $status;
 		}

+		// Use a new Status in case a hook handler put something here without aborting.
+		$status = Status::newGood();
 		$hookRes = $this->hookRunner->onPageDelete( $this->page, $this->deleter, $reason, $status, $this->suppress );
 		if ( !$hookRes && !$status->isGood() ) {
 			// Note: as per the PageDeleteHook documentation, `return false` is ignored if $status is good.
@ -314,7 +338,7 @@ class DeletePage {
 	}

 	/**
-	 * @private Public for BC only
+	 * @internal The only external caller allowed is DeletePageJob.
 	 * Back-end article deletion
 	 *
 	 * Only invokes batching via the job queue if necessary per DeleteRevisionsBatchSize.
@ -392,7 +416,6 @@ class DeletePage {
 			$dbw->startAtomic( __METHOD__ );
 		}

-		// If done archiving, also delete the article.
 		if ( !$done ) {
 			$dbw->endAtomic( __METHOD__ );

@ -411,85 +434,86 @@ class DeletePage {
 			$job = new DeletePageJob( $jobParams );
 			$this->jobQueueGroup->push( $job );
 			$status->value = false;
-		} else {
-			// Get archivedRevisionCount by db query, because there's no better alternative.
-			// Jobs cannot pass a count of archived revisions to the next job, because additional
-			// deletion operations can be started while the first is running.  Jobs from each
-			// gracefully interleave, but would not know about each other's count.  Deduplication
-			// in the job queue to avoid simultaneous deletion operations would add overhead.
-			// Number of archived revisions cannot be known beforehand, because edits can be made
-			// while deletion operations are being processed, changing the number of archivals.
-			$archivedRevisionCount = $dbw->selectRowCount(
-				'archive',
-				'*',
-				[
-					'ar_namespace' => $title->getNamespace(),
-					'ar_title' => $title->getDBkey(),
-					'ar_page_id' => $id
-				], __METHOD__
-			);
-
-			// Clone the title and wikiPage, so we have the information we need when
-			// we log and run the ArticleDeleteComplete hook.
-			$logTitle = clone $title;
-			$wikiPageBeforeDelete = clone $this->page;
-
-			// Now that it's safely backed up, delete it
-			$dbw->delete( 'page', [ 'page_id' => $id ], __METHOD__ );
-
-			// Log the deletion, if the page was suppressed, put it in the suppression log instead
-			$logtype = $this->suppress ? 'suppress' : 'delete';
-
-			$logEntry = new ManualLogEntry( $logtype, $this->logSubtype );
-			$logEntry->setPerformer( $this->deleter->getUser() );
-			$logEntry->setTarget( $logTitle );
-			$logEntry->setComment( $reason );
-			$logEntry->addTags( $this->tags );
-			if ( !$this->isDeletePageUnitTest ) {
-				// TODO: Remove conditional once ManualLogEntry is servicified (T253717)
-				$logid = $logEntry->insert();
-
-				$dbw->onTransactionPreCommitOrIdle(
-					static function () use ( $logEntry, $logid ) {
-						// T58776: avoid deadlocks (especially from FileDeleteForm)
-						$logEntry->publish( $logid );
-					},
-					__METHOD__
-				);
-			} else {
-				$logid = 42;
-			}
-
-			$dbw->endAtomic( __METHOD__ );
-
-			$this->doDeleteUpdates( $revisionRecord );
-
-			$legacyDeleter = $this->userFactory->newFromAuthority( $this->deleter );
-			$this->hookRunner->onArticleDeleteComplete(
-				$wikiPageBeforeDelete,
-				$legacyDeleter,
-				$reason,
-				$id,
-				$content,
-				$logEntry,
-				$archivedRevisionCount
-			);
-			$this->hookRunner->onPageDeleteComplete(
-				$wikiPageBeforeDelete,
-				$this->deleter,
-				$reason,
-				$id,
-				$revisionRecord,
-				$logEntry,
-				$archivedRevisionCount
-			);
-			$status->value = $logid;
-
-			// Show log excerpt on 404 pages rather than just a link
-			$key = $this->recentDeletesCache->makeKey( 'page-recent-delete', md5( $logTitle->getPrefixedText() ) );
-			$this->recentDeletesCache->set( $key, 1, BagOStuff::TTL_DAY );
+			return $status;
 		}

+		// Get archivedRevisionCount by db query, because there's no better alternative.
+		// Jobs cannot pass a count of archived revisions to the next job, because additional
+		// deletion operations can be started while the first is running.  Jobs from each
+		// gracefully interleave, but would not know about each other's count.  Deduplication
+		// in the job queue to avoid simultaneous deletion operations would add overhead.
+		// Number of archived revisions cannot be known beforehand, because edits can be made
+		// while deletion operations are being processed, changing the number of archivals.
+		$archivedRevisionCount = $dbw->selectRowCount(
+			'archive',
+			'*',
+			[
+				'ar_namespace' => $title->getNamespace(),
+				'ar_title' => $title->getDBkey(),
+				'ar_page_id' => $id
+			], __METHOD__
+		);
+
+		// Clone the title and wikiPage, so we have the information we need when
+		// we log and run the ArticleDeleteComplete hook.
+		$logTitle = clone $title;
+		$wikiPageBeforeDelete = clone $this->page;
+
+		// Now that it's safely backed up, delete it
+		$dbw->delete( 'page', [ 'page_id' => $id ], __METHOD__ );
+
+		// Log the deletion, if the page was suppressed, put it in the suppression log instead
+		$logtype = $this->suppress ? 'suppress' : 'delete';
+
+		$logEntry = new ManualLogEntry( $logtype, $this->logSubtype );
+		$logEntry->setPerformer( $this->deleter->getUser() );
+		$logEntry->setTarget( $logTitle );
+		$logEntry->setComment( $reason );
+		$logEntry->addTags( $this->tags );
+		if ( !$this->isDeletePageUnitTest ) {
+			// TODO: Remove conditional once ManualLogEntry is servicified (T253717)
+			$logid = $logEntry->insert();
+
+			$dbw->onTransactionPreCommitOrIdle(
+				static function () use ( $logEntry, $logid ) {
+					// T58776: avoid deadlocks (especially from FileDeleteForm)
+					$logEntry->publish( $logid );
+				},
+				__METHOD__
+			);
+		} else {
+			$logid = 42;
+		}
+
+		$dbw->endAtomic( __METHOD__ );
+
+		$this->doDeleteUpdates( $revisionRecord );
+
+		$legacyDeleter = $this->userFactory->newFromAuthority( $this->deleter );
+		$this->hookRunner->onArticleDeleteComplete(
+			$wikiPageBeforeDelete,
+			$legacyDeleter,
+			$reason,
+			$id,
+			$content,
+			$logEntry,
+			$archivedRevisionCount
+		);
+		$this->hookRunner->onPageDeleteComplete(
+			$wikiPageBeforeDelete,
+			$this->deleter,
+			$reason,
+			$id,
+			$revisionRecord,
+			$logEntry,
+			$archivedRevisionCount
+		);
+		$status->value = $logid;
+
+		// Show log excerpt on 404 pages rather than just a link
+		$key = $this->recentDeletesCache->makeKey( 'page-recent-delete', md5( $logTitle->getPrefixedText() ) );
+		$this->recentDeletesCache->set( $key, 1, BagOStuff::TTL_DAY );
+
 		return $status;
 	}

--- a/includes/page/WikiPage.php
+++ b/includes/page/WikiPage.php
@ -2634,6 +2634,8 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 	 * return value.  Callers must decide for themselves how to deal with this.  $safetyMargin
 	 * is provided as an unreliable but situationally useful help for some common cases.
 	 *
+	 * @deprecated since 1.37 Use DeletePage::isBatchedDelete instead.
+	 *
 	 * @param int $safetyMargin Added to the revision count when checking for batching
 	 * @return bool True if deletion would be batched, false otherwise
 	 */
@ -2655,6 +2657,11 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 	 * @since 1.35 Signature changed, user moved to second parameter to prepare for requiring
 	 *             a user to be passed
 	 * @since 1.36 User second parameter is required
+	 * @deprecated since 1.37 Use DeletePage instead. Calling ::deleteIfAllowed and letting DeletePage handle
+	 * permission checks is preferred over doing permission checks yourself and then calling ::deleteUnsafe.
+	 * Note that DeletePage returns a good status with false value in case of scheduled deletion, instead of
+	 * a status with a warning. Also, the new method doesn't have an $error parameter, since any error is
+	 * added to the returned Status.
 	 *
 	 * @param string $reason Delete reason for deletion log
 	 * @param UserIdentity $deleter The deleting user
@ -2687,6 +2694,7 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 			->setTags( $tags ?: [] )
 			->setLogSubtype( $logsubtype )
 			->forceImmediate( $immediate )
+			->keepLegacyHookErrorsSeparate()
 			->deleteUnsafe( $reason );
 		$error = $deletePage->getLegacyHookErrors();
 		if ( $status->isGood() && $status->value === false ) {
@ -2704,6 +2712,8 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 	 * Deletions can often be completed inline without involving the job queue.
 	 *
 	 * Potentially called many times per deletion operation for pages with many revisions.
+	 * @deprecated since 1.37 No external caller besides DeletePageJob should use this.
+	 *
 	 * @param string $reason
 	 * @param bool $suppress
 	 * @param UserIdentity $deleter
@ -2762,6 +2772,8 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 	/**
 	 * Do some database updates after deletion
 	 *
+	 * @deprecated since 1.37 With no replacement.
+	 *
 	 * @param int $id The page_id value of the page being deleted
 	 * @param Content|null $content Page content to be used when determining
 	 *   the required updates. This may be needed because $this->getContent()
@ -2777,6 +2789,7 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 		RevisionRecord $revRecord = null,
 		UserIdentity $user = null
 	) {
+		wfDeprecated( __METHOD__, '1.37' );
 		if ( !$revRecord ) {
 			throw new BadMethodCallException( __METHOD__ . ' now requires a RevisionRecord' );
 		}
@ -3193,11 +3206,14 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 	 * updates should remove any information about this page from secondary data
 	 * stores such as links tables.
 	 *
+	 * @deprecated since 1.37 With no replacement.
+	 *
 	 * @param RevisionRecord|Content|null $rev The revision being deleted. Also accepts a Content
 	 *       object for backwards compatibility.
 	 * @return DeferrableUpdate[]
 	 */
 	public function getDeletionUpdates( $rev = null ) {
+		wfDeprecated( __METHOD__, '1.37' );
 		$user = new UserIdentityValue( 0, 'Legacy code hater' );
 		$services = MediaWikiServices::getInstance();
 		$deletePage = $services->getDeletePageFactory()->newDeletePage(
--- a/tests/phpunit/includes/page/WikiPageDbTest.php
+++ b/tests/phpunit/includes/page/WikiPageDbTest.php
@ -700,6 +700,7 @@ class WikiPageDbTest extends MediaWikiLangTestCase {
 	 * @covers WikiPage::doDeleteUpdates
 	 */
 	public function testDoDeleteUpdates() {
+		$this->hideDeprecated( 'WikiPage::doDeleteUpdates' );
 		$user = $this->getTestUser()->getUserIdentity();
 		$page = $this->createPage(
 			__METHOD__,
@ -789,6 +790,7 @@ class WikiPageDbTest extends MediaWikiLangTestCase {
 	}

 	public function testGetDeletionUpdates() {
+		$this->hideDeprecated( 'WikiPage::getDeletionUpdates' );
 		$m1 = $this->defineMockContentModelForUpdateTesting( 'M1' );

 		$mainContent1 = $this->createMockContent( $m1, 'main 1' );