-
Notifications
You must be signed in to change notification settings - Fork 11.8k
[12.x] Add $deleteWhenMissingModels support for queued notifications
#58396
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
base: 12.x
Are you sure you want to change the base?
Conversation
|
Thanks for submitting a PR! Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface. Pull requests that are abandoned in draft may be closed due to inactivity. |
|
|
||
| $shouldDelete = $reflectionClass->getDefaultProperties()['deleteWhenMissingModels'] | ||
| ?? count($reflectionClass->getAttributes(DeleteWhenMissingModels::class)) !== 0; | ||
| if (! $shouldDelete && $class === SendQueuedNotifications::class) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is all this code needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think when we're in handleModelNotFound, the model couldn't be restored from the queue, so we only have the class name, not the actual job instance.
Reflection on SendQueuedNotifications just gives us the default value (false), not what was actually set from the notification. The payload's displayName is the only way to get back to the original notification class at that point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the smallest change I could make to get it working. Happy on any suggestions though!
(though, I'm still confused as to why it worked pre-laravel 12 without this change. I couldn't figure it out)
Edit: I just tried out of curiosity to see if Claude could figure out why it worked previously and here is what it came back with:
commit dc63a5d changed
resolveName()toresolveQueuedJobClass()in Laravel 12. For notifications,resolveName()returned the notification class via displayName, which accidentally made$deleteWhenMissingModelswork. The new method returns the actual job class (SendQueuedNotifications), which broke it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆 I feel called out.
This PR adds support for the
$deleteWhenMissingModelsproperty on queued notifications, allowing notifications to be silently discarded when their dependent models no longer exist.Fixes #58025