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

[9.x] Standardize withCount() & withExists() eager loading aggregates. #41914

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

r-kujawa
Copy link

What does this PR do?

  • Standardizes the way you can eager load your withCount() & withExists() aggregates.
  • Adds asserts to existing tests to support this functionality☝🏼.

Why do we need this?

Before this PR

There are 3 different ways to pass parameters in with...() functions:

  1. Passing relation strings (internally will check is_array($relations) ? $relations : func_get_args()).
  2. Passing an array of relations (with or without a closure as the value).
  3. Passing the first parameter as a relation string and second argument as a closure.

If we compare the with(), withCount() & withExists functions, this is what each of them currently support:

  • with(): 1, 2 & 3.
  • withCount(): 1 & 2.
  • withExists(): 1 (with only 1 relation 😞) & 2.

The reason why I am writing this PR is because I ran into a situation where I supposed that since I was able to do it with one of these methods, why shouldn't I be able to do the same with any of the other 2 methods (that's where I got disappointed 😞).

I did notice that it would probably be best practice and most efficient to have way 1 & 3 removed and only support way 2. But that would result in some breaking changes (LMK if it sounds interesting and I will make a PR for 10.x).

With this PR

The 3 functions mentioned above will support all 3 different ways to pass the params.
🔴 The only caveat I might see is that the with() function includes a second parameter in it's signature (@param string|\Closure|null $callback) while the other 2 don't.

@r-kujawa r-kujawa changed the title Standardize 'withCount()' & withExists() eager loading aggregates. Standardize withCount() & withExists() eager loading aggregates. Apr 11, 2022
@taylorotwell taylorotwell merged commit f5c50ff into laravel:9.x Apr 11, 2022
@nuernbergerA
Copy link
Contributor

@taylorotwell This is actually a breaking change because the example from the docs no longer works now

use Illuminate\Database\Eloquent\Builder;
 
$posts = Post::withCount(['votes', 'comments' => function (Builder $query) {
    $query->where('content', 'like', 'code%');
}])->get();
 
echo $posts[0]->votes_count;
echo $posts[0]->comments_count;

will throw Undefined array key 1

taylorotwell added a commit that referenced this pull request Apr 12, 2022
taylorotwell added a commit that referenced this pull request Apr 12, 2022
r-kujawa added a commit to r-kujawa/framework that referenced this pull request Apr 12, 2022
r-kujawa added a commit to r-kujawa/framework that referenced this pull request Apr 12, 2022
r-kujawa added a commit to r-kujawa/framework that referenced this pull request Apr 12, 2022
@GrahamCampbell GrahamCampbell changed the title Standardize withCount() & withExists() eager loading aggregates. [9.x] Standardize withCount() & withExists() eager loading aggregates. Apr 12, 2022
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