Framework: ShouldQueue implementation should probably be at Channel level and not Manager.

[crynobone]

Imagine if we have a TeamHasNewUser notification which we would broadcast to everyone under the team, and we have set via() as below:

class TeamHasNewUser extends Notification implements ShouldQueue
{
    public function via($notifiable)
    {
        return ['nexmo', 'database', 'mail'];
    }

    // ...
}

What if during the queue, nexmo and database notification was successful but the mail 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 to nexmo and database before trying to send to mail.

I would personally think we should have dispatch each notification separately if we're using ShouldQueue.

[GrahamCampbell]

👍

[crynobone]

As an example, this is our common issue when using SES for sending notification on 5.2, but since we have retries setup, it does not reach failed_jobs.

[themsaid]

@crynobone If I'm sending to multiple notifiables: Notification::send(User::all(), ...) I wonder what should happen if sending to a user in the middle threw an exception, this will halt execution completely that the notifiables after that one in loop won't receive any.

For example, sending SMS to 5 users:
1 & 2 received successfully
3 had a wrong phone number => driver throws exception
4 & 5 won't recieve any notifications

What might make sense in this situation? I wonder!

[crynobone]

I would personally prefer each notifiable and channel as an individual jobs.

[srmklive]

I agree with @crynobone every notifiable & channel should be setup as an individual job.

[barryvdh]

Shouldn't exceptions be caught to prevent the halting of other channels?
Should we standardise the exception handling, throw a 'FailedNotification' event?

[crynobone]

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 FailedNotification event?

[barryvdh]

No I agree that you would probably want to retry just 1 channel instead of all notifications.
But some Exceptions can't be fixed by just retrying (eg. invalid email, token expired, payment plan expired etc). But that's a different issue; https://github.com/laravel/framework/pull/14874

[crynobone]

But some Exceptions can't be fixed by just retrying (eg. invalid email, token expired, payment plan expired etc).

But that a solution for a different problem all together. Better have that discussion on your PR.

[reinink]

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.

[reinink]

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'];
    }
}

[taylorotwell]

Yeah this sounds good to me. Anyone looked at implementing it?

[reinink]

So I think adding a separate queue() method is actually pretty straight forward. It would mostly require some adjustments in the ChannelManager to check to check if there are any queued channels.

There is also some design decisions that would have to be made. Would we keep the ShouldQueue implementation on the notification object at that point? It might be smart to keep the Queueable trait to help set the connection, queue and delay. Or maybe for backwards compatibility, you could keep ShouldQueue, in which case everything is queued, regardless of if the channel was defined in the via() or queue() method.

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 Queueable trait and ShouldQueue implementation entirely from the notification object. Then, by default all the queued channels would use the default connection, queue and delay. Like this:

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?

[themsaid]

Implemented in https://github.com/laravel/framework/pull/15681

[reinink]

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?

[crynobone]

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.