Skip to content
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

Dispatch additional events #212

Merged
merged 2 commits into from
Nov 13, 2024
Merged

Dispatch additional events #212

merged 2 commits into from
Nov 13, 2024

Conversation

dwightwatson
Copy link
Collaborator

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

  • Illuminate\Notifications\Events\NotificationSending
  • Illuminate\Notifications\Events\NotificationSent

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.

@dwightwatson dwightwatson merged commit 2685d4f into master Nov 13, 2024
6 of 7 checks passed
@dwightwatson dwightwatson deleted the events branch November 13, 2024 22:05
@gdebrauwer
Copy link
Contributor

@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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants