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

[11.x] Fix loadCount unwanted retrieved event #51731

Closed

Conversation

imanghafoori1
Copy link
Contributor

@imanghafoori1 imanghafoori1 commented Jun 6, 2024

The #51276 issue mentions an extra model event fired when the loadCount is called. That is, in fact, relevant to all the load families of methods like loadMax, loadMin and etc that internally use loadAggregate.
The retrieved event is fired when the ->get() is called on line 92 and the event can be suppressed by wrapping it around the Model::withoutEvent.

  • This is not backward compatible if an app relies on this flaw. (I do not know if the same solution has been suggested before)
  • The added tests will fail without this change, due to the fired event.

@imanghafoori1 imanghafoori1 force-pushed the fix_load_retrieved_event branch 2 times, most recently from 99d8828 to 5256eca Compare June 7, 2024 15:58
@imanghafoori1 imanghafoori1 force-pushed the fix_load_retrieved_event branch from 5256eca to c0acb32 Compare June 7, 2024 16:10
@taylorotwell
Copy link
Member

I'm not sure this is totally a bug? A model query is made therefore the event is fired?

@imanghafoori1
Copy link
Contributor Author

@taylorotwell This is something totally unexpected as is the result of implementation details of load* methods.
I think Laravel may fire a special type of event dedicated to lazy eager loading instead, so it can be monitored while testing to ensure N+1 problem is avoided.

@sebapastore
Copy link

I have investigated the issue and arrived at the same conclusion as @imanghafoori1 regarding both the problem and its solution.

Additionally, I considered an alternative perspective: it is not necessary to make the query from the parent model; instead, it can be done directly from the related models.

Moreover, the documentation states: "Using the loadCount method, you may load a relationship count after the parent model has already been retrieved." While documentation may evolve, I believe this reflects the intended use of loadCount: the parent model should already be retrieved, thus it will not be retrived again during the loadCount call.

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.

3 participants