laravel-notification-channels / fcm

Firebase Cloud Messaging (FCM) notifications channel for Laravel
https://laravel-notification-channels.com/
MIT License
495 stars 127 forks source link

Dispatch additional events #212

Closed dwightwatson closed 1 week ago

dwightwatson commented 1 week ago

This updates the channel to dispatch additional Laravel events, providing more hooks to get the FCM responses in apps.

I looked to the Laravel channels for Vonage and Slack for inspiration and was surprised to see they didn't fire these events. So I've inserted them where I think it seems appropriate.

gdebrauwer commented 1 week ago

@dwightwatson These changes cause a breaking change! Before these changes, the framework automatically dispatched the NotificationSent event using the collection of MulticastSendReport objects returned by the send method of the FcmChannel class. With the changes you made, the NotificationSent event is now fired with a SendReport and that multiple times if there are multiple SendReport objects.

Can these changes be reverted? If they need to stay, then a new major version should be tagged. In my opinion, the NotificationSent event should not be fired by the package. Let the framework handle that automatically. It is also weird that multiple NotificationSent events would be triggered when there are multiple SendReport objects in a MulticastSendReport while you only send a single notification class from within your application code.

dwightwatson commented 1 week ago

Thanks and apologies @gdebrauwer. I couldn't understand why I wasn't familiar with these events before, I did not realise they were fired by the framework instead.

I have reverted these changes and will tag the next release to cover them.