- 🇺🇸United States smustgrave
Moving to NW for the open threads and suggestions made.
- 🇺🇸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 neededOnly 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.
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?