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.

Sign up for GitHub

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

Jump to bottom

All Notification events should use SerializesModels #24350

Closed
hotmeteor opened this issue May 28, 2018 · 2 comments
Closed

All Notification events should use SerializesModels #24350

hotmeteor opened this issue May 28, 2018 · 2 comments

Comments

@hotmeteor
Copy link
Contributor

hotmeteor commented May 28, 2018 •
edited
Loading

  • Laravel Version: 5.6
  • PHP Version: 7.1
  • Database Driver & Version: all

Description:

All Notification events (NotificationSending, NotificationSent, NotificationFailed) should use SerializesModels like BroadcastNotificationCreated does.

Sometimes the $notifiable model has a bunch of extra stuff on it (relationships, etc) added during a flow and is being unnecessary added to queued jobs from the listeners. This has caused issues for our queueing system because the jobs can have huge payloads.

Steps To Reproduce:

Send a notification using a notifiable with a lot of loaded relational data. See your queued listener jobs get huge.
@tillkruss
Copy link
Contributor

Feel free to submit a PR.

@hotmeteor hotmeteor mentioned this issue May 29, 2018
Merged
@hotmeteor
Copy link
Contributor Author

Submitted: #24368

taylorotwell pushed a commit that referenced this issue May 30, 2018
@taylorotwell
Add Queueable, SerializesModels to all notification events (#24368)
3b7a244 
Refs #24350
symfony-splitter pushed a commit to illuminate/notifications that referenced this issue May 30, 2018
@taylorotwell
Add Queueable, SerializesModels to all notification events (#24368)
6d2386c 
Refs laravel/framework#24350
taylorotwell pushed a commit to illuminate/notifications that referenced this issue Sep 17, 2018
@taylorotwell
Add Queueable, SerializesModels to all notification events (#24368)
99dad78 
Refs laravel/framework#24350
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@hotmeteor @tillkruss