-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[13.x] Add ability to default queue by class type #58094
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
74b4c22 to
815c3ab
Compare
|
How would this handle things like Notifications where you can define multiple queues inside the class depending on the channel (mail, db, slack, etc) it is delivering to? |
|
@browner12 Thanks for your reply! 🙇 For Notifications do you mean like Details $queue = $notification->queue ?? $this->manager->resolveDefaultQueue($notification); // this is the new stuff it checks if they've set the default the new way
if (method_exists($notification, 'viaQueues')) {
$queue = $notification->viaQueues()[$channel] ?? $queue;
} |
|
Is this really a default if it doesn't apply for all jobs/job types but only specific classes? Can't the base classes (e.g. abstract classes, traits, etc.) define them themselves? If you want to see all of them, wouldn't an artisan command make more sense? |
|
@shaedrich Yeah, I guess they could but..
(I've added this to the main description, as I realise it wasn't the clearest 👍🏻 ) |
But you could extend said classes
It gets these information to one point but takes them away from the classes where the apply to. When you change these classes, you then have to think about changing the service provider as well. Just like with morph maps. But your addition might sure be helpful to some 👍🏻 |
rename trait to follow the actual singleton name more tweaks
dont think we need it
|
Marking this for review to see if this might be of interest. If not, no problem. 🫡 |
no point returning this, not planning on chaining it
|
I do like the overall idea here. Will try to review soon. I think I would call it something like |
|
@jackbayliss I'm curious if this is missing a couple of things. First, queued event listeners. We queue jobs for these in the event dispatcher. Right now it doesn't seem like you can route these by listener class name in Queue::route(). Secondly, and maybe not as important, queued jobs for broadcasted events. |
|
@taylorotwell I'll have a poke about, I'll draft for now and open once I think it's ready for a re-review - thanks for your time on this. |
…k into 13.x-queue-by-class
|
@jackbayliss I'm fixing it now. |
|
@taylorotwell Cheers thank you! |
|
@jackbayliss I've added support for broadcast events and queued listeners. Need help: I've updated the Queue::route([
Foo::class => ['connection', 'queue'],
Bar::class => 'queue',
]);
Queue::route(Foo::class, connection: 'foo', queue: 'bar');Can you update the places we set queue to also set connection everywhere we are using this feature? The trait has a new |
| /** | ||
| * Set the queue route for the given class. | ||
| * | ||
| * @param array|class-string $class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can type the array as well:
| * @param array|class-string $class | |
| * @param array<class-string, string|array{0: string, 1: string}>|class-string $class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll let Big T decide on that one 👍🏻
|
@taylorotwell Hopefully sorted! or at least closer, presume there will be some formatting tweaks, but let me know if anything else needs tweaking - will be around at some point tomorrow after work. 👍🏻 - Thanks again. |
|
Thanks! |
Currently, queue routing in Laravel requires each job, notification, or mailable to explicitly define its queue (e.g. via a
$queueproperty orviaQueues()etc) or extend/ implement / use a class that has it defined, As a result, queue configuration often becomes repetitive and scattered across multiple classes.I tried a basic version of this via #57712 but, Taylor rightly said the DX was off so this is a re-attempt.
This PR introduces a way to register default queues / connection effectively "route" it, based on a queueable object’s class, parent class, interface, or trait
This is my mind has a few use cases:
Defaults can be routed via the
Queuemanager / Facade:This allows you to do:
You can pass in a class, interface, or trait and if the queued class is, extending / implementing or using the class it'll set the default queue / connection to what the user has set. I don't believe it's BC as it's only called if the original queue parameter is null - and anything else ie
viaQueuesstill takes precedent.I've targeted 13.x as it's introduced a new trait - and feels like a big change for a patch release.
Method names etc open to changing.