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

[8.x] Add support for attaching existing model instances in factories #35494

Merged
merged 9 commits into from Dec 7, 2020
Merged

[8.x] Add support for attaching existing model instances in factories #35494

merged 9 commits into from Dec 7, 2020

Conversation

bakerkretzmar
Copy link
Contributor

@bakerkretzmar bakerkretzmar commented Dec 4, 2020

TL;DR: adds support for passing Eloquent model instances, or collections of model instances, into the for() and hasAttached() factory methods.

Sometimes when creating models in a test, we need to be able to attach existing model instances to them instead of relying on the relation's factory. Currently the way to do this is to create all the models separately and then relate them manually, or to pass the column name and ID of the existing models into a factory's state or create() method.

This PR makes attaching existing models more straightforward. It's similar to #34539, but focuses on making the for() and hasAttached() methods more flexible and doesn't add any new methods.

BelongsTo / MorphTo

Before

$posts = Post::factory(10)
    ->for(User::factory()->state(['admin' => true]))
    ->create();

// Now we have to retrieve the user through a specific Post

$user = $posts->first()->user;

$this->be($user)->get(/*...*/);

After

$posts = Post::factory(10)
    ->for($user = User::factory()->create())
    ->create();

// Now the user is available in $user

$this->be($user)->get(/*...*/);

BelongsToMany / MorphToMany

Before

$users = User::factory(3)
    ->hasAttached(Role::factory(3), ['admin' => 'Y'])
    ->create();

// Now there are 9 roles, and we have to retrieve them via specific Users

The alternative to this, right now, is to create the users and roles separately and then attach them manually:

$roles = Role::factory(3)->create();
$users = User::factory(3)->create();
$users->each->roles()->attach($roles);

After

$users = User::factory(3)
    ->hasAttached($roles = Role::factory(3)->create(), ['admin' => 'Y'])
    ->create();

// Now there are three roles, three users, each user has the same three roles, and they're available in $users and $roles

@taylorotwell taylorotwell merged commit c9dcaa2 into laravel:8.x Dec 7, 2020
@taylorotwell
Copy link
Member

@bakerkretzmar just wanted to note in your first example there will not be 10 users... there will be 1...

$posts = Post::factory(10)
    ->for(User::factory()->state(['admin' => true]))
    ->create();

@bakerkretzmar
Copy link
Contributor Author

@taylorotwell yep sorry you're right, updated the example! I think I got that mixed up with using factories inside other factory definitions.

@bakerkretzmar
Copy link
Contributor Author

Looks like there's an issue with this, we missed a couple places where we need to check the type to get the class name. Looking into it now and will PR a fix.

@hackel
Copy link
Contributor

hackel commented Dec 9, 2020

@bakerkretzmar One more place that was missed is in the magic method handling for when the relationship name is different than the model name. e.g.

$posts = Post::factory(10)
    ->forOtherUser($user = User::factory()->create())
    ->create();
assert($posts->every(fn ($post) => $post->otherUser->is($user)));

Looks like it's a simple fix for for in \Illuminate\Database\Eloquent\Factories\Factory::__call:

return $this->for($parameters[0] instanceof Factory ? $factory->state($parameters[0] ?? []) : $parameters[0], $relationship);

Not sure if this should also extend to has* methods, since it's not using hasAttached.

@bastien-phi
Copy link
Contributor

@bakerkretzmar I just saw this PR, I was pretty busy the last few months.

I'm glade you took the initial idea from my PR and made it much better ! 🔥

Thanks a lot !

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.

None yet

5 participants