jobqueue: Follow-up for fc5d51f12936ed (added GenericParameterJob)

* Remove duplicate $params check from Job::factory done in Job::__construct.

* In Job::factory(), restore use of a valid title as default for passing as
  constructor arg to old job classes. Their constructor may expect it to
  be valid.
  Keep the invalid dummy in Job::__construct, and document why.

* tests: Update test case for failure mode when using Job::factory
  with a class that requires a title. It asserted getting an invalid
  title. This now restores the behaviour prior to fc5d51f12936ed,
  which is that job classes that require a title, get a valid one.

* tests: Remove test case for testToString that used
  an explicitly passed but invalid params value. I've converted
  that to expect the exception we now throw instead.

* tests: Update getMockJob(), also used by testToString, which was
  relying on undocumented behaviour that 'new Title' is public
  and gets namespace=0 and title=''. Before fc5d51f12936ed,
  title params weren't in toString() and it asserted outputting
  three spaces (delimiter, empty string from formatted title,
  delimiter).
  In fc5d51f12936ed, this changed to asserting "Special:" which
  seems unintentional as we didn't pass it the internally reserved
  NS_SPECIAL/'' value, and yet was caught by the dbkey=='' check.
  Given this test case doesn't deal with titles, omit it for now.

  A job can either have a $title and title/namespace in params,
  or neither. This test was asserting an in-memory scenario
  where $title can be an object, but title/namespace absent from
  params.

Bug: T221368
Depends-On: I89f6ad6967d6f82d87a62c15c0dded901c51b714
Change-Id: I2ec99a12ecc627359a2aae5153d5d7c54156ff46

diff --git a/includes/jobqueue/Job.php b/includes/jobqueue/Job.php
index 6054e35..d2f1dbc 100644 (file)
--- a/includes/jobqueue/Job.php
+++ b/includes/jobqueue/Job.php
@@ -70,14 +70,19 @@ abstract class Job implements RunnableJob {
                        // Backwards compatibility for old signature ($command, $title, $params)
                        $title = $params;
                        $params = func_num_args() >= 3 ? func_get_arg( 2 ) : [];
+               } elseif ( isset( $params['namespace'] ) && isset( $params['title'] ) ) {
+                       // Handle job classes that take title as constructor parameter.
+                       // If a newer classes like GenericParameterJob uses these parameters,
+                       // then this happens in Job::__construct instead.
+                       $title = Title::makeTitle( $params['namespace'], $params['title'] );
                } else {
-                       $title = ( isset( $params['namespace'] ) && isset( $params['title'] ) )
-                               ? Title::makeTitle( $params['namespace'], $params['title'] )
-                               : Title::makeTitle( NS_SPECIAL, '' );
+                       // Default title for job classes not implementing GenericParameterJob.
+                       // This must be a valid title because it not directly passed to
+                       // our Job constructor, but rather it's subclasses which may expect
+                       // to be able to use it.
+                       $title = Title::makeTitle( NS_SPECIAL, 'Blankpage' );
                }
 
-               $params = is_array( $params ) ? $params : []; // sanity
-
                if ( isset( $wgJobClasses[$command] ) ) {
                        $handler = $wgJobClasses[$command];
 
@@ -114,25 +119,35 @@ abstract class Job implements RunnableJob {
                        // Backwards compatibility for old signature ($command, $title, $params)
                        $title = $params;
                        $params = func_num_args() >= 3 ? func_get_arg( 2 ) : [];
-                       $params = is_array( $params ) ? $params : []; // sanity
-                       // Set namespace/title params if both are missing and this is not a dummy title
-                       if (
-                               $title->getDBkey() !== '' &&
-                               !isset( $params['namespace'] ) &&
-                               !isset( $params['title'] )
-                       ) {
-                               $params['namespace'] = $title->getNamespace();
-                               $params['title'] = $title->getDBKey();
-                               // Note that JobQueue classes will prefer the parameters over getTitle()
-                               $this->title = $title;
-                       }
+               } else {
+                       // Newer jobs may choose to not have a top-level title (e.g. GenericParameterJob)
+                       $title = null;
+               }
+
+               if ( !is_array( $params ) ) {
+                       throw new InvalidArgumentException( '$params must be an array' );
+               }
+
+               if (
+                       $title &&
+                       !isset( $params['namespace'] ) &&
+                       !isset( $params['title'] )
+               ) {
+                       // When constructing this class for submitting to the queue,
+                       // normalise the $title arg of old job classes as part of $params.
+                       $params['namespace'] = $title->getNamespace();
+                       $params['title'] = $title->getDBKey();
                }
 
                $this->command = $command;
                $this->params = $params + [ 'requestId' => WebRequest::getRequestId() ];
+
                if ( $this->title === null ) {
+                       // Set this field for access via getTitle().
                        $this->title = ( isset( $params['namespace'] ) && isset( $params['title'] ) )
                                ? Title::makeTitle( $params['namespace'], $params['title'] )
+                               // GenericParameterJob classes without namespace/title params
+                               // should not use getTitle(). Set an invalid title as placeholder.
                                : Title::makeTitle( NS_SPECIAL, '' );
                }
        }

diff --git a/tests/phpunit/includes/jobqueue/JobTest.php b/tests/phpunit/includes/jobqueue/JobTest.php
index 9fe3e3d..878b895 100644 (file)
--- a/tests/phpunit/includes/jobqueue/JobTest.php
+++ b/tests/phpunit/includes/jobqueue/JobTest.php
@@ -27,10 +27,6 @@ class JobTest extends MediaWikiTestCase {
                $requestId = 'requestId=' . WebRequest::getRequestId();
 
                return [
-                       [
-                               $this->getMockJob( false ),
-                               'someCommand Special: ' . $requestId
-                       ],
                        [
                                $this->getMockJob( [ 'key' => 'val' ] ),
                                'someCommand Special: key=val ' . $requestId
@@ -85,16 +81,24 @@ class JobTest extends MediaWikiTestCase {
        }
 
        public function getMockJob( $params ) {
-               $title = new Title();
                $mock = $this->getMockForAbstractClass(
                        Job::class,
-                       [ 'someCommand', $title, $params ],
+                       [ 'someCommand', $params ],
                        'SomeJob'
                );
 
                return $mock;
        }
 
+       /**
+        * @covers Job::__construct()
+        */
+       public function testInvalidParamsArgument() {
+               $params = false;
+               $this->setExpectedException( InvalidArgumentException::class, '$params must be an array' );
+               $job = $this->getMockJob( $params );
+       }
+
        /**
         * @dataProvider provideTestJobFactory
         *
@@ -165,15 +169,15 @@ class JobTest extends MediaWikiTestCase {
         */
        public function testJobSignatureTitleBased() {
                $testPage = Title::makeTitle( NS_PROJECT, 'x' );
-               $blankTitle = Title::makeTitle( NS_SPECIAL, '' );
+               $blankPage = Title::makeTitle( NS_SPECIAL, 'Blankpage' );
                $params = [ 'z' => 1, 'causeAction' => 'unknown', 'causeAgent' => 'unknown' ];
                $paramsWithTitle = $params + [ 'namespace' => NS_PROJECT, 'title' => 'x' ];
+               $paramsWithBlankpage = $params + [ 'namespace' => NS_SPECIAL, 'title' => 'Blankpage' ];
 
                $job = new RefreshLinksJob( $testPage, $params );
                $this->assertEquals( $testPage->getPrefixedText(), $job->getTitle()->getPrefixedText() );
-               $this->assertSame( $testPage, $job->getTitle() );
+               $this->assertTrue( $testPage->equals( $job->getTitle() ) );
                $this->assertJobParamsMatch( $job, $paramsWithTitle );
-               $this->assertSame( $testPage, $job->getTitle() );
 
                $job = Job::factory( 'refreshLinks', $testPage, $params );
                $this->assertEquals( $testPage->getPrefixedText(), $job->getTitle()->getPrefixedText() );
@@ -184,8 +188,8 @@ class JobTest extends MediaWikiTestCase {
                $this->assertJobParamsMatch( $job, $paramsWithTitle );
 
                $job = Job::factory( 'refreshLinks', $params );
-               $this->assertTrue( $blankTitle->equals( $job->getTitle() ) );
-               $this->assertJobParamsMatch( $job, $params );
+               $this->assertTrue( $blankPage->equals( $job->getTitle() ) );
+               $this->assertJobParamsMatch( $job, $paramsWithBlankpage );
        }
 
        /**