Problem/Motivation

I want to be able to quickly define many different queue worker plugins or write code that uses a unique queue without having to change settings.php each time.

Proposed resolution

Add a simple replacement queue factory (for the "queue" service) and updated the README docs.

The replacement factory can use "/" as a separator for a prefix to specify what service to look for.

Asking the service for the queue "queue_unique/my_work" will give you a Unique Queue with the queue name "my_work"

Remaining tasks

code + test

User interface changes

none

API changes

none

Data model changes

none

CommentFileSizeAuthor
#10 increment-3301657-10.txt2.88 KBpwolanin
#10 3301657-10.patch12.96 KBpwolanin
#8 3301657-8.patch12.56 KBpwolanin
#7 3301657-7.patch11.89 KBpwolanin
#6 3301657-6.patch11.33 KBpwolanin
#5 3301657-5.patch9.93 KBpwolanin
#4 3301657-4.patch9.93 KBpwolanin
#3 3301657-3.patch8.66 KBpwolanin
#2 3301657-2.patch5.01 KBpwolanin

Comments

pwolanin created an issue. See original summary.

pwolanin’s picture

pwolanin commented
Status: Active » Needs work
StatusFileSize
new 3301657-2.patch5.01 KB

POC patch. Still needs tests

pwolanin’s picture

pwolanin commented
Status: Needs work » Needs review
StatusFileSize
new 3301657-3.patch8.66 KB
pwolanin’s picture

pwolanin commented
Issue summary: View changes
StatusFileSize
new 3301657-4.patch9.93 KB

Change "." as prefix separator to "/" as the separator.

Another test method also.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 3301657-5.patch9.93 KB

updated code comments

pwolanin’s picture

pwolanin commented
StatusFileSize
new 3301657-6.patch11.33 KB

Found a bug in the factory, improved test coverage.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 3301657-7.patch11.89 KB

and one more check in the test.

pwolanin’s picture

pwolanin commented
StatusFileSize
new 3301657-8.patch12.56 KB

And another condition in test for good measure

jludwig’s picture

jludwig commented

I don't have anything valuable to add. Some nits:

The read me references Requirement:
+class RequirementEntityUpdateQueueWorker extends QueueWorkerBase {

Also:

In order for your queue to use the Queue Unique with the default queue service

I know is was there already, but the "the" in "the Queue Unique" is awkward.

In the queue factory

   * @param bool $reliable
   *   (optional) TRUE if the ordering of items and guaranteeing every item executes at
   *   least once is important, FALSE if scalability is the main concern. Defaults
   *   to FALSE.

exceeds 80 characters. Try

   *   (optional) TRUE if the ordering of items and guaranteeing every item
   *   executes at least once is important, FALSE if scalability is the main
   *   concern. Defaults to FALSE.

The first line in docblocks is supposed to be one complete sentence, so:

  /**
   * Constructs a new queue. A name prefix can be used to find a queue factory.
   *
   * @param string $name

would be

  /**
   * Constructs a new queue.
   *
   * A name prefix can be used to find a queue factory.
   *
   * @param string $name

Finally, the second comment in ::defaultServiceAndQueueName() needs a full stop:

        // If the name was "queue_unique/mymodule_work" we end with service
        // "queue_unique.database" and queue name "mymodule_work".
pwolanin’s picture

pwolanin commented
StatusFileSize
new 3301657-10.patch12.96 KB
new increment-3301657-10.txt2.88 KB

I think the first line in docblocks just needs to be < 80 chars.

Here are the other fixes, plus an annotation fix. Posting interdiff too.

jludwig’s picture

jludwig commented
Status: Needs review » Reviewed & tested by the community

Correct, I misinterpreted the standard:

All summaries (first lines of docblocks) must be under 80 characters, start with a capital letter, and end with a period (.). They must provide a brief description of what a function does, what a class does, what a file contains, etc.

https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

The patch in #10 looks great! Some notes:

- It's not a breaking change. Sites using this module don't need to change anything and it will keep working as it did before.
- The advanced usage added by the class works as described.
- Appropriate test coverage added for the new functionality + all tests pass w/patch.

pwolanin’s picture

pwolanin commented

An alternate, sometime better, way to swap the service is to define a ServiceProvider class in another module.

I could also add that to the README, or as a follow-up this, module could have it and control whether it operates based on a config setting or some such.

The one I'm using is just this:


namespace Drupal\my_module;

use Drupal\Core\DependencyInjection\ContainerBuilder;
use Drupal\Core\DependencyInjection\ServiceProviderBase;

/**
 * Service Provider to alter services.
 *
 * @see https://www.drupal.org/node/2026959
 */
class MyModuleServiceProvider extends ServiceProviderBase {

  /**
   * {@inheritdoc}
   */
  public function alter(ContainerBuilder $container) {
    $class = 'Drupal\queue_unique\QueueFactory';
    if (class_exists($class) && $container->hasDefinition('queue')) {
      $definition = $container->getDefinition('queue');
      $definition->setClass($class);
    }
  }

}

  • e0ipso committed ba2a5d7 on 8.x-2.x 
    Issue #3301657 by pwolanin, jludwig: Add an alternate QueueFactory...
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Fixed

Merged, and tagged for the 3.0.0 (I moved to semantic release).

I don't know if this will be a heavily used feature, but I'd like to support your use case.

Thanks for the effort you put on this.

Status: Fixed » Closed (fixed)

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