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

1 files changed, 32 insertions, 17 deletions

diff --git a/includes/jobqueue/Job.php b/includes/jobqueue/Job.php
index 6054e35a9fd9..d2f1dbcb31d3 100644
--- 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, '' );
 		}
 	}