[5.2] firstOrCreate will not create new db rows when a model has a mutator

[miscbits]

fixes issue #14649

When using the firstOrCreate function, the attributes passed in will be used to search for the value in the database even if the model has mutators which will change the values that should be in attributes. This change fills a model and then runs the attributesToArray() method to get the true value of the attributes needed to see if this model is the first in the database.

[crynobone]

Wouldn't fill() only works if we un-guard the model?

[miscbits]

yes if someone tries to use variables that aren't fillable, but if that's the case it should not create a new model anyway because those attributes are guarded

[crynobone]

not create a new model anyway because those attributes are guarded

Mass assignment exception doesn't occur when you call save(), it occur within fill() https://github.com/laravel/framework/blob/5.2/src/Illuminate/Database/Eloquent/Model.php#L439-L457

You might want to look at forceFill().

[miscbits]

See the newest commit. Instead of forceFill() I just moved the instance line to the top of the method
$instance = $this->model->newInstance($attributes);
This was already in the method beforehand so it should not change the behavior of the method and still fix the bug that was reported.

[antonkomarev]

Accordingly firstOrCreate method. Why it's not just utilizing firstOrNew method to make an instance and just save it?

public function firstOrCreate(array $attributes)
{
    $instance = $this->firstOrNew($attributes);
    $instance->save();
    return $instance;
}

I understand that it's not related to this PR, but just noticed code duplication.

[fernandobandeira]

👍

This is actually changing the behavior of the function, should it really go to 5.2?

[themsaid]

How would this change handle the case of mutators with dynamic data?

public function setNameAttribute($value)
{
    $this->attributes['name'] = $this->title.'. '.$value;
}

$mutatedAttributes = $this->model->newInstance($attributes)->attributesToArray();

attributesToArray() applies the mutator having title = '' if it wasn't provided already which leads to unexpected results.

[taylorotwell]

I think in general this makes sense as an idea but I don't agree with using attributesToArray here. I think you only need to do ->newInstance($attributes)->getAttributes()...

Running attributesToArray will return the get accessors on all the attributes, which may not be the expected behavior either. You only want to run the set mutators and then get the raw value of the attributes at that point, which getAttributes will give you.

[miscbits]

@taylorotwell I was talking about the possibility of that in the bug report cause I was thinking about it today. I haven't had time to look/test that possibility. I'll send up a new commit momentarily to this PR.

[miscbits]

@fernandobandeira good idea and will do.

[miscbits]

@fernandobandeira nevermind. you're just sending in an ID for the find function, so why would this fix need to be implemented? (unless someone put a mutator on the id of course)

[miscbits]

@themsaid I think the results would be expected really. It might not be if you forget you wrote that mutator but if a dev is aware of the mutators in their model class it should be apparent why results like that may come about. It's still better than the behavior that allows you to create more than one instance of the same object in your database.