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

[6.x] Add createMany to factoryBuilder #31171

Merged
merged 2 commits into from
Jan 20, 2020
Merged

[6.x] Add createMany to factoryBuilder #31171

merged 2 commits into from
Jan 20, 2020

Conversation

simoebenhida
Copy link
Contributor

Create multiple records with custom data !

Before

factory(User::class)->create([
    'name' => 'Taylor',
]);
factory(User::class)->create([
    'name' => 'John',
]);
factory(User::class)->create([
    'name' => 'Doe',
]);

After

factory(User::class)->createMany([
    [
        'name' => 'Taylor',
    ],
    [
        'name' => 'John'
    ],
    [
        'name' => 'Doe'
    ],
]);

Implementation Notes:
I added createMany method to FactoryBuilder, it loops over create and return an eloquent collection at the end.

Thanks!

@GrahamCampbell GrahamCampbell changed the title add createMany to factoryBuilder [6.x] Add createMany to factoryBuilder Jan 19, 2020
@browner12
Copy link
Contributor

browner12 commented Jan 20, 2020

I don't think the "before" is even that bad. It's even fewer LOC.

Or to really shorten it, you could do something like this:

foreach(['Taylor', 'John', 'Doe'] as $name)
{
    factory(User::class)->create(['name' => $name]);
}

@simoebenhida
Copy link
Contributor Author

@browner12 it could be multiple columns !

@robmpreston
Copy link

robmpreston commented Jan 20, 2020

I don't the the "before" is even that bad. It's even fewer LOC.

Or to really shorten it, you could do something like this:

foreach(['Taylor', 'John', 'Doe'] as $name)
{
    factory(User::class)->create(['name' => $name]);
}

This is really surface level thinking, IMO. That's just one example and there are many where I think this would simplify things. It seems along the lines of the recent firstWhere addition that was added to Eloquent. I'm in favor. 👍

@browner12
Copy link
Contributor

I'm also not a fan of firstWhere 😓

It does not simplify things. It's just a different way to accomplish the same goal.

It doesn't significantly reduce lines of code. It does not make the code more readable. It does not make the code more testable. It does not make the code more performant. It's simply different.

If you have an example of how it truly simplifies something, would love to hear it so I can reconsider my thinking.

--

If you multiple columns try this pattern I've used before:

$users = [
    'John', 'Doe', 'jdoe@gmail.com',
    'Bob', 'Doe', 'bdoe@gmail.com',
    'Pete', 'Doe', 'pdoe@gmail.com',
];

foreach($users as $user) {
    factory(User::class)->create([
        'first_name' => $user[0],
        'last_name' => $user[1],
        'email' => $user[2],
    ]);
}

@taylorotwell taylorotwell merged commit 41c40bc into laravel:6.x Jan 20, 2020
@shadoWalker89
Copy link
Contributor

@browner12 I must say i was surprised that taylor accepted the firstWhere and didn't say that should be added on the PR's author project IMHO where('foo', 'bar')->first() is much readable, but this PR is good. createMany is so much readable then making a foreach loop.
Inserting multiple rows at once is supported by every SGBD and creating something for tests for databases should not be any different.

@browner12
Copy link
Contributor

Agree to disagree. 😄

@devcircus
Copy link
Contributor

I use to push back on all the helpers and such that remove a few keystrokes to do something that is already simple but so many feel differently. Doesn't change my mind but I gave up trying to keep the clutter out of the codebase.

@browner12
Copy link
Contributor

I've got a little fight left in me. 😄

Really what I'm looking for is for people to justify their PR, not just to say "it simplifies things" and assume that's enough. Tell me why it simplifies things, or show me an example that clearly demonstrates why it's better.

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.

6 participants