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

[10.x] Standardize withCount() & withExists() eager loading aggregates. #42254

Closed
wants to merge 2 commits into from

Conversation

r-kujawa
Copy link

@r-kujawa r-kujawa commented May 4, 2022

🔴 There has been an attempt to add this functionality in #41914 but got reverted in #41943 due to a bug in the code. I figured I'd modify the function signatures to match the with() function and send it to 10.x instead.

What does this PR do?

  • Standardizes the way you can eager load your withCount() & withExists() aggregates.
  • Adds logic 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 assumed 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.

With this PR

The 3 functions mentioned above will support all 3 different ways to pass the params.

@taylorotwell
Copy link
Member

I don't see a reason to introduce a breaking change the community isn't widely asking for.

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.

2 participants