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.3] Fix database factory error when zero is provided #15764

Merged
merged 1 commit into from
Oct 5, 2016
Merged

[5.3] Fix database factory error when zero is provided #15764

merged 1 commit into from
Oct 5, 2016

Conversation

srmklive
Copy link
Contributor

@srmklive srmklive commented Oct 5, 2016

Fixes #15755

@GrahamCampbell
Copy link
Member

👍

@taylorotwell
Copy link
Member

I'm more curious in why when you pass zero before you would get models?

@themsaid
Copy link
Member

themsaid commented Oct 5, 2016

@taylorotwell I think because range(1, 0) returns [1,0]

public function make(array $attributes = [])
{
    if ($this->amount === 1) {
        return $this->makeInstance($attributes);
    }

    return new Collection(array_map(function () use ($attributes) {
        return $this->makeInstance($attributes);
    }, range(1, $this->amount)));
}

@srmklive
Copy link
Contributor Author

srmklive commented Oct 5, 2016

@taylorotwell because passing 0 always create 2 items for a specific model as reported in #15755. If we skip it entirely, one instance of the model is created.

@taylorotwell
Copy link
Member

Would it better to just return an empty Collection instead of throw an exception?

@srmklive
Copy link
Contributor Author

srmklive commented Oct 5, 2016

@taylorotwell yeah i think that will be better.

@srmklive
Copy link
Contributor Author

srmklive commented Oct 5, 2016

@taylorotwell Code updated. An empty collection is returned instead of exception.

@taylorotwell taylorotwell merged commit f37f67a into laravel:5.3 Oct 5, 2016
@bobbybouwmann
Copy link
Contributor

I think the real question is why would you use factory with 0? I mean calling a factory method with 0 will never return any models so it's not logic to call it with 0 right?

@etherealite
Copy link

etherealite commented Oct 6, 2016

Wow, holy man you guys are super duper quick on issues! I figured I'd check back in next week to see if any cared about fixing such a minor issue, but you guys got on it within hours.

@bobbybouwmann The story goes I wanted to generate a random number of dependent records for a set of given parent records. The random range was 0 to 15 children per parent record. I noticed that my $parent->dependants()->save($dependant); call was being passed an array() when the random value was equal to 0 and not the Eloquent\Model instance I was expecting.

Don't ask me why I did it this way. I was working on a project as fast as I could go and this is what I ended up producing.

$parents = factory(App\Parent::class, 5)->create();
$parents->each(function ($parent) {
      $random = rand(0, 15);
      if ($random <= 1) {
             $parent->dependants()->save(factory(App\Dependant::class, $random)->make());
    });

results in

TypeError: Argument 1 passed to Illuminate\Database\Eloquent\Relations\HasOneOrMany::save() 
must be an instance of Illuminate\Database\Eloquent\Model, array given on line 1

@srmklive
Copy link
Contributor Author

srmklive commented Oct 6, 2016

@etherealite Why don't you try this. May fix your issue.

$parents = factory(App\Parent::class, 5)->create();
$parents->each(function ($parent) {
    $random = rand(0, 15);
    if ($random <= 1) {
        $item = factory(App\Dependant::class, $random)->make();

        $parent->dependants()->save($item);
    }
});

@etherealite
Copy link

etherealite commented Oct 6, 2016

Thanks for the tip @srmklive. I solved the issue by making a check $random > 1 call $model->saveMany() and then adding an additional check for $random === 1 that will call $model->save().

This is just experimental code, so I can be as dirty as I would like.

Again, I'm super impressed by the responsiveness and eagerness to contribute displayed in this community.

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