-
Notifications
You must be signed in to change notification settings - Fork 35
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
Allow for eager loading models #165
base: main
Are you sure you want to change the base?
Conversation
->eachById(function (Model $model) use ($discovered) { | ||
foreach ($discovered['fill'][$model::class][$model->getKey()] as [$event, $target_property]) { | ||
// This let's us set the property even if it's protected | ||
(fn () => $this->{$target_property} = clone $model)(...)->call($event); |
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'm not certain this should be clone
here — there are pros and cons to both approaches. Deserves discussion.
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.
Namely: with clone
you avoid subtle bugs around recursion and side effects (although some of these are addressed in the latest Laravel 11 release). Without clone
, each event will be mutating the same model, which is probably preferable. Otherwise changes in one even won't impact others correctly.
Many events create or update models in their
handle()
method, and up until now, this could cause an N+1 issue if you were operating on a lot of events that all interact with the same model at the same time (either batch operations or replays).This introduces a new
#[EagerLoad]
attribute that you can add to any protected model property of an event (they must be protected because the model shouldn't be serialized with the event data).For example:
When verbs encounters a batch of events that have
EagerLoad
attributes, it'll load them all in a single query and then push the results into the appropriate events.🪄