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

[5.6] Add 'after' callback for model factories #23495

Merged
merged 5 commits into from
Mar 12, 2018
Merged

[5.6] Add 'after' callback for model factories #23495

merged 5 commits into from
Mar 12, 2018

Conversation

Indemnity83
Copy link
Contributor

@Indemnity83 Indemnity83 commented Mar 11, 2018

This PR implements the ability to add an after callback for model factories. The callback allows the user to define an action to be run after the model is created or made. A common use case for this behavior is to associate a user who is the owner of a group or team with the group or team, or to create an associated profile record for a new user model.

# TeamFactory.php

use Faker\Generator as Faker;

$factory->define(\App\Team::class, function (Faker $faker) {
    return [
        'name' => $faker->company,
        'slug' => $faker->slug(2),
        'owner_id' => function () {
            return factory(\App\User::class)->create()->id;
        },
    ];
});

$factory->after(\App\Team::class, 'create', function(\App\Team $team, Faker $faker) {
   $team->users()->attach($team->owner, ['role' => 'owner']);
});

If the action is omitted from the after function call, 'create' is assumed. Simplified ussage documentation snippet below:

After Callbacks

After callbacks allow you to define additional required logic to run after a model is created by a model factory. For example, your User model might have a required profile model, or you have a group or team model that has an owner but also requires the user to be added to a pivot table.

$factory->after(\App\Team::class, function(\App\Team $team, Faker $faker) {
     $team->users()->attach($team->owner, ['role' => 'owner']);
});

By default after callbacks are run after a model is created; if you want to run a callback after a make() call you can define the action the callback is to be run after as a third argument.

$factory->after(\App\User::class, 'make', function(\App\User $user, Faker $faker) {
     $profile = factory(\App\Profile::class)->make(['user_id' => $user->id]);
     $user->setRelation('profile', $profile);
});

@Indemnity83 Indemnity83 changed the title [5.6] WIP Add 'after' callback for model factories [5.6 Add 'after' callback for model factories Mar 11, 2018
@Indemnity83 Indemnity83 changed the title [5.6 Add 'after' callback for model factories [5.6] Add 'after' callback for model factories Mar 11, 2018
@deleugpn
Copy link
Contributor

I really like this, but I think it would be more relevant if you could show an use-case that cannot be achieved by Eloquent Created Event.

@browner12
Copy link
Contributor

Yah, part of me feels this would be more appropriate to do in your Seeder class. IMO, the Factory class should be responsible only for generating it's single class, and additional logic should be done either with event handlers, as @deleugpn suggested, or in the corresponding Seeder.

@Indemnity83
Copy link
Contributor Author

Hmm, maybe I'm just missing an existing opportunity here. I specifically ran into wanting to do this in testing where I needed to have a team, which has users and belongs to an owner. Existing functionality lets me define the owner of the team when I fake up a team; but that owner isn't a member of the team until they're also added to the pivot table. The team isn't valid without an owner_id. Similarly, the team isn't valid if the owner isn't also a member.

I don't think the Eloquent Created event applies to this situation, unless there is an intuitive way I'm missing to add this event listener during test setup?

I will concede that I can just attach the user to the team as the second line every time I create a new team. But I literally do it every test that has a team, they are directly coupled which is why I think this is appropriate.

@shadoWalker89
Copy link
Contributor

+1
The way i'm doing it know is that i add methods to my test case like createUser(), createPaper(), create...
Which makes the test case feel gross.

I was thinking about making a PR that does the exact same thing :D hope it gets merged, this is really what i have been missing for my tests

@unstoppablecarl
Copy link
Contributor

I like this a lot. The case that is missing is applying it conditionally like with a state. If it is something that should always be done when that model is created use events. I have room into this car where I want to make records and children for tests. The test code ends up with a ton of boilerplate. An API that let you bind after callbacks to a specific state would be awesome. Something like factory(User::class)->states('withPosts')

Shouldn't the action be 'created' ?

@deleugpn
Copy link
Contributor

This is definitely a much cleaner solution than the current approach of having create{Something} methods in the test class. It is also preferred over Seeds since a lot of developers do prefer to have the seeding stage in their test separately.
And a good response to "why not events" is because you definitely wants to fire these relationship (attach/detach/sync) bindings only when using Factory and not when using Eloquent.

@unstoppablecarl
Copy link
Contributor

I get the action names now. I would rather see methods named afterCreate afterMake

@taylorotwell taylorotwell merged commit 889970a into laravel:5.6 Mar 12, 2018
@taylorotwell
Copy link
Member

Made some changes to this. Methods are afterMaking and afterCreating... also allow you to register multiple callbacks per model.

@unstoppablecarl
Copy link
Contributor

I still want to see them per state :-\

@unstoppablecarl
Copy link
Contributor

unstoppablecarl commented Mar 12, 2018

Would you accept a pr that allowed you to set them by state?

@antonkomarev
Copy link
Contributor

This should be reflected in documentation.

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

7 participants