Better approach for handling sending failures

[themsaid]

@barryvdh just got this Pull Request merged into the core:
laravel/framework#14874

I believe we should fire that event instead of throwing an exception in case of failure, that way we don't halt the script preventing other successful channels from working.

What do you guys think?
@laravel-notification-channels/collaborators

[barryvdh]

Depends on the failure I think.
If it's something that is a configuration error (eg. forgot your key) or a glitch (connection timed out or something), an Exception would probably be okay (if it's queued and would be restarted). (If laravel/framework#14785 is discussed).

In my case it was that we need to handle the response gracefully, eg. it's expected behavior that a notification can fail in some cases. Shouldn't break the app, but should be handled properly.
Exceptions would be hard to catch on the right place, without breaking the flow.

[irazasyed]

Event firing for failure makes sense 👍

Except for critical cases where it needs attention like @barryvdh mentioned, In cases of config error or as such, it makes sense to throw an exception instead of just firing an event. But the problem with queues should still be considered.

So, are we updating the channels or wait for some time?

[JayBizzle]

Are we close to a decision on this. I like what @barryvdh did in his GCM channel

[dwightwatson]

Prepping this on the APN channel: laravel-notification-channels/apn#79

Let anything exceptional fail loudly as it would anyway, and dispatch NotificationFailed events when we can determine that the notification wasn't delivered.

[klimov-paul]

Despite Laravel introduces NotificationFailed event it is never used inside its core. None of the standard notification channels trigger it.
Its usage inside the particular notification channel creates inconsistency with Laravel core.

Also suppressing of the actual notification channel error is not desirable for queued notifications.
See: laravel-notification-channels/twilio#60

[atymic]

It's kind of hard situation - if you're using queues, having the exception thrown is fine - it will be reported and can be retried later.

The issue is for those not using queues, an exception when sending a notification will trigger a fatal error, and break the execution flow (which is generally not desired - it should probably be reported (ie using report()) but not break the flow.

[klimov-paul]

an exception when sending a notification will trigger a fatal error, and break the execution flow (which is generally not desired - it should probably be reported (ie using report()) but not break the flow.

This is debatable. Take "forgot password" usecase, for example: user enters his email and system sends a notification with reset link inside of it. In case sending failed - the whole use case has failed, and you should notify the user about it right away instead of pretending everything is OK and ask him to check his email box.

Same situation with SMS based authentication flow, when a text message with confirmation code is sent to the user's phone number. In case sending failed the whole usecase failed: user unable to log in.

There are some cases, when notification failure can be acceptable, like sending "welcome" to the newly registered users, but this should be handled by developer explicitly.

[atymic]

I guess the issue is really the inconsistency between sync and queued notification. For example, your examples will look fine to the user if they are queued, but fail to send.

Do keep in mind that @themsaid is the one of the laravel core members as well (who is suggesting that an event should be used).

cc @irazasyed @barryvdh @laravel-notification-channels/collaborators for thoughts?

[vonec]

facing a similar situation with fcm notification channel. i think the best way to handle is to have an event fired so that the developer can handle the situation at his will rather than throwing an exception.

[shirshak55]

Most of the cases do use email / sms which is queued anyway. And silencing error is not cool. And worst part is by default it doesn't even get inside log etc like exceptions are dismissed in thin air. Many people might not know they should use event to handle exception so I am completely against channel. Events based error handling routes exception to other listeners etc which can be potential performance problem also.

[vonec]

even then the exceptions should be handled in some way, so its not the point of performance but the choice to identify

Illuminate\Notifications\Events\NotificationFailed is already available and should be implemented

[the-dijkstra]

Having a problem with this NotificationFailed event not being triggered at all. after some research inside the core code. I found out this event is not being implemented at all. just sitting there.

Here is my use case I have a single notification class with multiple channels mail, SMS, database handled by SendQueuedNotifications and I need to track the delivery status for each channel ('SMS delivery status', 'mail delivery status'). the proper way to handle those would be inside NotificationFailed since it's supposed to provide you with:

public function handle(NotificationFailed $event)
{
    // $event->channel
    // $event->notifiable
    // $event->notification
    // $event->response
}

Trying to handle the failure inside the $notification->failed() wouldn't give you access to notifiable nether the channel. so you can't tell in which channel the failure happened.

[the-dijkstra]

@themsaid

don't you think the failed method inside a Notification class should have the following signature:

public function failed(Throwable $e, Mixed $notifiable, string $channel);

instead of

public function failed(Throwable $e);

This will give us all the necessary params to fire a NotificationFailed event from within the notification failed method.

public function failed(Throwable $e, Mixed $notifiable, string $channel) {
   event(new NotificationFailed($notifiable,$this,$channel,[
     'error' => $e->getMessage()
   ]));
}