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

[8.x] Added ability to cancel notifications immediately prior to sending #37930

Merged
merged 3 commits into from
Jul 6, 2021
Merged

[8.x] Added ability to cancel notifications immediately prior to sending #37930

merged 3 commits into from
Jul 6, 2021

Conversation

gbradley
Copy link
Contributor

@gbradley gbradley commented Jul 6, 2021

This PR makes it easier to cancel notifications immediately prior to sending, without having to register custom event listeners or create job classes.

Use case

Queued or delayed notifications make it possible that data changes between the notification being queued and its job being processed. Sometimes the up-to-date data is no longer compatible with the kind of notification being sent, for example a "DispatchConfirmation" email on an order that has since been cancelled.

Currently the primary way to prevent notifications being dispatched with stale information is to register a custom event listener, listen for the NotificationSending event, and attempt to call user-defined methods on the notification class in order to determine if the notification should be sent or not. Alternatively, a Job can be queued which synchronously dispatches a notification after the current state is checked.

This PR just simplifies this process by checking for the existance of a shouldBeSent() method on the notification and cancelling sending if the method returns anything but true.

Non-breaking changes

The NotificationSending event is still dispatched, so applications using the technique described above will not be affected.

@driesvints driesvints changed the title Added ability to cancel notifications immediately prior to sending. [8.x] Added ability to cancel notifications immediately prior to sending Jul 6, 2021
@ankurk91
Copy link
Contributor

ankurk91 commented Jul 6, 2021

#30070

@gbradley
Copy link
Contributor Author

gbradley commented Jul 6, 2021

@ankurk91 Interesting, I wasn't aware job middlewares could be applied to notifications, but it makes sense. While you could apply middleware for this I would still find it cumbersome, because I'd likely have to define separate middleware classes for each notification class I wanted to work with. The approach in this PR would let me avoid that and keep the logic for each notifcation inside the relevant notification class.

@taylorotwell taylorotwell merged commit 95413a5 into laravel:8.x Jul 6, 2021
chu121su12 pushed a commit to chu121su12/framework that referenced this pull request Jul 7, 2021
…ing (laravel#37930)

* Added ability to cancel notifications immedaitely prior to sending.

* Fixed style

* formatting

Co-authored-by: Graham Bradley <[email protected]>
Co-authored-by: Taylor Otwell <[email protected]>
@davi5e
Copy link

davi5e commented Jul 16, 2021

Just a quick question, does this PR does the same thing Laravel Relevance Ensurer does? As in the code should be interchangeable with minor tweaks?

@gbradley
Copy link
Contributor Author

gbradley commented Jul 16, 2021 via email

@mrpritchett
Copy link

Should the notification mock assertions like assertNotSentTo obey this method? I'm hitting a case where assertNotSentTo returns false because it's not hitting the shouldSend method.

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.

5 participants