-
Notifications
You must be signed in to change notification settings - Fork 11k
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
ShouldQueue implementation should probably be at Channel level and not Manager. #14785
Comments
👍 |
@crynobone If I'm sending to multiple notifiables: For example, sending SMS to 5 users: What might make sense in this situation? I wonder! |
I would personally prefer each notifiable and channel as an individual jobs. |
I agree with @crynobone every notifiable & channel should be setup as an individual job. |
Shouldn't exceptions be caught to prevent the halting of other channels? |
We could do that but how would we retry the queue only on the unsuccessful channel? catching |
No I agree that you would probably want to retry just 1 channel instead of all notifications. |
But that a solution for a different problem all together. Better have that discussion on your PR. |
For what it's worth I also think that all notifiables and channels should be handled in separate queued jobs. What's (potentially) worse than some users not getting the notification would be some users getting the same emails/sms messages/phone call/postcard three times, depending of course on what you have your retry limit set to. This is pretty easy to get around right now at the notifiables level: // Instead of:
Notification::send($users, new SomeNotification());
// Do this:
foreach ($users as $user) {
$user->notify(new SomeNotification());
} This will cause separate jobs to be created for each user, instead of one queued job for all the users. However, there isn't a way to do this for channels. |
I also think it would be great if some channels could be queued and others not. This makes a lot of sense for the database and broadcast channels, which should happen immediately. Here is one idea of how this could be handled. Basically add another "via" method called "queue". Any channels defined in "via" would not be queued, any in "queued" are queued. class MyNotification extends Notification
{
public function via($notifiable)
{
return ['database'];
}
public function queue($notifiable)
{
return ['mail', 'nexmo'];
}
} |
Yeah this sounds good to me. Anyone looked at implementing it? |
So I think adding a separate There is also some design decisions that would have to be made. Would we keep the I'd be inclined to go a step further and have all notifications handed in their own queued job. We'd start by removing both the public function queue($notifiable)
{
// use the default connection, queue and delay
return ['mail', 'nexmo'];
} However, if you wanted to specify the connection, queue or delay on a per channel basis, you could do this using a basic array: public function queue($notifiable)
{
return [
'mail' => [
'connection' => 'sqs',
'queue' => 'high',
'delay' => 60,
],
'mail' => [
// don't set some values to use the defaults
'queue' => 'low',
],
];
} Since each of the notifications would be handled in their own queued job, this level of granularity would be possible. Thoughts? |
Implemented in #15681 |
This is great. However, technically #15681 was only step one (running each notification in their own job). Could we reopen this to address the second part, which is defining which channels should be queued and which should not be? |
Probably based to have that on it's own discussion, as the requirement is slightly different from this. |
Imagine if we have a
TeamHasNewUser
notification which we would broadcast to everyone under the team, and we have setvia()
as below:What if during the queue,
nexmo
anddatabase
notification was successful but themail
server wasn't available for a short period (either unable to make network connection etc). Since we run this on queue (and if we have our worker configured with retries) we would actually retry to send tonexmo
anddatabase
before trying to send tomail
.I would personally think we should have dispatch each notification separately if we're using
ShouldQueue
.The text was updated successfully, but these errors were encountered: