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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.7] Added loadCount method to eloquent collections #25997

Merged
merged 2 commits into from
Oct 17, 2018
Merged

[5.7] Added loadCount method to eloquent collections #25997

merged 2 commits into from
Oct 17, 2018

Conversation

timacdonald
Copy link
Member

@timacdonald timacdonald commented Oct 8, 2018

This PR introduces the ability to load relationship counts on an Eloquent\Collection. With the query builder you can $query->with($relations) and also $query->withCount($relations), but on the eloquent collection we can only $collection->load($relations). This introduces the ability to $collection->loadCount($relations).

Usually, of course, you would want to be loading these counts when you are retrieving the models from the DB in the first place, i.e. with the query builder as shown above, however in certain circumstances that is not possible. One example is when the model you are retrieving has a polymorphic relation on which you need to dynamically load counts based on the class type.

A real world example of how I would use this in my current app...

I am tracking certain events. An event is something like "Post created" or "Comment created". Each event has an eventable relation that is polymorphic, i.e. it might be a Post or a Comment

class Event extends Eloquent
{
    public function eventable()
    {
        return $this->morphTo();
    }
}

On my events endpoint, I want to show each event and some information about the eventable. The information shown for each eventable item can vary. For a Post it might be the number of comments it has recieved and for a Comment it might be the number of hearts it has received.

Show me the code!

$events = Event::latest()->with('eventable')->paginate();

$groups = $events->map(function ($event) {
    return $event->eventable;
})->groupBy(function ($eventable) {
    return get_class($eventable);
});

$groups[Post::class]->loadCount('comments');
$groups[Comment::class]->loadCount('hearts');

return new EventIndexResponse($events);

Not sure if this was the best example of its use, but it is a real one. Hopefully it would be useful in other situations also.

Under the hood the query is having each called on it, causing the db hits to be chunked. Only the model primary key and relation counts are being retrieved from the DB to keep the payloads and memory impact as low as possible. It is defaulting the the 1000 chunk count.

Some places where this idea has been discussed / requested:


If a few people could do a once over of the test I've introduced that would be awesome - not used to mocking soooo many things 馃槄

Also, if there is anyway to optimise the way I'm doing this, I'm all ears 馃憘

@X-Coder264
Copy link
Contributor

Maybe add an integration test for this? There is so much mocking going on here that the test feels too brittle.

$query = $this->first()->newQueryWithoutRelationships()
->whereKey($this->modelKeys())
->select($this->first()->getKeyName())
->withCount($relations);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use ->withCount(...func_get_args()) and remove line 72.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noice one, much nicer.

@staudenmeir
Copy link
Contributor

staudenmeir commented Oct 8, 2018

This doesn't work for me: You are using ->fill() to set the attribute, but this requires the count column to be in $fillable.

There can be issues with global scopes:

class User extends Model {
    use SoftDeletes;
}

$users = User::withTrashed()->get();
$users->loadCount(...); // Doesn't load soft-deleted models.

Is there a specific reason for using each() instead of get()?

@timacdonald
Copy link
Member Author

timacdonald commented Oct 8, 2018

@staudenmeir there was an ignorant reason, yes, but looking at it with fresh eyes...no, it doesn't give us any benefit chunking the results as they are all gonna stay in memory when we attach the counts to the existing models. I've updated that. Thanks for pointing that out.

You are right about the ->fill() issue, I've adjusted this to use ->forceFill() and also adjusted the query we are using to bypass the global scopes (soft deletes etc.) and ensure we can load the counts on trashed models.

@X-Coder264 I'll add some integration tests as well - all that mocking was freaking me out tbh!

@timacdonald
Copy link
Member Author

timacdonald commented Oct 8, 2018

Integration tests added 馃挭 thanks for the feedback so far on this one. They cover soft deleted items and guarded attributes as mentioned.

@staudenmeir
Copy link
Contributor

staudenmeir commented Oct 11, 2018

Should we also add a Model::loadCount() method?

Of course, there is no performance advantage for a single model, but we also have Model::load() and Model::loadMissing(). And $post->loadCount('comments'); is shorter than this:

$posts->comments_count = $post->comments()->count();

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.

None yet

4 participants