Merge "Change return format of DeletePage entrypoints"

includes/api/ApiDelete.php

@@ -135,7 +135,8 @@ class ApiDelete extends ApiBase {
 			$r['scheduled'] = true;
 		} else {
 			// Scheduled deletions don't currently have a log entry available at this point
-			$r['logid'] = $status->value;
+			// @phan-suppress-next-line PhanTypeArraySuspiciousNullable
+			$r['logid'] = $status->value[0];
 		}
 		$this->getResult()->addValue( null, $this->getModuleName(), $r );
 	}
@@ -146,7 +147,9 @@ class ApiDelete extends ApiBase {
 	 * @param WikiPage $page WikiPage object to work on
 	 * @param string|null &$reason Reason for the deletion. Autogenerated if null
 	 * @param string[] $tags Tags to tag the deletion with
-	 * @return StatusValue
+	 * @return StatusValue Same as DeletePage::deleteIfAllowed, but if the status is good, then:
+	 *  - For immediate deletions, the value is an array of IDs of the deletion log entries
+	 *  - For scheduled deletions, the value is false
 	 */
 	private function delete( WikiPage $page, &$reason = null, array $tags = [] ): StatusValue {
 		$title = $page->getTitle();
@@ -164,7 +167,9 @@ class ApiDelete extends ApiBase {
 		}
 
 		$deletePage = $this->deletePageFactory->newDeletePage( $page, $this->getAuthority() );
-		return $deletePage->setTags( $tags )->deleteIfAllowed( $reason );
+		$deletionStatus = $deletePage->setTags( $tags )->deleteIfAllowed( $reason );
+		$deletionStatus->value = $deletePage->deletionWasScheduled() ? false : $deletePage->getSuccessfulDeletionsIDs();
+		return $deletionStatus;
 	}
 
 	/**
@@ -219,7 +224,12 @@ class ApiDelete extends ApiBase {
 			$reason = '';
 		}
 
-		return FileDeleteForm::doDelete( $title, $file, $oldimage, $reason, $suppress, $this->getUser(), $tags );
+		$ret = FileDeleteForm::doDelete( $title, $file, $oldimage, $reason, $suppress, $this->getUser(), $tags );
+		if ( $ret->isOK() && is_int( $ret->value ) ) {
+			// DeletePage (and consequently self::delete()) returns an array of IDs, so do the same here.
+			$ret->value = [ $ret->value ];
+		}
+		return $ret;
 	}
 
 	public function mustBePosted() {

includes/page/DeletePage.php

@@ -75,6 +75,8 @@ class DeletePage {
 	private $webRequestID;
 	/** @var UserFactory */
 	private $userFactory;
+	/** @var BacklinkCacheFactory */
+	private $backlinkCacheFactory;
 
 	/** @var bool */
 	private $isDeletePageUnitTest = false;
@@ -98,8 +100,12 @@ class DeletePage {
 	/** @var bool */
 	private $mergeLegacyHookErrors = true;
 
-	/** @var BacklinkCacheFactory */
-	private $backlinkCacheFactory;
+	/** @var int[]|null */
+	private $successfulDeletionsIDs;
+	/** @var bool|null */
+	private $wasScheduled;
+	/** @var bool Whether a deletion was attempted */
+	private $attemptedDeletion = false;
 
 	/**
 	 * @param HookContainer $hookContainer
@@ -225,6 +231,43 @@ class DeletePage {
 		$this->isDeletePageUnitTest = $test;
 	}
 
+	/**
+	 * Called before attempting a deletion, allows the result getters to be used
+	 */
+	private function setDeletionAttempted(): void {
+		$this->attemptedDeletion = true;
+		$this->successfulDeletionsIDs = [];
+		$this->wasScheduled = false;
+	}
+
+	/**
+	 * Asserts that a deletion operation was attempted
+	 * @throws BadMethodCallException
+	 */
+	private function assertDeletionAttempted(): void {
+		if ( !$this->attemptedDeletion ) {
+			throw new BadMethodCallException( 'No deletion was attempted' );
+		}
+	}
+
+	/**
+	 * @return int[] Array of log IDs of successful deletions
+	 * @throws BadMethodCallException If no deletions were attempted
+	 */
+	public function getSuccessfulDeletionsIDs(): array {
+		$this->assertDeletionAttempted();
+		return $this->successfulDeletionsIDs;
+	}
+
+	/**
+	 * @return bool Whether (part of) the deletion was scheduled
+	 * @throws BadMethodCallException If no deletions were attempted
+	 */
+	public function deletionWasScheduled(): bool {
+		$this->assertDeletionAttempted();
+		return $this->wasScheduled;
+	}
+
 	/**
 	 * Same as deleteUnsafe, but checks permissions.
 	 *
@@ -232,6 +275,7 @@ class DeletePage {
 	 * @return StatusValue
 	 */
 	public function deleteIfAllowed( string $reason ): StatusValue {
+		$this->setDeletionAttempted();
 		$status = $this->authorizeDeletion();
 		if ( !$status->isGood() ) {
 			return $status;
@@ -303,12 +347,12 @@ class DeletePage {
 	 *
 	 * @param string $reason Delete reason for deletion log
 	 * @return Status Status object:
-	 *   - If immediate and successful, a good Status with value = log_id of the deletion log entry.
-	 *   - If scheduled, a good Status with value = false.
+	 *   - If successful (or scheduled), a good Status
 	 *   - If the page couldn't be deleted because it wasn't found, a Status with a non-fatal 'cannotdelete' error.
 	 *   - A fatal Status otherwise.
 	 */
 	public function deleteUnsafe( string $reason ): Status {
+		$this->setDeletionAttempted();
 		$status = Status::newGood();
 
 		$legacyDeleter = $this->userFactory->newFromAuthority( $this->deleter );
@@ -354,6 +398,9 @@ class DeletePage {
 	 * @return Status
 	 */
 	public function deleteInternal( string $reason, ?string $webRequestId = null ): Status {
+		// The following is necessary for direct calls from the outside
+		$this->setDeletionAttempted();
+
 		$title = $this->page->getTitle();
 		$status = Status::newGood();
 
@@ -437,7 +484,7 @@ class DeletePage {
 
 			$job = new DeletePageJob( $jobParams );
 			$this->jobQueueGroup->push( $job );
-			$status->value = false;
+			$this->wasScheduled = true;
 			return $status;
 		}
 
@@ -512,7 +559,7 @@ class DeletePage {
 			$logEntry,
 			$archivedRevisionCount
 		);
-		$status->value = $logid;
+		$this->successfulDeletionsIDs[] = $logid;
 
 		// Show log excerpt on 404 pages rather than just a link
 		$key = $this->recentDeletesCache->makeKey( 'page-recent-delete', md5( $logTitle->getPrefixedText() ) );

includes/page/WikiPage.php

@@ -2669,10 +2669,13 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 			->keepLegacyHookErrorsSeparate()
 			->deleteUnsafe( $reason );
 		$error = $deletePage->getLegacyHookErrors();
-		if ( $status->isGood() && $status->value === false ) {
-			// BC for scheduled deletion
-			$status->warning( 'delete-scheduled', wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) );
-			$status->value = null;
+		if ( $status->isGood() ) {
+			// BC with old return format
+			if ( $deletePage->deletionWasScheduled() ) {
+				$status->warning( 'delete-scheduled', wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) );
+			} else {
+				$status->value = $deletePage->getSuccessfulDeletionsIDs()[0];
+			}
 		}
 		return $status;
 	}
@@ -2711,10 +2714,13 @@ class WikiPage implements Page, IDBAccessObject, PageRecord {
 			->setLogSubtype( $logsubtype )
 			->forceImmediate( $immediate )
 			->deleteInternal( $reason, $webRequestId );
-		if ( $status->isGood() && $status->value === false ) {
-			// BC for scheduled deletion
-			$status->warning( 'delete-scheduled', wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) );
-			$status->value = null;
+		if ( $status->isGood() ) {
+			// BC with old return format
+			if ( $deletePage->deletionWasScheduled() ) {
+				$status->warning( 'delete-scheduled', wfEscapeWikiText( $this->getTitle()->getPrefixedText() ) );
+			} else {
+				$status->value = $deletePage->getSuccessfulDeletionsIDs()[0];
+			}
 		}
 		return $status;
 	}

tests/phpunit/integration/includes/page/DeletePageTest.php

@@ -220,7 +220,8 @@ class DeletePageTest extends MediaWikiIntegrationTestCase {
 		}
 
 		$reason = "testing deletion";
-		$status = $this->getDeletePage( $page, $deleter )
+		$deletePage = $this->getDeletePage( $page, $deleter );
+		$status = $deletePage
 			->setSuppress( $suppress )
 			->setTags( $tags )
 			->forceImmediate( $immediate )
@@ -230,10 +231,13 @@ class DeletePageTest extends MediaWikiIntegrationTestCase {
 		$this->assertTrue( $status->isGood(), 'Deletion should succeed' );
 
 		if ( $immediate ) {
-			$this->assertIsInt( $status->getValue() );
-			$logID = $status->getValue();
+			$this->assertFalse( $deletePage->deletionWasScheduled() );
+			$logIDs = $deletePage->getSuccessfulDeletionsIDs();
+			$this->assertCount( 1, $logIDs );
+			$logID = $logIDs[0];
+			$this->assertIsInt( $logID );
 		} else {
-			$this->assertFalse( $status->getValue() );
+			$this->assertTrue( $deletePage->deletionWasScheduled() );
 			$this->runJobs();
 			$logID = wfGetDB( DB_REPLICA )->selectField(
 				'logging',

tests/phpunit/unit/includes/page/DeletePageTest.php

@@ -3,6 +3,7 @@
 namespace MediaWiki\Tests\Unit\Page;
 
 use BacklinkCache;
+use BadMethodCallException;
 use BagOStuff;
 use CommentStore;
 use JobQueueGroup;
@@ -182,4 +183,52 @@ class DeletePageTest extends MediaWikiUnitTestCase {
 		$successAuthority = new UltimateAuthority( new UserIdentityValue( 42, 'Deleter' ) );
 		yield 'Successful' => [ $successAuthority, true ];
 	}
+
+	/**
+	 * @covers ::getSuccessfulDeletionsIDs
+	 */
+	public function testGetSuccessfulDeletionsIDs(): void {
+		$delPage = $this->getDeletePage(
+			$this->createMock( ProperPageIdentity::class ),
+			$this->createMock( Authority::class )
+		);
+		$delPage->deleteUnsafe( 'foo' );
+		$this->assertIsArray( $delPage->getSuccessfulDeletionsIDs() );
+	}
+
+	/**
+	 * @covers ::getSuccessfulDeletionsIDs
+	 */
+	public function testGetSuccessfulDeletionsIDs__notAttempted(): void {
+		$delPage = $this->getDeletePage(
+			$this->createMock( ProperPageIdentity::class ),
+			$this->createMock( Authority::class )
+		);
+		$this->expectException( BadMethodCallException::class );
+		$delPage->getSuccessfulDeletionsIDs();
+	}
+
+	/**
+	 * @covers ::deletionWasScheduled
+	 */
+	public function testDeletionWasScheduled(): void {
+		$delPage = $this->getDeletePage(
+			$this->createMock( ProperPageIdentity::class ),
+			$this->createMock( Authority::class )
+		);
+		$delPage->deleteUnsafe( 'foo' );
+		$this->assertIsBool( $delPage->deletionWasScheduled() );
+	}
+
+	/**
+	 * @covers ::deletionWasScheduled
+	 */
+	public function testDeletionWasScheduled__notAttempted(): void {
+		$delPage = $this->getDeletePage(
+			$this->createMock( ProperPageIdentity::class ),
+			$this->createMock( Authority::class )
+		);
+		$this->expectException( BadMethodCallException::class );
+		$delPage->deletionWasScheduled();
+	}
 }