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

[9.x] Allow using a model instance in place of nested model factories #44107

Merged
merged 4 commits into from
Sep 13, 2022

Conversation

jessarcher
Copy link
Member

@jessarcher jessarcher commented Sep 13, 2022

For applications with nested model factories, tests and seeders can get a little overwhelming when you want the nested models to all be related to a specific model.

For example, imagine you have User, Team, and Post models where posts belong to a user and a team, and the team also belongs to a user.

The following factory call would create two user records. One for the post itself, and one for the team that the post factory created.

$post = Post::factory()->create();

To get around this, you might write something like the following:

$user = User::factory()->create();
$post = Post::factory()
    ->for($user)
    ->for(Team::factory()->for($user))
    ->create();

That's still pretty nice! But maybe your posts have images that also belong to a team and/or a user. It can get out of control pretty quickly, and this is far from the most complex nesting I've seen.

This PR introduces a new using method for factories that allows you to provide one or more model instances to be used in place of any nested factories for the given models. What's more, it works recursively, automatically passing the model instances to any nested factories.

For our example above, your code would now look something like this:

$user = User::factory()->create();
$post = Post::factory()
    ->using($user)
    ->create();

In this example, the post and the nested team would all be owned by the provided user instance, with no additional records created. Depending on your application, this will either tidy up your factory calls, or it will prevent superfluous models from being created.

Of course, to take advantage of this feature, your factories must specify other factory instances in their definitions. If your factory definition calls the create method on any nested factories, then the created model will be used instead of anything passed to the using method.

@jessarcher jessarcher marked this pull request as ready for review September 13, 2022 08:00
@dillingham
Copy link
Contributor

This is a huge win for cleanliness.

One thought, maybe "using" could be called "provide". Being a vue user of provide, I personally wouldn't overlook "using" and realize "provide" was for all children.

@rodrigopedra
Copy link
Contributor

I prefer to keep ->using() to be consistent on how we define a custom pivot model when defining a belongs to many relationship.

But I agree this PR is a huge time saver.

@taylorotwell
Copy link
Member

I agree that using usually has a different meaning in Laravel.

What do you think about:

Post::factory()
        ->recycle($user)
        ->create();

@taylorotwell taylorotwell merged commit 5bc3c99 into 9.x Sep 13, 2022
@taylorotwell taylorotwell deleted the factory-using branch September 13, 2022 15:38
@sebdesign
Copy link
Contributor

Recycle is not very clear IMO, it could mean that something is trashed and then repurposed, It could be confused with delete/restore. Maybe reuse is more accurate?

@WAZIRI123
Copy link

i think reuse is more clear

@basepack
Copy link

basepack commented Sep 16, 2022

@jessarcher it won't work when following this approach: https://laravel.com/docs/9.x/eloquent-factories#defining-relationships-within-factories

// PostFactory
public function definition()
{
    return [
        'user_id' => User::factory(), 
    ];
}

// in your test
$user = User::factory()->create();
Post::factory()->recycle($user)->create();
Post::factory()->recycle($user)->create();

ray(User::count()); // 2

@jessarcher
Copy link
Member Author

jessarcher commented Sep 18, 2022

Hi @basepack, I'm unable to replicate this. Using your code in a fresh Laravel project, I only have one User at the end.

Are you sure there's only 1 user in your database just before you call the Post factory? And are you sure that your PostFactory is just returning the User factory, without calling the create method on it?

If so, are you able to create a repo that reproduces this issue as it seems like there might be more to it than the code provided?

@martin-ro
Copy link

without calling the create method on it?

That threw me off at first.

@basepack
Copy link

basepack commented Sep 19, 2022

Hi @jessarcher, @martin-ro, no I do not call ->create(), I will post my complete setup here, maybe that will uncover the issue:

// SiteFactory
namespace Database\Factories;

use App\Models\User;
use Faker\Provider\Uuid;
use Illuminate\Database\Eloquent\Factories\Factory;

/**
 * @extends \Illuminate\Database\Eloquent\Factories\Factory<\App\Models\Site>
 */
class SiteFactory extends Factory
{

    public function definition()
    {
        return [
            'uuid' => Uuid::uuid(),
            'domain_name' => $this->faker->domainName,
        ];
    }
}
// EmailaddressFactory
namespace Database\Factories;

use App\Models\Site;
use Illuminate\Database\Eloquent\Factories\Factory;

/**
 * @extends \Illuminate\Database\Eloquent\Factories\Factory<\App\Models\Emailaddress>
 */
class EmailaddressFactory extends Factory
{
    public function definition()
    {
        return [
            'emailaddress' => $this->faker->unique()->safeEmail,
            'site_id' => Site::factory(),
        ];
    }

    public function withEmailaddress(string $emailaddress): self
    {
        return $this
            ->state([
                'emailaddress' => $emailaddress,
            ])
            ->for(
                Site::factory([
                    'domain_name' => str($emailaddress)->after('@')->toString(),
                ])
            );
    }
}
it('test', function () {
    $site = Site::factory()->create([
        'domain_name' => 'example.com',
    ]);

    $adminEmail = Emailaddress::factory()
        ->recycle($site)
        ->withEmailaddress('info@example.com')
        ->create();

    $userEmail = Emailaddress::factory()
        ->recycle($site)
        ->withEmailaddress('user@example.com')
        ->create();

    ray(Site::count()); // 3 <-- should be 1
    ray(Emailaddress::count()); // 2
});

Edit: removed the UserFactory.

@jessarcher
Copy link
Member Author

Thanks @basepack, I was able to replicate it with this setup. It looks like it's the for call in the withEmailaddress state that's triggering the extra site(s) to be created.

I'll dig into this properly tomorrow.

@michal3377
Copy link
Contributor

This is a great feature, making factories much easier to write.
However, could we maybe go 1 step further, allowing for providing multiple models of a given type, which would be then randomly picked? This would allow developers to provide their own, already created pool of models, so they could be reused in all nested relations.
For example, we often see that the User model relates to everything - as an author of some content, comments, reactions, performed operations etc. In more complex applications, that nesting can go deep. In that scenario, much better would be seeding a number of users once and then reusing them in other relations (so we don't create a user only to be the author of a single comment). If that nested reuse could be achieved with a one-liner (which seems to be not far away from what this PR achieves), that would be a game changer.

In my opinion, Eloquent Factories is a great concept and the only thing it lacks is better support for model reuse. That enhancement would fix that and make them much more versatile.

@jessarcher
Copy link
Member Author

Hey @basepack, I've created #44265 to address your issue 👍

@jessarcher
Copy link
Member Author

@michal3377 Feel free to attempt a PR. I can potentially see the benefit in seeders where you might want a handful of users to be used throughout instead of either recycling a single user, or creating a new user for every relationship.

@basepack
Copy link

@jessarcher thanks for that, looks good! 👍

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

10 participants