Skip to content
This repository has been archived by the owner on Jul 16, 2021. It is now read-only.

New Eloquent method whereHasAndWith #606

Closed
projct1 opened this issue May 24, 2017 · 30 comments
Closed

New Eloquent method whereHasAndWith #606

projct1 opened this issue May 24, 2017 · 30 comments

Comments

@projct1
Copy link

projct1 commented May 24, 2017

If i need use whereHas and append same result through with method, i have to duplicate constraint closure:

$constraint = function($builder) {
    $builder->where('field', 1);
}

Model::whereHas('relation', $constraint)->with(['relation' => $constraint]);

New method will resolve this inconvenience:

Model::whereHasAndWith('relation', $constraint);
@Dylan-DPC-zz
Copy link

Model::whereHas('relation', $constraint)->with(['relation' => $constraint]);

You are just duplicating stuff.. whereHas will work the same way the with does

@projct1
Copy link
Author

projct1 commented May 24, 2017

You dont understand.
I want get all model where field = 1 and i get all relation only where field = 1 too. So, if i write just with('relation'), i will got all relations... I dont need it. So, now i have to use $constraint twice.

@Dylan-DPC-zz
Copy link

no..

you can constraint your eager loads by passing a closure to it
https://laravel.com/docs/5.4/eloquent-relationships#constraining-eager-loads

@projct1
Copy link
Author

projct1 commented May 24, 2017

...
I know... I write this in first message...

@jaripekkala
Copy link

Just to clarify the problem that is indeed real

// The first user
factory(User::class)->create()->posts()->create([
    'title' => 'Foo',
]);

// The second user
factory(User::class)->create()->posts()->create([
    'title' => 'Bar',
]);
$constraint = function ($builder) {
    $builder->where('title', 'Foo');
}

With

  • All users
  • With only posts titled Foo
User::with(['posts' => $constraint])->get();
[
    {
        "id": 1,
        "posts": [
            { "id": 1, "user_id": 1, "title": "Foo" }
        ]
    },
    { 
        "id": 2,
        "posts": []
    }
]

With and whereHas

  • Only users that has posts titled Foo
  • with only posts titled Foo
User::whereHas('posts', $constraint]
    ->with(['posts' => $constraint])
    ->get();
[
    {
        "id": 1,
        "posts": [
            { "id": 1, "user_id": 1, "title": "Foo" }
        ]
    }
]

@projct1
Copy link
Author

projct1 commented May 28, 2017

My suggestion is replace this:

$constraint = function ($builder) {
    $builder->where('title', 'Foo');
}

User::whereHas('posts', $constraint]
    ->with(['posts' => $constraint])
    ->get();

To this:

User::whereHasAndWith('posts', function ($builder) {
    $builder->where('title', 'Foo');
}])->get();

@Dylan-DPC-zz
Copy link

it makes no sense to get items that are not related to the parent.

@projct1
Copy link
Author

projct1 commented May 28, 2017

@Dylan-DPC Dude, u totally dont understand what we want )
I just want shorten duplicate of code.

@projct1
Copy link
Author

projct1 commented May 28, 2017

With new method whereHasAndWith will not necessary duplicate constraint function.

@Dylan-DPC-zz
Copy link

I understood what you want from your example. but it makes no sense to have a function that returns rows that are not related to the parent model.

Also why don't you query the relation from posts side using with? I guess that should solve your issue

@projct1
Copy link
Author

projct1 commented May 28, 2017

We need get users, who has posts only with title Foo and we need load posts to users only with title Foo.
For this target we need use constraint for user (we need users, who has posts only with title Foo) and same constraint for posts (we need load to model user posts only with title Foo).

@jaripekkala
Copy link

// Charge all users that has unpaid payments
// and has a active credit card.

// Users with inactive credit card are dealt 
// with somewhere else.

User::with([
    'payments' => function ($query) {
        $query->where('paid', false);
    },
    'creditCard' => function ($query) {
        $query->where('active', true);
    },
])
->where('can_pay', true)
->get()
->each(function (User $user) {
    $user->charge($user->payments->sum('price'));
});

With the query above you get all the users with can_pay true. There are no guarantee that it has a active credit card and any payments and it will fail.

How would you query this example via payments or creditCard?

I see the proposed whereHasAndWith method here very handy.

@Riari
Copy link

Riari commented May 29, 2017

Why not write a query scope for it?

@Riari
Copy link

Riari commented May 29, 2017

Actually, I'm not sure why this is even necessary. Taking this example:

User::whereHas('posts', $constraint)
    ->with(['posts' => $constraint])
    ->get();

Surely the second use of $constraint is pointless as you're already selecting users that have posts according to $constraint. You should just be able to do this: User::whereHas('posts', $constraint)->with('posts')->get();.

Still, you could turn it into a query scope to make it shorter.

@projct1
Copy link
Author

projct1 commented May 29, 2017

@Riari That's the case! Without constraint for with, laravel gets all post for user regardless constraint in whereHas...

@Dylan-DPC-zz
Copy link

if you consider it a bug, open an issue in laravel/framework as well so that others can investigate

@projct1
Copy link
Author

projct1 commented May 29, 2017

Ok, thanks.

@jaripekkala
Copy link

It's not a bug.

@projct1
Copy link
Author

projct1 commented May 29, 2017

We'll see what people say )

@projct1
Copy link
Author

projct1 commented May 29, 2017

Who votes to adding new method whereHasAndWith?

@jaripekkala
Copy link

https://github.com/salomoni/whereHasAndWith

screen shot 2017-05-29 at 7 57 01 pm

Actual Laravel app that demonstrates the problem. See web.php, welcome.blade.php and DatabaseSeeder if you still can't catch it.

The fourth column is the one we are after here.

Neither of in the middle as you guys very hard try to suggest.

@Riari
Copy link

Riari commented May 29, 2017

No harm in making suggestions that OP apparently didn't try or mention at first.

Anyway, this is easily solved either by writing a query scope or tackling the query from a different angle; in the Users and Posts example, starting with a query builder for Posts makes more sense because the constraining is exclusively being done on that side:

Post::whereTitle('foo')->with('user')->get();

Or if applicable, do both.

The main problem I have with whereHasAndWith is down to semantics. Method names like that start to get unclear unless you're really familiar with Eloquent already. Perhaps onlyWith would be better?

@jaripekkala
Copy link

I agree the method naming could use re-thinking.

The User-Post example was the simplest one I could think. A real life scenario would require couple more relations to load and more where clauses to apply. At some point you are facing the same problem what ever side you look at it.

Also the data structure isn't ideal when queried through posts. And the user data is duplicated if many posts share the same user.

@Riari
Copy link

Riari commented May 29, 2017

Who's to say the examples given so far don't qualify as realistic? Real scenarios aren't always more complex.

This doesn't change the fact that you can just write a query scope. Hell, you could use one to implement this exact feature request yourself.

@barryvdh
Copy link

I've come across this myself. Would like to see this happen. Not sure if it can be just a method to make it easier to re-use the callback, or the actual subquery could be re-used.

@projct1
Copy link
Author

projct1 commented May 29, 2017

@Riari

This doesn't change the fact that you can just write a query scope. Hell, you could use one to implement this exact feature request yourself.

Your proposal obliges you to always use scopes. That can be not always convenient.

@projct1
Copy link
Author

projct1 commented May 29, 2017

@Riari

The main problem I have with whereHasAndWith is down to semantics. Method names like that start to get unclear unless you're really familiar with Eloquent already. Perhaps onlyWith would be better?

In my, everything is clear. Name of method speaks for itself: User::whereHasAndWith('posts', $constraint)
What other ideas for naming?

@Riari
Copy link

Riari commented May 29, 2017

Your proposal obliges you to always use scopes. That can be not always convenient.

Can you give an example? In my mind, this is a perfect example​ of the kind of problem query scopes are designed to resolve: when you're chaining Eloquent methods and repeating code. You can get exactly what you're asking for in this issue by making use of it.

In my, everything is clear. Name of method speaks for itself

It may be clear to you; that doesn't necessarily mean it would be to anyone else. I personally don't think it fits into Laravel conventions. onlyWith is much more concise and conveys the same meaning.

@jaripekkala
Copy link

onlyWith sounds good to me. I'll create PR later this week if nobody then has yet done it.

@frasmage
Copy link

frasmage commented Jun 5, 2017

Since it doesn't looks like Taylor likes this idea, you can add it to your own applications with a simple query scope (perhaps on a abstract parent class or a trait)

public function scopeOnlyWith($q, $relation, $constraint)
{
    $q->whereHas($relation, $constraint)
    ->with([$relation => $constraint]);
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants