From fc5d51f12936edb4c7d067e298b5ec3a7c09a8fa Mon Sep 17 00:00:00 2001 From: Aaron Schulz Date: Fri, 29 Mar 2019 23:07:48 -0700 Subject: [PATCH] jobqueue: add GenericParameterJob and RunnableJob interface Simplify the code of jobs that do not care about titles and removes the direct Title dependency from JobQueue. Remove getTitle() from IJobSpecification itself. Move all the Job::factory calls into a single JobQueue::factoryJob() method. Depends-on: Iee78f4baeca0c0b4d6db073f2fbcc56855114ab0 Change-Id: I9c9d0726d4066bb0aa937665847ad6042ade13ec --- autoload.php | 2 + includes/deferred/CdnCacheUpdate.php | 11 +- includes/jobqueue/GenericParameterJob.php | 34 ++++++ includes/jobqueue/IJobSpecification.php | 19 ++-- includes/jobqueue/Job.php | 51 +++++---- includes/jobqueue/JobQueue.php | 30 +++-- includes/jobqueue/JobQueueDB.php | 31 ++--- includes/jobqueue/JobQueueFederated.php | 6 +- includes/jobqueue/JobQueueGroup.php | 2 +- includes/jobqueue/JobQueueMemory.php | 11 +- includes/jobqueue/JobQueueRedis.php | 32 +++--- includes/jobqueue/JobSpecification.php | 16 ++- includes/jobqueue/RunnableJob.php | 54 +++++++++ includes/jobqueue/jobs/CdnPurgeJob.php | 10 +- .../jobqueue/jobs/ClearUserWatchlistJob.php | 35 +++--- .../jobs/ClearWatchlistNotificationsJob.php | 6 +- includes/jobqueue/jobs/DeletePageJob.php | 13 +-- includes/jobqueue/jobs/DuplicateJob.php | 15 ++- includes/jobqueue/jobs/EnqueueJob.php | 12 +- includes/jobqueue/jobs/NullJob.php | 11 +- includes/jobqueue/jobs/UserGroupExpiryJob.php | 6 +- includes/page/WikiPage.php | 4 +- includes/watcheditem/WatchedItemStore.php | 7 +- tests/phpunit/includes/SiteStatsTest.php | 4 +- .../includes/jobqueue/JobQueueTest.php | 4 +- tests/phpunit/includes/jobqueue/JobTest.php | 106 ++++++++++++++++-- .../jobs/ClearUserWatchlistJobTest.php | 10 +- 27 files changed, 362 insertions(+), 180 deletions(-) create mode 100644 includes/jobqueue/GenericParameterJob.php create mode 100644 includes/jobqueue/RunnableJob.php diff --git a/autoload.php b/autoload.php index b22aeab527..0ce423f9cf 100644 --- a/autoload.php +++ b/autoload.php @@ -565,6 +565,7 @@ $wgAutoloadLocalClasses = [ 'GenerateNormalizerDataMl' => __DIR__ . '/maintenance/language/generateNormalizerDataMl.php', 'GenerateSitemap' => __DIR__ . '/maintenance/generateSitemap.php', 'GenericArrayObject' => __DIR__ . '/includes/libs/GenericArrayObject.php', + 'GenericParameterJob' => __DIR__ . '/includes/jobqueue/GenericParameterJob.php', 'GetConfiguration' => __DIR__ . '/maintenance/getConfiguration.php', 'GetLagTimes' => __DIR__ . '/maintenance/getLagTimes.php', 'GetReplicaServer' => __DIR__ . '/maintenance/getReplicaServer.php', @@ -1288,6 +1289,7 @@ $wgAutoloadLocalClasses = [ 'RowUpdateGenerator' => __DIR__ . '/includes/utils/RowUpdateGenerator.php', 'RunBatchedQuery' => __DIR__ . '/maintenance/runBatchedQuery.php', 'RunJobs' => __DIR__ . '/maintenance/runJobs.php', + 'RunnableJob' => __DIR__ . '/includes/jobqueue/RunnableJob.php', 'SVGMetadataExtractor' => __DIR__ . '/includes/media/SVGMetadataExtractor.php', 'SVGReader' => __DIR__ . '/includes/media/SVGMetadataExtractor.php', 'SamplingStatsdClient' => __DIR__ . '/includes/libs/stats/SamplingStatsdClient.php', diff --git a/includes/deferred/CdnCacheUpdate.php b/includes/deferred/CdnCacheUpdate.php index 6f961e8b71..2d07f75156 100644 --- a/includes/deferred/CdnCacheUpdate.php +++ b/includes/deferred/CdnCacheUpdate.php @@ -71,13 +71,10 @@ class CdnCacheUpdate implements DeferrableUpdate, MergeableUpdate { self::purge( $this->urls ); if ( $wgCdnReboundPurgeDelay > 0 ) { - JobQueueGroup::singleton()->lazyPush( new CdnPurgeJob( - Title::makeTitle( NS_SPECIAL, 'Badtitle/' . __CLASS__ ), - [ - 'urls' => $this->urls, - 'jobReleaseTimestamp' => time() + $wgCdnReboundPurgeDelay - ] - ) ); + JobQueueGroup::singleton()->lazyPush( new CdnPurgeJob( [ + 'urls' => $this->urls, + 'jobReleaseTimestamp' => time() + $wgCdnReboundPurgeDelay + ] ) ); } } diff --git a/includes/jobqueue/GenericParameterJob.php b/includes/jobqueue/GenericParameterJob.php new file mode 100644 index 0000000000..f7da42bd2e --- /dev/null +++ b/includes/jobqueue/GenericParameterJob.php @@ -0,0 +1,34 @@ += 3 ? func_get_arg( 2 ) : []; } else { - // Subclasses can override getTitle() to return something more meaningful - $title = Title::makeTitle( NS_SPECIAL, 'Blankpage' ); + $title = ( isset( $params['namespace'] ) && isset( $params['title'] ) ) + ? Title::makeTitle( $params['namespace'], $params['title'] ) + : Title::makeTitle( NS_SPECIAL, '' ); } + $params = is_array( $params ) ? $params : []; // sanity + if ( isset( $wgJobClasses[$command] ) ) { $handler = $wgJobClasses[$command]; if ( is_callable( $handler ) ) { $job = call_user_func( $handler, $title, $params ); } elseif ( class_exists( $handler ) ) { - $job = new $handler( $title, $params ); + if ( is_subclass_of( $handler, GenericParameterJob::class ) ) { + $job = new $handler( $params ); + } else { + $job = new $handler( $title, $params ); + } } else { $job = null; } @@ -112,17 +113,27 @@ abstract class Job implements IJobSpecification { if ( $params instanceof Title ) { // Backwards compatibility for old signature ($command, $title, $params) $title = $params; - $params = func_get_arg( 2 ); - } else { - // Subclasses can override getTitle() to return something more meaningful - $title = Title::makeTitle( NS_SPECIAL, 'Blankpage' ); + $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; + } } $this->command = $command; - $this->title = $title; - $this->params = is_array( $params ) ? $params : []; - if ( !isset( $this->params['requestId'] ) ) { - $this->params['requestId'] = WebRequest::getRequestId(); + $this->params = $params + [ 'requestId' => WebRequest::getRequestId() ]; + if ( $this->title === null ) { + $this->title = ( isset( $params['namespace'] ) && isset( $params['title'] ) ) + ? Title::makeTitle( $params['namespace'], $params['title'] ) + : Title::makeTitle( NS_SPECIAL, '' ); } } @@ -145,7 +156,7 @@ abstract class Job implements IJobSpecification { /** * @return Title */ - public function getTitle() { + final public function getTitle() { return $this->title; } @@ -268,8 +279,6 @@ abstract class Job implements IJobSpecification { public function getDeduplicationInfo() { $info = [ 'type' => $this->getType(), - 'namespace' => $this->getTitle()->getNamespace(), - 'title' => $this->getTitle()->getDBkey(), 'params' => $this->getParams() ]; if ( is_array( $info['params'] ) ) { diff --git a/includes/jobqueue/JobQueue.php b/includes/jobqueue/JobQueue.php index 064400268e..f5ed7b91cb 100644 --- a/includes/jobqueue/JobQueue.php +++ b/includes/jobqueue/JobQueue.php @@ -361,7 +361,7 @@ abstract class JobQueue { * Outside callers should use JobQueueGroup::pop() instead of this function. * * @throws JobQueueError - * @return Job|bool Returns false if there are no jobs + * @return RunnableJob|bool Returns false if there are no jobs */ final public function pop() { $this->assertNotReadOnly(); @@ -383,7 +383,7 @@ abstract class JobQueue { /** * @see JobQueue::pop() - * @return Job|bool + * @return RunnableJob|bool */ abstract protected function doPop(); @@ -393,11 +393,11 @@ abstract class JobQueue { * This does nothing for certain queue classes or if "claimTTL" is not set. * Outside callers should use JobQueueGroup::ack() instead of this function. * - * @param Job $job + * @param RunnableJob $job * @return void * @throws JobQueueError */ - final public function ack( Job $job ) { + final public function ack( RunnableJob $job ) { $this->assertNotReadOnly(); if ( $job->getType() !== $this->type ) { throw new JobQueueError( "Got '{$job->getType()}' job; expected '{$this->type}'." ); @@ -408,9 +408,9 @@ abstract class JobQueue { /** * @see JobQueue::ack() - * @param Job $job + * @param RunnableJob $job */ - abstract protected function doAck( Job $job ); + abstract protected function doAck( RunnableJob $job ); /** * Register the "root job" of a given job into the queue for de-duplication. @@ -482,11 +482,11 @@ abstract class JobQueue { /** * Check if the "root" job of a given job has been superseded by a newer one * - * @param Job $job + * @param IJobSpecification $job * @throws JobQueueError * @return bool */ - final protected function isRootJobOldDuplicate( Job $job ) { + final protected function isRootJobOldDuplicate( IJobSpecification $job ) { if ( $job->getType() !== $this->type ) { throw new JobQueueError( "Got '{$job->getType()}' job; expected '{$this->type}'." ); } @@ -497,10 +497,10 @@ abstract class JobQueue { /** * @see JobQueue::isRootJobOldDuplicate() - * @param Job $job + * @param IJobSpecification $job * @return bool */ - protected function doIsRootJobOldDuplicate( Job $job ) { + protected function doIsRootJobOldDuplicate( IJobSpecification $job ) { if ( !$job->hasRootJobParams() ) { return false; // job has no de-deplication info } @@ -686,6 +686,16 @@ abstract class JobQueue { return null; // not supported } + /** + * @param string $command + * @param array $params + * @return Job + */ + protected function factoryJob( $command, $params ) { + // @TODO: dependency inject this as a callback + return Job::factory( $command, $params ); + } + /** * @throws JobQueueReadOnlyError */ diff --git a/includes/jobqueue/JobQueueDB.php b/includes/jobqueue/JobQueueDB.php index c2772a6381..47ee5886d4 100644 --- a/includes/jobqueue/JobQueueDB.php +++ b/includes/jobqueue/JobQueueDB.php @@ -290,7 +290,7 @@ class JobQueueDB extends JobQueue { /** * @see JobQueue::doPop() - * @return Job|bool + * @return RunnableJob|bool */ protected function doPop() { $dbw = $this->getMasterDB(); @@ -314,10 +314,12 @@ class JobQueueDB extends JobQueue { break; // nothing to do } $this->incrStats( 'pops', $this->type ); + // Get the job object from the row... - $title = Title::makeTitle( $row->job_namespace, $row->job_title ); - $job = Job::factory( $row->job_cmd, $title, - self::extractBlob( $row->job_params ) ); + $params = self::extractBlob( $row->job_params ); + $params = is_array( $params ) ? $params : []; // sanity + $params += [ 'namespace' => $row->job_namespace, 'title' => $row->job_title ]; + $job = $this->factoryJob( $row->job_cmd, $params ); $job->setMetadata( 'id', $row->job_id ); $job->setMetadata( 'timestamp', $row->job_timestamp ); break; // done @@ -481,10 +483,10 @@ class JobQueueDB extends JobQueue { /** * @see JobQueue::doAck() - * @param Job $job + * @param RunnableJob $job * @throws MWException */ - protected function doAck( Job $job ) { + protected function doAck( RunnableJob $job ) { $id = $job->getMetadata( 'id' ); if ( $id === null ) { throw new MWException( "Job of type '{$job->getType()}' has no ID." ); @@ -617,11 +619,14 @@ class JobQueueDB extends JobQueue { return new MappedIterator( $dbr->select( 'job', self::selectFields(), $conds ), function ( $row ) { - $job = Job::factory( - $row->job_cmd, - Title::makeTitle( $row->job_namespace, $row->job_title ), - strlen( $row->job_params ) ? unserialize( $row->job_params ) : [] - ); + $params = strlen( $row->job_params ) ? unserialize( $row->job_params ) : []; + $params = is_array( $params ) ? $params : []; // sanity + $params += [ + 'namespace' => $row->job_namespace, + 'title' => $row->job_title + ]; + + $job = $this->factoryJob( $row->job_cmd, $params ); $job->setMetadata( 'id', $row->job_id ); $job->setMetadata( 'timestamp', $row->job_timestamp ); @@ -774,8 +779,8 @@ class JobQueueDB extends JobQueue { return [ // Fields that describe the nature of the job 'job_cmd' => $job->getType(), - 'job_namespace' => $job->getTitle()->getNamespace(), - 'job_title' => $job->getTitle()->getDBkey(), + 'job_namespace' => $job->getParams()['namespace'] ?? NS_SPECIAL, + 'job_title' => $job->getParams()['title'] ?? '', 'job_params' => self::makeBlob( $job->getParams() ), // Additional job metadata 'job_timestamp' => $db->timestamp(), diff --git a/includes/jobqueue/JobQueueFederated.php b/includes/jobqueue/JobQueueFederated.php index 30ab7e739b..8b5a62ef54 100644 --- a/includes/jobqueue/JobQueueFederated.php +++ b/includes/jobqueue/JobQueueFederated.php @@ -199,7 +199,7 @@ class JobQueueFederated extends JobQueue { * @param HashRing &$partitionRing * @param int $flags * @throws JobQueueError - * @return array List of Job object that could not be inserted + * @return IJobSpecification[] List of Job object that could not be inserted */ protected function tryJobInsertions( array $jobs, HashRing &$partitionRing, $flags ) { $jobsLeft = []; @@ -299,7 +299,7 @@ class JobQueueFederated extends JobQueue { return false; } - protected function doAck( Job $job ) { + protected function doAck( RunnableJob $job ) { $partition = $job->getMetadata( 'QueuePartition' ); if ( $partition === null ) { throw new MWException( "The given job has no defined partition name." ); @@ -308,7 +308,7 @@ class JobQueueFederated extends JobQueue { $this->partitionQueues[$partition]->ack( $job ); } - protected function doIsRootJobOldDuplicate( Job $job ) { + protected function doIsRootJobOldDuplicate( IJobSpecification $job ) { $signature = $job->getRootJobParams()['rootJobSignature']; $partition = $this->partitionRing->getLiveLocation( $signature ); try { diff --git a/includes/jobqueue/JobQueueGroup.php b/includes/jobqueue/JobQueueGroup.php index 4bac304d13..83e5fb24bf 100644 --- a/includes/jobqueue/JobQueueGroup.php +++ b/includes/jobqueue/JobQueueGroup.php @@ -234,7 +234,7 @@ class JobQueueGroup { * @param int|string $qtype JobQueueGroup::TYPE_* constant or job type string * @param int $flags Bitfield of JobQueueGroup::USE_* constants * @param array $blacklist List of job types to ignore - * @return Job|bool Returns false on failure + * @return RunnableJob|bool Returns false on failure */ public function pop( $qtype = self::TYPE_DEFAULT, $flags = 0, array $blacklist = [] ) { global $wgJobClasses; diff --git a/includes/jobqueue/JobQueueMemory.php b/includes/jobqueue/JobQueueMemory.php index cbcd4fb28a..cb20a76079 100644 --- a/includes/jobqueue/JobQueueMemory.php +++ b/includes/jobqueue/JobQueueMemory.php @@ -111,7 +111,7 @@ class JobQueueMemory extends JobQueue { /** * @see JobQueue::doPop * - * @return Job|bool + * @return RunnableJob|bool */ protected function doPop() { if ( $this->doGetSize() == 0 ) { @@ -143,9 +143,9 @@ class JobQueueMemory extends JobQueue { /** * @see JobQueue::doAck * - * @param Job $job + * @param RunnableJob $job */ - protected function doAck( Job $job ) { + protected function doAck( RunnableJob $job ) { if ( $this->getAcquiredCount() == 0 ) { return; } @@ -206,11 +206,10 @@ class JobQueueMemory extends JobQueue { /** * @param IJobSpecification $spec - * - * @return Job + * @return RunnableJob */ public function jobFromSpecInternal( IJobSpecification $spec ) { - return Job::factory( $spec->getType(), $spec->getTitle(), $spec->getParams() ); + return $this->factoryJob( $spec->getType(), $spec->getParams() ); } /** diff --git a/includes/jobqueue/JobQueueRedis.php b/includes/jobqueue/JobQueueRedis.php index 98a5491501..886468826b 100644 --- a/includes/jobqueue/JobQueueRedis.php +++ b/includes/jobqueue/JobQueueRedis.php @@ -307,7 +307,7 @@ LUA; /** * @see JobQueue::doPop() - * @return Job|bool + * @return RunnableJob|bool * @throws JobQueueError */ protected function doPop() { @@ -379,12 +379,12 @@ LUA; /** * @see JobQueue::doAck() - * @param Job $job - * @return Job|bool + * @param RunnableJob $job + * @return RunnableJob|bool * @throws UnexpectedValueException * @throws JobQueueError */ - protected function doAck( Job $job ) { + protected function doAck( RunnableJob $job ) { $uuid = $job->getMetadata( 'uuid' ); if ( $uuid === null ) { throw new UnexpectedValueException( "Job of type '{$job->getType()}' has no UUID." ); @@ -463,11 +463,11 @@ LUA; /** * @see JobQueue::doIsRootJobOldDuplicate() - * @param Job $job + * @param IJobSpecification $job * @return bool * @throws JobQueueError */ - protected function doIsRootJobOldDuplicate( Job $job ) { + protected function doIsRootJobOldDuplicate( IJobSpecification $job ) { if ( !$job->hasRootJobParams() ) { return false; // job has no de-deplication info } @@ -627,7 +627,7 @@ LUA; * * @param string $uid * @param RedisConnRef $conn - * @return Job|bool Returns false if the job does not exist + * @return RunnableJob|bool Returns false if the job does not exist * @throws JobQueueError * @throws UnexpectedValueException */ @@ -641,8 +641,10 @@ LUA; if ( !is_array( $item ) ) { // this shouldn't happen throw new UnexpectedValueException( "Could not find job with ID '$uid'." ); } - $title = Title::makeTitle( $item['namespace'], $item['title'] ); - $job = Job::factory( $item['type'], $title, $item['params'] ); + + $params = $item['params']; + $params += [ 'namespace' => $item['namespace'], 'title' => $item['title'] ]; + $job = $this->factoryJob( $item['type'], $params ); $job->setMetadata( 'uuid', $item['uuid'] ); $job->setMetadata( 'timestamp', $item['timestamp'] ); // Add in attempt count for debugging at showJobs.php @@ -684,8 +686,8 @@ LUA; return [ // Fields that describe the nature of the job 'type' => $job->getType(), - 'namespace' => $job->getTitle()->getNamespace(), - 'title' => $job->getTitle()->getDBkey(), + 'namespace' => $job->getParams()['namespace'] ?? NS_SPECIAL, + 'title' => $job->getParams()['title'] ?? '', 'params' => $job->getParams(), // Some jobs cannot run until a "release timestamp" 'rtimestamp' => $job->getReleaseTimestamp() ?: 0, @@ -700,11 +702,13 @@ LUA; /** * @param array $fields - * @return Job|bool + * @return RunnableJob|bool */ protected function getJobFromFields( array $fields ) { - $title = Title::makeTitle( $fields['namespace'], $fields['title'] ); - $job = Job::factory( $fields['type'], $title, $fields['params'] ); + $params = $fields['params']; + $params += [ 'namespace' => $fields['namespace'], 'title' => $fields['title'] ]; + + $job = $this->factoryJob( $fields['type'], $params ); $job->setMetadata( 'uuid', $fields['uuid'] ); $job->setMetadata( 'timestamp', $fields['timestamp'] ); diff --git a/includes/jobqueue/JobSpecification.php b/includes/jobqueue/JobSpecification.php index b04aa83808..80a46d04ba 100644 --- a/includes/jobqueue/JobSpecification.php +++ b/includes/jobqueue/JobSpecification.php @@ -28,8 +28,7 @@ * $job = new JobSpecification( * 'null', * array( 'lives' => 1, 'usleep' => 100, 'pi' => 3.141569 ), - * array( 'removeDuplicates' => 1 ), - * Title::makeTitle( NS_SPECIAL, 'nullity' ) + * array( 'removeDuplicates' => 1 ) * ); * JobQueueGroup::singleton()->push( $job ) * @endcode @@ -63,8 +62,19 @@ class JobSpecification implements IJobSpecification { $this->validateParams( $opts ); $this->type = $type; + if ( $title instanceof Title ) { + // Make sure JobQueue classes can pull the title from parameters alone + if ( $title->getDBkey() !== '' ) { + $params += [ + 'namespace' => $title->getNamespace(), + 'title' => $title->getDBkey() + ]; + } + } else { + $title = Title::makeTitle( NS_SPECIAL, '' ); + } $this->params = $params; - $this->title = $title ?: Title::makeTitle( NS_SPECIAL, 'Blankpage' ); + $this->title = $title; $this->opts = $opts; } diff --git a/includes/jobqueue/RunnableJob.php b/includes/jobqueue/RunnableJob.php new file mode 100644 index 0000000000..e477b12c1e --- /dev/null +++ b/includes/jobqueue/RunnableJob.php @@ -0,0 +1,54 @@ +removeDuplicates = false; // delay semantics are critical } diff --git a/includes/jobqueue/jobs/ClearUserWatchlistJob.php b/includes/jobqueue/jobs/ClearUserWatchlistJob.php index 77adfa1a94..01fa46c002 100644 --- a/includes/jobqueue/jobs/ClearUserWatchlistJob.php +++ b/includes/jobqueue/jobs/ClearUserWatchlistJob.php @@ -10,7 +10,17 @@ use MediaWiki\MediaWikiServices; * @ingroup JobQueue * @since 1.31 */ -class ClearUserWatchlistJob extends Job { +class ClearUserWatchlistJob extends Job implements GenericParameterJob { + /** + * @param array $params + * - userId, The ID for the user whose watchlist is being cleared. + * - maxWatchlistId, The maximum wl_id at the time the job was first created, + */ + public function __construct( array $params ) { + parent::__construct( 'clearUserWatchlist', $params ); + + $this->removeDuplicates = true; + } /** * @param User $user User to clear the watchlist for. @@ -19,26 +29,7 @@ class ClearUserWatchlistJob extends Job { * @return ClearUserWatchlistJob */ public static function newForUser( User $user, $maxWatchlistId ) { - return new self( - null, - [ 'userId' => $user->getId(), 'maxWatchlistId' => $maxWatchlistId ] - ); - } - - /** - * @param Title|null $title Not used by this job. - * @param array $params - * - userId, The ID for the user whose watchlist is being cleared. - * - maxWatchlistId, The maximum wl_id at the time the job was first created, - */ - public function __construct( Title $title = null, array $params ) { - parent::__construct( - 'clearUserWatchlist', - SpecialPage::getTitleFor( 'EditWatchlist', 'clear' ), - $params - ); - - $this->removeDuplicates = true; + return new self( [ 'userId' => $user->getId(), 'maxWatchlistId' => $maxWatchlistId ] ); } public function run() { @@ -101,7 +92,7 @@ class ClearUserWatchlistJob extends Job { if ( count( $watchlistIds ) === (int)$batchSize ) { // Until we get less results than the limit, recursively push // the same job again. - JobQueueGroup::singleton()->push( new self( $this->getTitle(), $this->getParams() ) ); + JobQueueGroup::singleton()->push( new self( $this->getParams() ) ); } return true; diff --git a/includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php b/includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php index 3b2c899012..f53174a573 100644 --- a/includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php +++ b/includes/jobqueue/jobs/ClearWatchlistNotificationsJob.php @@ -33,9 +33,9 @@ use MediaWiki\MediaWikiServices; * @ingroup JobQueue * @since 1.31 */ -class ClearWatchlistNotificationsJob extends Job { - function __construct( Title $title, array $params ) { - parent::__construct( 'clearWatchlistNotifications', $title, $params ); +class ClearWatchlistNotificationsJob extends Job implements GenericParameterJob { + function __construct( array $params ) { + parent::__construct( 'clearWatchlistNotifications', $params ); static $required = [ 'userId', 'casTime' ]; $missing = implode( ', ', array_diff( $required, array_keys( $this->params ) ) ); diff --git a/includes/jobqueue/jobs/DeletePageJob.php b/includes/jobqueue/jobs/DeletePageJob.php index e6dfae497e..b0ce6a5d69 100644 --- a/includes/jobqueue/jobs/DeletePageJob.php +++ b/includes/jobqueue/jobs/DeletePageJob.php @@ -3,16 +3,13 @@ /** * Class DeletePageJob */ -class DeletePageJob extends Job { - public function __construct( $title, $params = [] ) { - parent::__construct( 'deletePage', $title, $params ); +class DeletePageJob extends Job implements GenericParameterJob { + public function __construct( array $params ) { + parent::__construct( 'deletePage', $params ); + + $this->title = Title::makeTitle( $params['namespace'], $params['title'] ); } - /** - * Execute the job - * - * @return bool - */ public function run() { // Failure to load the page is not job failure. // A parallel deletion operation may have already completed the page deletion. diff --git a/includes/jobqueue/jobs/DuplicateJob.php b/includes/jobqueue/jobs/DuplicateJob.php index c005a29a8b..4231e1563e 100644 --- a/includes/jobqueue/jobs/DuplicateJob.php +++ b/includes/jobqueue/jobs/DuplicateJob.php @@ -26,29 +26,28 @@ * * @ingroup JobQueue */ -final class DuplicateJob extends Job { +final class DuplicateJob extends Job implements GenericParameterJob { /** * Callers should use DuplicateJob::newFromJob() instead * - * @param Title $title * @param array $params Job parameters */ - function __construct( Title $title, array $params ) { - parent::__construct( 'duplicate', $title, $params ); + function __construct( array $params ) { + parent::__construct( 'duplicate', $params ); } /** * Get a duplicate no-op version of a job * - * @param Job $job + * @param RunnableJob $job * @return Job */ - public static function newFromJob( Job $job ) { - $djob = new self( $job->getTitle(), $job->getParams() ); + public static function newFromJob( RunnableJob $job ) { + $djob = new self( $job->getParams() ); $djob->command = $job->getType(); $djob->params = is_array( $djob->params ) ? $djob->params : []; $djob->params = [ 'isDuplicate' => true ] + $djob->params; - $djob->metadata = $job->metadata; + $djob->metadata = $job->getMetadata(); return $djob; } diff --git a/includes/jobqueue/jobs/EnqueueJob.php b/includes/jobqueue/jobs/EnqueueJob.php index 72923ce755..f9735d53c1 100644 --- a/includes/jobqueue/jobs/EnqueueJob.php +++ b/includes/jobqueue/jobs/EnqueueJob.php @@ -32,15 +32,14 @@ * @ingroup JobQueue * @since 1.25 */ -final class EnqueueJob extends Job { +final class EnqueueJob extends Job implements GenericParameterJob { /** * Callers should use the factory methods instead * - * @param Title $title * @param array $params Job parameters */ - function __construct( Title $title, array $params ) { - parent::__construct( 'enqueue', $title, $params ); + public function __construct( array $params ) { + parent::__construct( 'enqueue', $params ); } /** @@ -75,10 +74,7 @@ final class EnqueueJob extends Job { } } - $eJob = new self( - Title::makeTitle( NS_SPECIAL, 'Badtitle/' . __CLASS__ ), - [ 'jobsByDomain' => $jobMapsByDomain ] - ); + $eJob = new self( [ 'jobsByDomain' => $jobMapsByDomain ] ); // If *all* jobs to be pushed are to be de-duplicated (a common case), then // de-duplicate this whole job itself to avoid build up in high traffic cases $eJob->removeDuplicates = $deduplicate; diff --git a/includes/jobqueue/jobs/NullJob.php b/includes/jobqueue/jobs/NullJob.php index 80826fe112..01afe6f040 100644 --- a/includes/jobqueue/jobs/NullJob.php +++ b/includes/jobqueue/jobs/NullJob.php @@ -31,7 +31,7 @@ * @code * $ php maintenance/eval.php * > $queue = JobQueueGroup::singleton(); - * > $job = new NullJob( Title::newMainPage(), [ 'lives' => 10 ] ); + * > $job = new NullJob( [ 'lives' => 10 ] ); * > $queue->push( $job ); * @endcode * You can then confirm the job has been enqueued by using the showJobs.php @@ -44,13 +44,12 @@ * * @ingroup JobQueue */ -class NullJob extends Job { +class NullJob extends Job implements GenericParameterJob { /** - * @param Title $title * @param array $params Job parameters (lives, usleep) */ - function __construct( Title $title, array $params ) { - parent::__construct( 'null', $title, $params ); + function __construct( array $params ) { + parent::__construct( 'null', $params ); if ( !isset( $this->params['lives'] ) ) { $this->params['lives'] = 1; } @@ -67,7 +66,7 @@ class NullJob extends Job { if ( $this->params['lives'] > 1 ) { $params = $this->params; $params['lives']--; - $job = new self( $this->title, $params ); + $job = new self( $params ); JobQueueGroup::singleton()->push( $job ); } diff --git a/includes/jobqueue/jobs/UserGroupExpiryJob.php b/includes/jobqueue/jobs/UserGroupExpiryJob.php index bd0df5b276..ac8f94ab50 100644 --- a/includes/jobqueue/jobs/UserGroupExpiryJob.php +++ b/includes/jobqueue/jobs/UserGroupExpiryJob.php @@ -21,9 +21,9 @@ * @ingroup JobQueue */ -class UserGroupExpiryJob extends Job { - public function __construct( $params = [] ) { - parent::__construct( 'userGroupExpiry', Title::newMainPage(), $params ); +class UserGroupExpiryJob extends Job implements GenericParameterJob { + public function __construct( array $params = [] ) { + parent::__construct( 'userGroupExpiry', $params ); $this->removeDuplicates = true; } diff --git a/includes/page/WikiPage.php b/includes/page/WikiPage.php index 655fa2711f..f33ce3536f 100644 --- a/includes/page/WikiPage.php +++ b/includes/page/WikiPage.php @@ -2736,6 +2736,8 @@ class WikiPage implements Page, IDBAccessObject { $dbw->endAtomic( __METHOD__ ); $jobParams = [ + 'namespace' => $this->getTitle()->getNamespace(), + 'title' => $this->getTitle()->getDBkey(), 'wikiPageId' => $id, 'requestId' => $webRequestId ?? WebRequest::getRequestId(), 'reason' => $reason, @@ -2745,7 +2747,7 @@ class WikiPage implements Page, IDBAccessObject { 'logsubtype' => $logsubtype, ]; - $job = new DeletePageJob( $this->getTitle(), $jobParams ); + $job = new DeletePageJob( $jobParams ); JobQueueGroup::singleton()->push( $job ); $status->warning( 'delete-scheduled', diff --git a/includes/watcheditem/WatchedItemStore.php b/includes/watcheditem/WatchedItemStore.php index 8aca689a24..e287a35525 100644 --- a/includes/watcheditem/WatchedItemStore.php +++ b/includes/watcheditem/WatchedItemStore.php @@ -904,10 +904,9 @@ class WatchedItemStore implements WatchedItemStoreInterface, StatsdAwareInterfac } // If the page is watched by the user (or may be watched), update the timestamp - $job = new ClearWatchlistNotificationsJob( - $user->getUserPage(), - [ 'userId' => $user->getId(), 'timestamp' => $timestamp, 'casTime' => time() ] - ); + $job = new ClearWatchlistNotificationsJob( [ + 'userId' => $user->getId(), 'timestamp' => $timestamp, 'casTime' => time() + ] ); // Try to run this post-send // Calls DeferredUpdates::addCallableUpdate in normal operation diff --git a/tests/phpunit/includes/SiteStatsTest.php b/tests/phpunit/includes/SiteStatsTest.php index a6a92c68a6..b0d89a6ae5 100644 --- a/tests/phpunit/includes/SiteStatsTest.php +++ b/tests/phpunit/includes/SiteStatsTest.php @@ -10,11 +10,11 @@ class SiteStatsTest extends MediaWikiTestCase { $this->setService( 'MainWANObjectCache', $cache ); $jobq = JobQueueGroup::singleton(); - $jobq->push( new NullJob( Title::newMainPage(), [] ) ); + $jobq->push( Job::factory( 'null', Title::newMainPage(), [] ) ); $this->assertEquals( 1, SiteStats::jobs(), 'A single job enqueued bumps jobscount stat to 1' ); - $jobq->push( new NullJob( Title::newMainPage(), [] ) ); + $jobq->push( Job::factory( 'null', Title::newMainPage(), [] ) ); $this->assertEquals( 1, SiteStats::jobs(), 'SiteStats::jobs() count does not reflect addition ' . 'of a second job (cached)' diff --git a/tests/phpunit/includes/jobqueue/JobQueueTest.php b/tests/phpunit/includes/jobqueue/JobQueueTest.php index 81a80b66d5..1baaa5489e 100644 --- a/tests/phpunit/includes/jobqueue/JobQueueTest.php +++ b/tests/phpunit/includes/jobqueue/JobQueueTest.php @@ -380,12 +380,12 @@ class JobQueueTest extends MediaWikiTestCase { } function newJob( $i = 0, $rootJob = [] ) { - return new NullJob( Title::newMainPage(), + return Job::factory( 'null', Title::newMainPage(), [ 'lives' => 0, 'usleep' => 0, 'removeDuplicates' => 0, 'i' => $i ] + $rootJob ); } function newDedupedJob( $i = 0, $rootJob = [] ) { - return new NullJob( Title::newMainPage(), + return Job::factory( 'null', Title::newMainPage(), [ 'lives' => 0, 'usleep' => 0, 'removeDuplicates' => 1, 'i' => $i ] + $rootJob ); } } diff --git a/tests/phpunit/includes/jobqueue/JobTest.php b/tests/phpunit/includes/jobqueue/JobTest.php index 769b1930fe..9fe3e3d1a4 100644 --- a/tests/phpunit/includes/jobqueue/JobTest.php +++ b/tests/phpunit/includes/jobqueue/JobTest.php @@ -29,31 +29,31 @@ class JobTest extends MediaWikiTestCase { return [ [ $this->getMockJob( false ), - 'someCommand ' . $requestId + 'someCommand Special: ' . $requestId ], [ $this->getMockJob( [ 'key' => 'val' ] ), - 'someCommand key=val ' . $requestId + 'someCommand Special: key=val ' . $requestId ], [ $this->getMockJob( [ 'key' => [ 'inkey' => 'inval' ] ] ), - 'someCommand key={"inkey":"inval"} ' . $requestId + 'someCommand Special: key={"inkey":"inval"} ' . $requestId ], [ $this->getMockJob( [ 'val1' ] ), - 'someCommand 0=val1 ' . $requestId + 'someCommand Special: 0=val1 ' . $requestId ], [ $this->getMockJob( [ 'val1', 'val2' ] ), - 'someCommand 0=val1 1=val2 ' . $requestId + 'someCommand Special: 0=val1 1=val2 ' . $requestId ], [ $this->getMockJob( [ new stdClass() ] ), - 'someCommand 0=object(stdClass) ' . $requestId + 'someCommand Special: 0=object(stdClass) ' . $requestId ], [ $this->getMockJob( [ $mockToStringObj ] ), - 'someCommand 0={STRING_OBJ_VAL} ' . $requestId + 'someCommand Special: 0={STRING_OBJ_VAL} ' . $requestId ], [ $this->getMockJob( [ @@ -72,7 +72,7 @@ class JobTest extends MediaWikiTestCase { ], "triggeredRecursive" => true ] ), - 'someCommand pages={"932737":[0,"Robert_James_Waller"]} ' . + 'someCommand Special: pages={"932737":[0,"Robert_James_Waller"]} ' . 'rootJobSignature=45868e99bba89064e4483743ebb9b682ef95c1a7 ' . 'rootJobTimestamp=20160309110158 masterPos=' . '{"file":"db1023-bin.001288","pos":"308257743","asOfTime":' . @@ -85,11 +85,13 @@ class JobTest extends MediaWikiTestCase { } public function getMockJob( $params ) { + $title = new Title(); $mock = $this->getMockForAbstractClass( Job::class, - [ 'someCommand', new Title(), $params ], + [ 'someCommand', $title, $params ], 'SomeJob' ); + return $mock; } @@ -115,7 +117,7 @@ class JobTest extends MediaWikiTestCase { return [ 'class name' => [ 'NullJob' ], 'closure' => [ function ( Title $title, array $params ) { - return new NullJob( $title, $params ); + return Job::factory( 'null', $title, $params ); } ], 'function' => [ [ $this, 'newNullJob' ] ], 'static function' => [ self::class . '::staticNullJob' ] @@ -123,11 +125,91 @@ class JobTest extends MediaWikiTestCase { } public function newNullJob( Title $title, array $params ) { - return new NullJob( $title, $params ); + return Job::factory( 'null', $title, $params ); } public static function staticNullJob( Title $title, array $params ) { - return new NullJob( $title, $params ); + return Job::factory( 'null', $title, $params ); + } + + /** + * @covers Job::factory + * @covers Job::__construct() + */ + public function testJobSignatureGeneric() { + $testPage = Title::makeTitle( NS_PROJECT, 'x' ); + $blankTitle = Title::makeTitle( NS_SPECIAL, '' ); + $params = [ 'z' => 1, 'lives' => 1, 'usleep' => 0 ]; + $paramsWithTitle = $params + [ 'namespace' => NS_PROJECT, 'title' => 'x' ]; + + $job = new NullJob( [ 'namespace' => NS_PROJECT, 'title' => 'x' ] + $params ); + $this->assertEquals( $testPage->getPrefixedText(), $job->getTitle()->getPrefixedText() ); + $this->assertJobParamsMatch( $job, $paramsWithTitle ); + + $job = Job::factory( 'null', $testPage, $params ); + $this->assertEquals( $blankTitle->getPrefixedText(), $job->getTitle()->getPrefixedText() ); + $this->assertJobParamsMatch( $job, $params ); + + $job = Job::factory( 'null', $paramsWithTitle ); + $this->assertEquals( $testPage->getPrefixedText(), $job->getTitle()->getPrefixedText() ); + $this->assertJobParamsMatch( $job, $paramsWithTitle ); + + $job = Job::factory( 'null', $params ); + $this->assertTrue( $blankTitle->equals( $job->getTitle() ) ); + $this->assertJobParamsMatch( $job, $params ); + } + + /** + * @covers Job::factory + * @covers Job::__construct() + */ + public function testJobSignatureTitleBased() { + $testPage = Title::makeTitle( NS_PROJECT, 'x' ); + $blankTitle = Title::makeTitle( NS_SPECIAL, '' ); + $params = [ 'z' => 1, 'causeAction' => 'unknown', 'causeAgent' => 'unknown' ]; + $paramsWithTitle = $params + [ 'namespace' => NS_PROJECT, 'title' => 'x' ]; + + $job = new RefreshLinksJob( $testPage, $params ); + $this->assertEquals( $testPage->getPrefixedText(), $job->getTitle()->getPrefixedText() ); + $this->assertSame( $testPage, $job->getTitle() ); + $this->assertJobParamsMatch( $job, $paramsWithTitle ); + $this->assertSame( $testPage, $job->getTitle() ); + + $job = Job::factory( 'refreshLinks', $testPage, $params ); + $this->assertEquals( $testPage->getPrefixedText(), $job->getTitle()->getPrefixedText() ); + $this->assertJobParamsMatch( $job, $paramsWithTitle ); + + $job = Job::factory( 'refreshLinks', $paramsWithTitle ); + $this->assertEquals( $testPage->getPrefixedText(), $job->getTitle()->getPrefixedText() ); + $this->assertJobParamsMatch( $job, $paramsWithTitle ); + + $job = Job::factory( 'refreshLinks', $params ); + $this->assertTrue( $blankTitle->equals( $job->getTitle() ) ); + $this->assertJobParamsMatch( $job, $params ); + } + + /** + * @covers Job::factory + * @covers Job::__construct() + */ + public function testJobSignatureTitleBasedIncomplete() { + $testPage = Title::makeTitle( NS_PROJECT, 'x' ); + $blankTitle = Title::makeTitle( NS_SPECIAL, '' ); + $params = [ 'z' => 1, 'causeAction' => 'unknown', 'causeAgent' => 'unknown' ]; + + $job = new RefreshLinksJob( $testPage, $params + [ 'namespace' => 0 ] ); + $this->assertEquals( $blankTitle->getPrefixedText(), $job->getTitle()->getPrefixedText() ); + $this->assertJobParamsMatch( $job, $params + [ 'namespace' => 0 ] ); + + $job = new RefreshLinksJob( $testPage, $params + [ 'title' => 'x' ] ); + $this->assertEquals( $blankTitle->getPrefixedText(), $job->getTitle()->getPrefixedText() ); + $this->assertJobParamsMatch( $job, $params + [ 'title' => 'x' ] ); } + private function assertJobParamsMatch( IJobSpecification $job, array $params ) { + $actual = $job->getParams(); + unset( $actual['requestId'] ); + + $this->assertEquals( $actual, $params ); + } } diff --git a/tests/phpunit/includes/jobqueue/jobs/ClearUserWatchlistJobTest.php b/tests/phpunit/includes/jobqueue/jobs/ClearUserWatchlistJobTest.php index 27cae8aa2f..1a2941d5d2 100644 --- a/tests/phpunit/includes/jobqueue/jobs/ClearUserWatchlistJobTest.php +++ b/tests/phpunit/includes/jobqueue/jobs/ClearUserWatchlistJobTest.php @@ -51,13 +51,9 @@ class ClearUserWatchlistJobTest extends MediaWikiTestCase { $this->setMwGlobals( 'wgUpdateRowsPerQuery', 2 ); JobQueueGroup::singleton()->push( - new ClearUserWatchlistJob( - null, - [ - 'userId' => $user->getId(), - 'maxWatchlistId' => $maxId, - ] - ) + new ClearUserWatchlistJob( [ + 'userId' => $user->getId(), 'maxWatchlistId' => $maxId, + ] ) ); $this->assertEquals( 1, JobQueueGroup::singleton()->getQueueSizes()['clearUserWatchlist'] ); -- 2.20.1