laravel / slack-notification-channel

Slack Notification Channel for laravel.
https://laravel.com/docs/notifications#slack-notifications
MIT License
864 stars 59 forks source link

fix: do not notify when route is empty #96

Closed TomaszOnePilot closed 3 weeks ago

TomaszOnePilot commented 3 weeks ago

Hello Laravel team 😃 We moved slack-notification-channel from v2 to v3, but not yet applied what needed to use SlackWebApiChannel, so for now we would like to use still SlackWebhookChannel for a while.

We have an issue in production :

TypeError
Illuminate\Notifications\Slack\SlackChannel::buildJsonPayload(): Argument #1 ($message) must be of type Illuminate\Notifications\Slack\SlackMessage, App\Notifications\Messages\SlackMessage given, called in /var/www/app.onepilot.fr/releases/20240816150940/vendor/laravel/slack-notification-channel/src/Slack/SlackChannel.php on line 41

I can see that it was partially fixed here : https://github.com/laravel/slack-notification-channel/pull/87 But it fixed only for false values.

And in some cases we would like for example to specify many channels in laravel Notification's via() method, like for example 👇

Capture d’écran 2024-08-17 à 16 01 35

And let's say that sometimes we want only to notify via email like that 👇

Capture d’écran 2024-08-17 à 16 03 54

The slack notification will fail here because we don't specify the route and we try notifying anyway. For example when you have in your logic a notifiable that in routeNotificationForSlack() (or in routeNotificationFor() because note that AnonymousNotifiable doesn't have routeNotificationForSlack method) can return null, like in AnonymousNotifiable for example 👇

Capture d’écran 2024-08-17 à 16 07 16 Capture d’écran 2024-08-17 à 19 16 47

And null is not striclty equal to false in SlackNotificationRouterChannel's send method, so the script goes on and enter SlackWebApiChannel, while we did not implement it yet (staying with v2 webhook way for now) and in addition we would like here only to notify by email, not slack.

XristMisyris commented 2 weeks ago

Hello,

this release invalidates the following part of documentation

null - which defers routing to the channel configured in the notification itself. You may use the to method when building your SlackMessage to configure the channel within the notification.

aromka commented 2 weeks ago

Indeed this goes against the documentation and breaks the functionality. Slack notifications are no longer sent if you’re defining the channel in the notification itself.

richardnbanks commented 2 weeks ago

I don't mind the change, however this was a breaking change to the expected and documented behaviour.

If routeNotificationForSlack is not explicitly set it will no longer fallback to the channel defined in services.slack.notifications.channel.

As a workaround I add the following:

/**
 * Route notifications for the Slack channel.
 */
public function routeNotificationForSlack(Notification $notification): mixed
{
    return config('services.slack.notifications.channel');
}
Artyomushko commented 1 week ago

Unfortunately, it is a breaking change as already said above. Moved to 3.3.0 right now. But hope this will be fixed

driesvints commented 1 week ago

Thanks all. I reverted this one.