From 4dce6445966a1fdeb1e635eca534af91188b74bf Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 18 Apr 2019 16:29:41 +0100 Subject: [PATCH] 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 --- includes/jobqueue/Job.php | 49 ++++++++++++++------- tests/phpunit/includes/jobqueue/JobTest.php | 26 ++++++----- 2 files changed, 47 insertions(+), 28 deletions(-) diff --git a/includes/jobqueue/Job.php b/includes/jobqueue/Job.php index 6054e35a9f..d2f1dbcb31 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, '' ); } } diff --git a/tests/phpunit/includes/jobqueue/JobTest.php b/tests/phpunit/includes/jobqueue/JobTest.php index 9fe3e3d1a4..878b895c26 100644 --- 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 ); } /** -- 2.20.1