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

[11.x] Added for method to Eloquent models #53610

Closed
wants to merge 2 commits into from

Conversation

ash-jc-allen
Copy link
Contributor

Hey!

This PR proposes a new for method that helps set foreign keys on models. It's a fresh (and simplified!) take on my past attempt from a couple of years ago: #42467

I think a major drawback of the previous approach was that it wasn't obvious whether for would be used in queries, or for setting fields on models. So it got a bit confusing!

In this approach, I've completely ditched the Builder side of things. Although on the surface, it looks like it would have been nice, I think it would have caused more issues than it solved! So, instead, the for method only exists on the Model classes.

So you can call it like so:

use App\Models\Comment;
use App\Models\Post;
use App\Models\User;

// Imagine these are from a request or something instead.
$post = Post::first(); // ID: 123
$user = User::first(); // ID: 456



// Before:
$comment = Comment::create([
    'comment' => 'hello',
    'post_id' => $post->id,
    'user_id' => $user->id,
]);



// After (using the "for" method):
$comment = Comment::make(['comment' => 'hello'])
    ->for($post)
    ->for($user);

$comment->save();

Assuming there is a posts and users relationship, the comment will now have the following data set:

'comment' => 'hello'
'post_id' => 123,
'user_id' => 456,

There's also the ability to specify the relationship name if needed:

Comment::make(['comment' => 'hello'])
    ->for($post, 'blogPost')
    ->for($user)
    ->save();

I also quite like how it almost reads like a human-readable sentence when it's written like this too:

new Comment()
    ->for($post)
    ->for($user)
    ->fill(['comment' => 'hello'])
    ->save();

Hopefully this is something that other devs would also find useful. Although it's a relatively small change, I like the way that it can reduce the likelihood of typos and also uses a similar syntax to model factories.

But I completely understand if it's not something worth merging in. I'd kick myself if I didn't try and scratch the itch though haha!

If it's something you might consider merging, please let me know if there's anything you might want me to change 😄

@Bosphoramus
Copy link

Yes, please! 🤩🤩

@caendesilva
Copy link
Contributor

I love it. The implementation looks simple and easy to maintain, whilst the feature is very useful.

@osbre
Copy link
Contributor

osbre commented Nov 21, 2024

Wouldn't it be better if this was powered by actual relationship method assignments? So

$store->owner()->assign($user);

If I recall correctly

@ahinkle
Copy link
Contributor

ahinkle commented Nov 21, 2024

I can see the benefit, but it feels a bit ambiguous and might raise questions if I came across it while browsing the code—or for juniors who might struggle to decide which approach to use. The existing methods are a bit more explicit in their intent.

Cool idea though!

@ericwoelki
Copy link

Wouldn't it be better if this was powered by actual relationship method assignments? So

$store->owner()->assign($user);

If I recall correctly

You already can do $model->relation()->associate($related) for cases like this. Docs

@osbre
Copy link
Contributor

osbre commented Nov 22, 2024

@ericwoelki right, but we are talking about ->for($related) and its implementation details

@ericwoelki
Copy link

ericwoelki commented Nov 22, 2024

@osbre Fair, I was pointing out that the method is associate() rather than assign(). I agree with you that deferring to it behind the scenes for the proposed for() method could simply its implementation and also has the benefit of populating the relation.

@Mejvas
Copy link

Mejvas commented Nov 22, 2024

This change makes sense because building relations in this way is already implemented in Illuminate\Database\Eloquent\Factories\Factory.

Already valid code for building eloquent models in tests (and best practice how to do it):

$post = Post::first();
$user = User::first();

$comment = Comment::factory()
    ->for($post)
    ->for($user)
    ->make(['comment' => 'hello']);

@morloderex
Copy link
Contributor

It doesn't look like this will work for all relations specifically i am thinking of ManyToMany relations and Morphed relations.

@taylorotwell
Copy link
Member

I think I will hold off on this one for now. A bit worried such a short, common word could already exist in applications and cause breaking changes. I also wonder if you could just use factories.

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.

9 participants