Problem/Motivation

The JobTypeInterface has a handleDuplicateJobs() method which can return null. But the Queue entity is not equipped to handle nulls returned by that method.

  /**
   * Handles existing jobs detected as duplicates when enqueuing a new job.
   *
   * A function can be used to execute a range of different strategies with
   * regard to duplicate jobs:
   * - to pass the decision up to the job creator, throw DuplicateJobException;
   * - to enqueue the new job anyway, return the job;
   * - to overwrite the duplicate job, if the backend implements
   *   SupportsDeletingJobsInterface delete the duplicate job on the backend
   *   and return the new job;
   * - to merge the payloads, delete the duplicate job and return the new job
   *   with a modified payload;
   * - to discard the new job and leave the duplicate intact, return NULL.
...
   * @return \Drupal\advancedqueue\JobResult|null
   *   A new job to enqueue on the backend, or null if no new job should
   *   be enqueued.
 ...
   */
  public function handleDuplicateJobs(Job $job, array $duplicates, BackendInterface $backend): ?Job;

Proposed resolution

Harden the queue entity to allow for the possibility that the job plugin wishes to cancel the new job because it's a duplicate.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

Issue fork advancedqueue-3476814

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw opened merge request !21

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Active » Needs review
jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Related issues: +#2918866: Add support for unique jobs
jsacksick’s picture

jsacksick commented

I'm confused by these changes... For example, prepareJob() currently cannot return NULL? In which case can it return a NULL job now?

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented

Yes, Queue::prepareJob() should be able to return NULL, meaning 'discard this job'. This makes sense because prepaeJob() relies on job type plugin's handleDuplicateJobs() method, which has the design feature that it can return nulls, meaning "discard the current because it's the duplicate of an already queued one".

Queue:

  protected function prepareJob(Job $job): ?Job {
    ...
      if ($duplicates = $this->getBackend()->getDuplicateJobs($job)) {
        $job = $job_type_plugin->handleDuplicateJobs($job, $duplicates, $this->getBackend());
      }
    }
    return $job;
  }

JobTypeInterface:

  /**
   * Handles existing jobs detected as duplicates when enqueing a new job.
   *
   * A function can be used to execute a range of different strategies with
   * regard to duplicate jobs:
...
   * - to discard the new job and leave the duplicate intact, return NULL.
...
   * @return \Drupal\advancedqueue\JobResult|null
   *   A new job to enqueue on the backend, or null if no new job should
   *   be enqueued.
   */
  public function handleDuplicateJobs(Job $job, array $duplicates, BackendInterface $backend): ?Job;
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

Plenty of merge conflicts now.

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented

I would like to fix this. Should I make my own fork and make a new MR, or can I somehow gain access to the existing MR to just add a commit to fix the conflicts?

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented

It seems I could just press a button and get access, so I decided to be bold and rebase the branch while solving the conflicts. I hope that's OK :)

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented
Status: Needs work » Needs review
rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented

phpstan complains:

Method Drupal\advancedqueue_test\Plugin\AdvancedQueue\JobType\SkipDuplicates::handleDuplicateJobs() has parameter $duplicates with no value type specified in iterable type array. 🪪  missingType.iterableValue 💡  See: https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type

However, we're using the {@inheritdoc} doc comment, and the interface does have the value type of the array specified, so I'm not sure how to fix this. I don't have much experience with phpstan.

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented

Fixed phpstan warnings. Ready to be merged!