Contrib.social

Running cron queues that use a derivative is broken

Open on Drupal.org →
Created on 18 March 2023, over 1 year ago
Updated 3 April 2023, over 1 year ago

Problem/Motivation

In ✨ Add delay to queue suspend Fixed the code for running cron queues has changed. This caused that queue workers that use a deriver cause a fatal error on cron runs.

An error message like the following can occur when running cron:

Drupal\Component\Plugin\Exception\PluginNotFoundException: The "feeds_feed_refresh" plugin does not exist. Valid plugin IDs for Drupal\Core\Queue\QueueWorkerManager are: feeds_feed_refresh:article, feeds_feed_refresh:csv, feeds_feed_refresh:issue, feeds_feed_refresh:products, feeds_feed_refresh:variations, feeds_feed_refresh:xml in Drupal\Core\Plugin\DefaultPluginManager->doGetDefinition() (line 53 of core/lib/Drupal/Component/Plugin/Discovery/DiscoveryTrait.php).

This is how a queue definition from the Feeds module looks like:

'feeds_feed_refresh:article' => array (6) [
        'title' => Drupal\Core\StringTranslation\TranslatableMarkup (5) (
            protected 'string' -> string (30) "Feed refresh: @feed_type_label"
            protected 'arguments' -> array (1) [
                '@feed_type_label' => string (7) "Article"
            ]
            protected 'translatedMarkup' -> null
            protected 'options' -> array (0) []
            protected 'stringTranslation' -> null
        )
        'id' => string (18) "feeds_feed_refresh"
        'cron' => array (1) [
            'time' => integer 60
        ]
        'deriver' => string (46) "Drupal\feeds\Plugin\Derivative\FeedQueueWorker"
        'class' => string (43) "Drupal\feeds\Plugin\QueueWorker\FeedRefresh"
        'provider' => string (5) "feeds"
    ]

The issue is that \Drupal\Core\Cron now picks the queue name from $queue_info['id'], while it previously took the key from the queue array. By my understanding 'id' in the example above is the base plugin ID. And the key on the array is the derivative plugin ID.

Steps to reproduce

  1. Implement a QueueWorker plugin that makes use of a deriver and that uses cron (see code example below).
  2. Implement the deriver plugin (see code example below).
  3. Create an item on the queue (see code example below).
  4. Run cron.

Example QueueWorker plugin:

namespace Drupal\mymodule\Plugin\QueueWorker;

use Drupal\Core\Queue\QueueWorkerBase;

/**
 * A queue worker.
 *
 * @QueueWorker(
 *   id = "myqueue",
 *   title = @Translation("My Queue"),
 *   cron = {"time" = 60},
 *   deriver = "Drupal\mymodule\Plugin\Derivative\QueueWorkerDerivative"
 * )
 */
class MyQueueWorker extends QueueWorkerBase {

  /**
   * {@inheritdoc}
   */
  public function processItem($data) {}

}

Example deriver:

namespace Drupal\mymodule\Plugin\Derivative;

use Drupal\Component\Plugin\Derivative\DeriverBase;

/**
 * Provides separate queue works.
 *
 * @see \Drupal\mymodule\Plugin\QueueWorker\MyQueueWorker
 */
class QueueWorkerDerivative extends DeriverBase {

  /**
   * {@inheritdoc}
   */
  public function getDerivativeDefinitions($base_plugin_definition) {
    $example_data = [
      'foo' => 'Foo',
      'bar' => 'Bar',
    ];

    $derivatives = [];
    foreach ($example_data as $key => $label) {
      $derivatives[$key] = [
        'title' => strtr('My Queue: @label', [
          '@label' => $label,
        ]),
      ] + $base_plugin_definition;
    }

    return $derivatives;
  }

}

Create an item on the queue:

\Drupal::service('queue')->get('myqueue:foo')
  ->createItem([]);

Proposed resolution

Change the implementation of Drupal\Core\Cron::processQueues() so that the queue name is taken from the key of the queues array instead of picking the 'id' property directly.

Remaining tasks

  1. Confirm that this is a bug (and not a misunderstanding of how derivatives should be implemented).
  2. Implement the proposed resolution.
  3. Add a test that demonstrates the bug. Probably the example code from above should be put in a test module in core/modules/system/tests/modules.

User interface changes

API changes

Data model changes

Release notes snippet

🐛 Bug report
Status

Fixed

Version

10.1 ✨

Component
Cron  →

Last updated 2 days ago

No maintainer
Created by

🇳🇱Netherlands MegaChriz

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @MegaChriz
  • First commit to issue fork.
  • @acbramley opened merge request.
  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇦🇺Australia acbramley

    Failing test + fix pushed as separate commits. We were dropping keys at 2 steps - in the array_filter and the array_map. I've removed all of that and imo made this a bit easier to read through by iterating directly on $this->queueManager->getDefinitions()

  • Status changed to RTBC over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States smustgrave

    Thanks @acbramley for getting to this so quickly!

    Change looks good and the test case appears to cover the scenario exactly.

    Verified by install dev branch of the feeds module.
    Setup a simple feed importer that reads a csv and creates an Article with the title mapping.
    Ran cron and got the error

    Applied the patch

    Ran cron again, no error, and my page was created.

  • Comment over 1 year ago →
    🇳🇱Netherlands MegaChriz

    When I run the test without the fix I get indeed the expected test failure:

    Drupal\Tests\system\Kernel\System\CronQueueTest::testQueueWorkerDeriver
    Drupal\Component\Plugin\Exception\PluginNotFoundException: The "cron_queue_test_deriver" plugin does not exist. Valid plugin IDs for Drupal\Core\Queue\QueueWorkerManager are: cron_queue_test_requeue_exception, cron_queue_test_deriver:foo, cron_queue_test_deriver:bar, cron_queue_test_lease_time, cron_queue_test_suspend, cron_queue_test_exception, cron_queue_test_memory_delay_exception, cron_queue_test_database_delay_exception

    Nice that the test checks how many times an item from the queue was processed.

    And the applied fix makes the code of \Drupal\Core\Cron::processQueues() look a lot simpler too.

    And in 📌 Drupal 10.1 compatibility Fixed I checked that with the fix to core applied if this makes the Feeds tests pass on Drupal 10.1.x again and they do!

    RTBC++

  • Comment over 1 year ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • Comment over 1 year ago →
    System Message
    • larowlan → committed f2e1e5c5 on 10.1.x 
      Issue #3348832 by acbramley, MegaChriz, smustgrave: Running cron queues...
  • Status changed to Fixed over 1 year ago
  • Comment over 1 year ago →
    🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Fixed on commit, added a pre-condition just to be safe.

    diff --git a/core/modules/system/tests/src/Kernel/System/CronQueueTest.php b/core/modules/system/tests/src/Kernel/System/CronQueueTest.php
index 4bec9a7e07f..4e2f1e7858c 100644
--- a/core/modules/system/tests/src/Kernel/System/CronQueueTest.php
+++ b/core/modules/system/tests/src/Kernel/System/CronQueueTest.php
@@ -335,6 +335,7 @@ public function testQueueWorkerManagerSafeguard(): void {
    * Tests that cron queues from derivers work.
    */
   public function testQueueWorkerDeriver(): void {
+    $this->assertEquals(0, \Drupal::state()->get(CronQueueTestDeriverQueue::PLUGIN_ID, 0));
     $queue = \Drupal::queue(sprintf('%s:foo', CronQueueTestDeriverQueue::PLUGIN_ID));
     $queue->createItem('foo');

    Verified it still passed locally

    phpunit -c app/core app/core/modules/system/tests/src/Kernel/System/CronQueueTest.php --filter=Deriver
⏳️ Bootstrapped tests in 0 seconds.
🐘 PHP Version 8.1.16.
💧 Drupal Version 10.1.0-dev.
🗄️  Database engine mysql.
PHPUnit 9.6.6 by Sebastian Bergmann and contributors.

Testing Drupal\Tests\system\Kernel\System\CronQueueTest
.                                                                   1 / 1 (100%)

Time: 00:00.535, Memory: 10.00 MB

OK (1 test, 4 assertions)
  • Comment about 1 year ago →
    System Message

    Automatically closed - issue fixed for 2 weeks with no activity.

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024