[9.x] Added the "for" method to the Eloquent Builder and Model

[ash-jc-allen]

Hey! This PR proposes a new "for" method that can be used on models and in Eloquent queries.

I've marked this as a draft PR for the time being because I'm about half way through doing it. I still need to add the "for" method to the relationships (explained in more detail below). So, I didn't want to spend too much time working on it if you didn't think it would be worth merging in.

Context

This PR is inspired by the whereBelongsTo method that you can use in queries like:

Post::whereBelongsTo($user)->get();

I like how it makes the code a lot more human-readable and avoids writing column names (e.g. - user_id). So, I thought I'd give it a go and see if we could add something similar for creating/updating models.

I chose the "for" name based on the fact that the functionality is similar to the "for" model factories.

Before

Excuse the super basic examples, but they should hopefully demonstrate my general point. Before my proposed method, you could have either of these example statements:

Comment::create([
    'user_id' => $user->id,
    'post_id' => $post->id,
    'content' => 'abc',
]);

Or...

$post->comments()->create([
    'user_id' => $user->id,
    'content' => 'abc',
]);

Or...

$comment->update([
    'user_id' => $anotherUser->id,
    'content' => 'abc',
]);

After

But the problem is that there's always still another ID field in the create method if you're trying to make a relationship to more than one model. So, we could rewrite the queries as:

Comment::for($user)
    ->for(post)
    ->create([
        'content' => 'abc',
    ];

Or...

$post->comments()
    ->for($user)
    ->create([
        'content' => 'abc',
    ]);

$comment->for($anotherUser)->update([
    'content' => 'abc',
]);

At the moment, I've only got the Model::for() and $model->for() approaches working for all the different query methods (as far as I can see, but I might have missed something). I haven't made a start on the relationships (e.g. - $model->relationship()->for()->create()) yet just in case this PR wasn't something that would get merged. If you think that this is something that would be valuable to other devs though, I'll carry on and get the relationships working too.

Personally, I could see this being particularly useful feature and think it'd be a nice way of making our Laravel project codebases even cleaner! 😄

Apologies, by the way, if this sort of logic already exists and I've just completely missed it.

Hopefully, it's heading in a good direction, but it's no problem if you'd rather close the PR 😄

[michaelnabil230]

Nice job 👍🏻

[driesvints]

@ash-jc-allen remember that draft PRs aren't reviewed

[ash-jc-allen]

Hey, @driesvints! Sorry, I completely forgot about that.

I know that you won't review the PR whilst it's a draft, but from looking at the code examples in the PR description, would you say it's worth me carrying on with it? I wouldn't want to spend too much time on it if you don't think it's something that could get merged :)

[driesvints]

Hey @ash-jc-allen, I don't review PR's, only Taylor does. So please mark this as ready if you want a review from him.

[fgaroby]

$this->for being an array, is it really necessary to check its size with count?
foreach is enough, isn't it?

[ash-jc-allen]

Thanks for picking up on that for me. It was something I'd put in when I was first tinkering with another idea. But, I've taken that out now :)

[ash-jc-allen]

I think that this is probably ready now for some kind of review. Apologies that this one has sat as a draft for so long! I've added some basic tests for each of the methods that I've updated too.

I fully expect that I've probably missed something here or overlooked something. But, hopefully, it should be headed in the right direction and might be something that you think would be helpful for other developers.

If it's something that you might consider merging, please let me know if you'd like me to make any changes 😄

[deleugpn]

When I saw the tweet about for on Eloquent I expected something to do with loops. Personally I prefer the explicit definition of the columns, but if this were to be merged I think I would prefer belongingTo(...) than for

[fadymondy]

Awesome 👍

[dillingham]

When I saw the tweet about for on Eloquent I expected something to do with loops. Personally I prefer the explicit definition of the columns, but if this were to be merged I think I would prefer belongingTo(...) than for

Disagree.. a model is a single entity.. not a collection. Also loop is not assumed for factories. I've thought "for" so many times. Happy to see a PR for it

[Ali-Hassan-Ali]

very Goooooooooood

[Haythamasalama]

I think it's a great idea but I think the name method it's good to be by() to avoid misunderstanding

[lepikhinb]

IMO it would be great if the method also called whereBelongsTo, so then we would have a neat syntax for firstOrCreate and similar methods.

$post->views()
    ->for($user)
    ->firstOrCreate([]);

$video->templates()
    ->for($parentTemplate)
    ->firstOrCreate([], [
        'elements' => $parentTemplate->elements,
    ]);

[taylorotwell]

So for methods like firstOrNew - is the foreign key of the for model part of the query to determine if there is an existing model?

[ash-jc-allen]

I've just added some extra assertions to the tests covering firstOrNew to double check it.

As far as I'm aware, the for() won't be used when making queries to fetch data, it'll only ever be used when creating/making/updating. I think my general thinking behind this was because we could relationships (e.g. $post->comments()) or something like whereBelongsTo() to add the constraints when pulling rows from the database.

And then for() can be used specifically for writing/updating. I'm happy to try and change that though if needed :)

[taylorotwell]

I'm just thinking:

Comment::for($user)
    ->for(post)
    ->where('status', 'published')
    ->firstOrCreate([
        'content' => 'abc',
    ]);

It feels a bit ambiguous to me what this query would be. It feels like I would expect it to search for comments that belong to a given user and post that also have a given status and content. But, right now, it seems with this PR in its current state it would search for ANY comment with the given status and content - regardless of the user and post it belongs to.

[ash-jc-allen]

@taylorotwell Yeah the more that I look at your example, the more I agree with that. Based on that, do you want me to put together a quick change for the firstOrCreate method to try this out? :)

[lepikhinb]

@taylorotwell that’s what I was thinking as well. It would probably be cool to have a combined method. But it also shouldn’t be limited to relations.

Say, we have a having method (it’s already reserved, but I can’t think of a better name right now), which would modify all create, update and get queries.

Then the following query:

Comment::query()
    ->having($user)
    ->having('status', 'published')
    ->first();

Would be transformed into:

Comment::query()
    ->where('user_id', $user->id)
    ->where('status', 'published')
    ->first();

And the query as:

Comment::query()
    ->having($user)
    ->having('status', 'published')
    ->create(['content' => $content]);

Will get transformed into:

Comment::query()
    ->create([
        'user_id' => $user->id,
        'status' => 'published',
        'content' => $content,
    ]);

So, this could become a pretty versatile helper for all create / update / firstOr / updateOr / get methods.

[ash-jc-allen]

I've updated this now so that if you use for() in a query (like in your example), it'll add the "for"s as where constraints.

For example:

Comment::for($user)
    ->for(post)
    ->where('status', 'published')
    ->firstOrCreate([
        'content' => 'abc',
    ]);

This would attempt to find a comment that belongs to $user and belongs to $post. If it can't find one, it'll create a post that belongs to $user and belongs to $post.

I started adding the "for" functionality to the get() and first() methods, but didn't know if they were needed, so I reverted them back for the time being.

Hopefully, this should be working as expected 🙂

[taylorotwell]

@ash-jc-allen can you summarize where you can and can't use for based on the current state of this PR? I've gotten a bit lost on what methods support it, what relationships support it, etc.

[ash-jc-allen]

Yeah, I've got a bit lost too because I've not looked at it in a few days. As far as I'm aware, it affects the following methods:

Builder/Model

make

Comment::for($user)->make(...);

create

Comment::for($user)->create(...);

forceCreate

Comment::for($user)->forceCreate(...);

update (the for relationship is not applied to the search. It's only used for setting data that will be stored in the DB)

Comment::for($user)->update(...);

firstOrNew (the for relationship is also applied to the search for the "first")

Comment::for($user)->firstOrNew([], []);

firstOrCreate (the for relationship is also applied to the search for the "first")

Comment::for($user)->firstOrCreate([], []);

updateOrCreate (the for relationship is also applied to the search for the "update")

Comment::for($user)->updateOrCreate([], []);

HasOneOrMany

make

$post->comments()->for($user)->make([...]);

create

$post->comments()->for($user)->create([...]);

forceCreate

$post->comments()->for($user)->forceCreate([...]);

update (the for relationship is not applied to the search. It's only used for setting data that will be stored in the DB)

$post->comments()->for($user)->update([...]);

firstOrNew (the for relationship is also applied to the search for the "first")

$post->comments()->for($user)->firstOrNew([...], [...]);

firstOrCreate (the for relationship is also applied to the search for the "first")

$post->comments()->for($user)->firstOrCreate([...], [...]);

updateOrCreate (the for relationship is also applied to the search for the "update")

$post->comments()->for($user)->updateOrCreate([...], [...]);

[taylorotwell]

I kinda think I will hold off on this for now. I worry it will be a bit too implicit and magical as to what is going on and will be hard to remember exactly what is happening in each scenario vs. just actually setting where clauses explicitly in the code.

[ash-jc-allen]

@taylorotwell That's a totally fair point, and I agree. It was a worth a shot and I'm glad I got chance to explore the idea and scratch an itch 😄