Refactor QueueFactory to get queue backend service from QueueWorker plugin instead of Settings

Created on 25 October 2016, over 8 years ago
Updated 17 March 2023, almost 2 years ago

Problem/Motivation

The method responsible for instantiating the queue backend service for each queue worker \Drupal\Core\Queue\QueueFactory::get uses settings.php to determine if there is service set for requested worker name: 'queue_reliable_service_' . $name and 'queue_service_' . $name.

This means that if a module implements QueueWorker plugin it has no way of saying that the worker should use different backend service instead of the default database one.

This means that either the module overrides the whole queue factory service or possibly the settings service. Either way it is not pleasant.

A concrete example of where this would be useful is for queue items that process orders (e.g. in commerce). Heavier processes should make use of the queue so they are processed behind the scenes. However, if an order is loaded, it may be important to process those items immediately so the order is loaded 'up to date'. The obvious way to do this is to use a queue backend that stores the order ID in a separate field that can be used to query/filter other queue backend methods (e.g. claimItem).

To achieve this currently requires an entirely separate queue process (which means it wont be picked up by cron, drush queue-run etc) or to override the queue factory or settings service.

Proposed resolution

So instead of having hardcoded variable in settings.php I propose a new attribute is added to the QueueWorker annotation class backend_service. This would default to the current queue.database service if no value would be provided.

In the aforementioned method the queue worker plugin would be loaded(already in memory if called from Cron) and if it exists(I'm not sure if there can be a queue that does not have worker set up) and the service name would be retrieved from the plugin definition.

If there is no worker the default database backend would be used.

Remaining tasks

Decide if this is something we are happy to do.

User interface changes

None.

API changes

This solution is backwards compatible without any disruptive changes to the API, but developers wishing to make use of it can define their back end and annotate their worker to utilise it.

Data model changes

None.

Feature request
Status

Needs work

Version

10.1

Component
Base 

Last updated about 2 hours ago

Created by

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

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States smustgrave

    Moving to NW for the open threads and suggestions made.

  • 🇺🇦Ukraine voleger Ukraine, Rivne

    Needs review

  • 🇺🇸United States smustgrave

    This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

    Code wise everything looks good
    New functions and properties are typehinted
    trigger_errors are call where needed

    Only issue I see is that the link the trigger_error calls should point a change record that shuld be added to this ticket.

    Other then that everything looks good

    Good work!

  • 🇺🇦Ukraine voleger Ukraine, Rivne

    Added CR https://www.drupal.org/node/3338022 , updated links in PR

  • 🇺🇸United States smustgrave

    Thanks! Can you also include in the CR that the QueueFactory now requires QueueWorkerManagerInterface.

    Would search for other CR where parameters are added for wording suggestions.

  • 🇺🇦Ukraine voleger Ukraine, Rivne

    Updated CR.

  • 🇺🇸United States smustgrave

    Think that should be good.

  • 🇬🇧United Kingdom catch

    Left some comments on the MR.

  • So instead of having hardcoded variable in settings.php I propose a new attribute is added to the QueueWorker annotation class backend_service. This would default to the current queue.database service if no value would be provided.

    The issue summary mentions an annotation class, but that's not how the merge request is solving the problem. Is there a way to use an annotation? Or should the solution continue down the path of a member variable/constant?

  • 🇨🇭Switzerland berdir Switzerland

    The definition is the annotation and the MR updates the annotation.

    This would need to update the attribute class now as well.

    I'm not really convinced this is a good idea. I see that there are sometimes use cases where you implement a custom queue plugin and want to use a specific storage at the same time and that currently requires an extra line in settings.php, but I'd argue it's at least as common to use a certain backend for a queue plugin that you didn't implement yourself and this now requires a module with custom code, that might need its own set of environment-specific control to only do that on certain environments. Maybe we support both without deprecating the existing approach, not really a reason to not support both? The cache backend is similar in that cache bins can have a default backend that can be overridden.

  • 🇺🇦Ukraine voleger Ukraine, Rivne

    Settings.php overrides are unsuitable for the modules that try to control the backend of the queue worker.
    I like the idea of keeping the current approach to override the backend service.

    What if, instead of extending attribute/annotation, we introduce an extension point by dispatching an event right after determining the service name?

Production build 0.71.5 2024