ApiDelete: Handle batched deletions properly

When batched deletions via the job queue were added in Ie800fb5a, the
way this was reported caused ApiDelete to report an error. Instead it
should report success with appropriate signaling to the client.

Bug: T212356
Change-Id: I1ef66277e988572c6720cf3e3cb36b18530746b4

diff --git a/RELEASE-NOTES-1.33 b/RELEASE-NOTES-1.33
index 5b6f6b2..ce95bd6 100644 (file)
--- a/RELEASE-NOTES-1.33
+++ b/RELEASE-NOTES-1.33
@@ -75,7 +75,9 @@ production.
     exceptions will now include the "Wikimedia\Rdbms\" prefix in the class name.
   * The code including an exception class name is deprecated. In the future,
     all internal errors will use code "internal_api_error".
-* …
+* (T212356) When using action=delete on pages with many revisions, the module
+  may return a boolean-true 'scheduled' and no 'logid'. This signifies that the
+  deletion will be processed via the job queue.
 
 === Action API internal changes in 1.33 ===
 * A number of deprecated methods for API documentation, intended for overriding

diff --git a/includes/api/ApiBase.php b/includes/api/ApiBase.php
index d29131b..1efd747 100644 (file)
--- a/includes/api/ApiBase.php
+++ b/includes/api/ApiBase.php
@@ -1949,9 +1949,14 @@ abstract class ApiBase extends ContextSource {
         * @since 1.29
         * @param StatusValue $status
         * @param string[] $types 'warning' and/or 'error'
+        * @param string[] $filter Message keys to filter out (since 1.33)
         */
-       public function addMessagesFromStatus( StatusValue $status, $types = [ 'warning', 'error' ] ) {
-               $this->getErrorFormatter()->addMessagesFromStatus( $this->getModulePath(), $status, $types );
+       public function addMessagesFromStatus(
+               StatusValue $status, $types = [ 'warning', 'error' ], array $filter = []
+       ) {
+               $this->getErrorFormatter()->addMessagesFromStatus(
+                       $this->getModulePath(), $status, $types, $filter
+               );
        }
 
        /**

diff --git a/includes/api/ApiDelete.php b/includes/api/ApiDelete.php
index ec857b7..7e8041d 100644 (file)
--- a/includes/api/ApiDelete.php
+++ b/includes/api/ApiDelete.php
@@ -75,9 +75,10 @@ class ApiDelete extends ApiBase {
                        $status = self::delete( $pageObj, $user, $reason, $params['tags'] );
                }
 
-               if ( !$status->isGood() ) {
+               if ( !$status->isOk() ) {
                        $this->dieStatus( $status );
                }
+               $this->addMessagesFromStatus( $status, [ 'warning' ], [ 'delete-scheduled' ] );
 
                // Deprecated parameters
                if ( $params['watch'] ) {
@@ -92,8 +93,14 @@ class ApiDelete extends ApiBase {
                $r = [
                        'title' => $titleObj->getPrefixedText(),
                        'reason' => $reason,
-                       'logid' => $status->value
                ];
+               if ( $status->hasMessage( 'delete-scheduled' ) ) {
+                       $r['scheduled'] = true;
+               }
+               if ( $status->value !== null ) {
+                       // Scheduled deletions don't currently have a log entry available at this point
+                       $r['logid'] = $status->value;
+               }
                $this->getResult()->addValue( null, $this->getModuleName(), $r );
        }
 

diff --git a/includes/api/ApiErrorFormatter.php b/includes/api/ApiErrorFormatter.php
index a37ecc2..9669464 100644 (file)
--- a/includes/api/ApiErrorFormatter.php
+++ b/includes/api/ApiErrorFormatter.php
@@ -153,9 +153,10 @@ class ApiErrorFormatter {
         * @param string|null $modulePath
         * @param StatusValue $status
         * @param string[]|string $types 'warning' and/or 'error'
+        * @param string[] $filter Messages to filter out (since 1.33)
         */
        public function addMessagesFromStatus(
-               $modulePath, StatusValue $status, $types = [ 'warning', 'error' ]
+               $modulePath, StatusValue $status, $types = [ 'warning', 'error' ], array $filter = []
        ) {
                if ( $status->isGood() || !$status->getErrors() ) {
                        return;
@@ -178,7 +179,9 @@ class ApiErrorFormatter {
                                ->inLanguage( $this->lang )
                                ->title( $this->getDummyTitle() )
                                ->useDatabase( $this->useDB );
-                       $this->addWarningOrError( $tag, $modulePath, $msg );
+                       if ( !in_array( $msg->getKey(), $filter, true ) ) {
+                               $this->addWarningOrError( $tag, $modulePath, $msg );
+                       }
                }
        }
 

diff --git a/tests/phpunit/includes/api/ApiDeleteTest.php b/tests/phpunit/includes/api/ApiDeleteTest.php
index fc546ff..803eefb 100644 (file)
--- a/tests/phpunit/includes/api/ApiDeleteTest.php
+++ b/tests/phpunit/includes/api/ApiDeleteTest.php
@@ -41,6 +41,35 @@ class ApiDeleteTest extends ApiTestCase {
                $this->assertFalse( Title::newFromText( $name )->exists() );
        }
 
+       public function testBatchedDelete() {
+               $this->setMwGlobals( 'wgDeleteRevisionsBatchSize', 1 );
+
+               $name = 'Help:' . ucfirst( __FUNCTION__ );
+               for ( $i = 1; $i <= 3; $i++ ) {
+                       $this->editPage( $name, "Revision $i" );
+               }
+
+               $apiResult = $this->doApiRequestWithToken( [
+                       'action' => 'delete',
+                       'title' => $name,
+               ] )[0];
+
+               $this->assertArrayHasKey( 'delete', $apiResult );
+               $this->assertArrayHasKey( 'title', $apiResult['delete'] );
+               $this->assertSame( $name, $apiResult['delete']['title'] );
+               $this->assertArrayHasKey( 'scheduled', $apiResult['delete'] );
+               $this->assertTrue( $apiResult['delete']['scheduled'] );
+               $this->assertArrayNotHasKey( 'logid', $apiResult['delete'] );
+
+               // Run the jobs
+               JobQueueGroup::destroySingletons();
+               $jobs = new RunJobs;
+               $jobs->loadParamsAndArgs( null, [ 'quiet' => true ], null );
+               $jobs->execute();
+
+               $this->assertFalse( Title::newFromText( $name )->exists( Title::GAID_FOR_UPDATE ) );
+       }
+
        public function testDeleteNonexistent() {
                $this->setExpectedException( ApiUsageException::class,
                        "The page you specified doesn't exist." );

diff --git a/tests/phpunit/includes/api/ApiErrorFormatterTest.php b/tests/phpunit/includes/api/ApiErrorFormatterTest.php
index 312ef55..d7628e0 100644 (file)
--- a/tests/phpunit/includes/api/ApiErrorFormatterTest.php
+++ b/tests/phpunit/includes/api/ApiErrorFormatterTest.php
@@ -634,6 +634,40 @@ class ApiErrorFormatterTest extends MediaWikiLangTestCase {
                ];
        }
 
+       public function testAddMessagesFromStatus_filter() {
+               $result = new ApiResult( 8388608 );
+               $formatter = new ApiErrorFormatter( $result, Language::factory( 'qqx' ), 'plaintext', false );
+
+               $status = Status::newGood();
+               $status->warning( 'mainpage' );
+               $status->warning( 'parentheses', 'foobar' );
+               $status->warning( wfMessage( 'mainpage' ) );
+               $status->error( 'mainpage' );
+               $status->error( 'parentheses', 'foobaz' );
+               $formatter->addMessagesFromStatus( 'status', $status, [ 'warning', 'error' ], [ 'mainpage' ] );
+               $this->assertSame( [
+                       'errors' => [
+                               [
+                                       'code' => 'parentheses',
+                                       'text' => '(parentheses: foobaz)',
+                                       'module' => 'status',
+                                       ApiResult::META_CONTENT => 'text',
+                               ],
+                               ApiResult::META_INDEXED_TAG_NAME => 'error',
+                       ],
+                       'warnings' => [
+                               [
+                                       'code' => 'parentheses',
+                                       'text' => '(parentheses: foobar)',
+                                       'module' => 'status',
+                                       ApiResult::META_CONTENT => 'text',
+                               ],
+                               ApiResult::META_INDEXED_TAG_NAME => 'warning',
+                       ],
+                       ApiResult::META_TYPE => 'assoc',
+               ], $result->getResultData() );
+       }
+
        /**
         * @dataProvider provideIsValidApiCode
         * @covers ApiErrorFormatter::isValidApiCode