[5.2+] FirstOrCreate doesn't work as expected

[mnabialek]

In my opinion firstOrCreate method doesn't work as expected. It does not apply only for this method but probably also for other Builder methods.

As example let's use it like this:

$user =  User::firstOrCreate([
    'name' => 'test',
]);

(and assume we have only id and name fields in users table) for table database.

When we run this multiple times, we will always have only one user created with test name.

But now, let's add mutator to User model like so:

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

let's again make table empty.

And what happens now - each time we run the code we have new record created because mutator is not used for verifying the record existence. It might get even worse, because If we has one record before creating mutator, each time we run the code we would have returned this record however in fact it would not be what we expect.

As a quick fix I think in firstOrCreate this piece of code:

if (! is_null($instance = $this->where($attributes)->first())) {
    return $instance;
}

should be changed in something like this:

if (!is_null($instance = $this->where($this->model->newInstance($attributes)->toArray())->first())) {
    return $instance;
} 

what would make the mutators are applied now to verifying records existence.

However it would be a quite breaking change (though I think it's the way it should always work this way) so probably it should be put into upcoming 5.3 release if decided it's a bug.

[themsaid]

I think the mutator shouldn't affect the query, I mean you may decide to change the mutator at some point 'Mr. '.$value instead of 'Mr '.$value for example, if the query to be affected by the mutator the results would be different than expected.

[mnabialek]

Well, I can change it, but in this case I would expect to get record with exact data I would put into database and either I'm getting totally different record or I'm getting created new record over and over instead of using correct one. Now the only way is to apply mutator manually or putting the code I put here outside firstOrCreate and putting result of toArray to firstOrCreate

[themsaid]

I personally believe queries should stay decoupled from mutators, think of all the drama that could happen if you provide name and you find the Builder querying for Mr name instead.

I think for use case FirstOrCreate is not a good choice. Not sure if others think the same though :)

[mnabialek]

Well, but mutators are only kind of helpers. So when I create user object with name test, I expect I have in database record with Mr test and not test. The problem is also like this - I might have perfectly working code using firstOrCreate when I don't use mutator and what happens when I add mutator to some attribute? The code won't work as expected.

Also at the moment:

$user =  User::firstOrCreate([
    'name' => 'test',
]);

and

$user =  User::firstOrCreate([
    'name' => 'Mr test',
]);

would work totally different although they both in theory should return with user Mr test but the first one doesn't.

PS. Obviously the implementation I showed above also should be a bit different - instead of toArray rather getAttributes should be used to not use accessors or other transformations made by toArray method

[miscbits]

ok I just want to get some clarity on this issue or see if Im doing something wrong here. So I tried to replicate what you were saying and here is what happened for me.

1. I created a new project and made the users migration have only name and id. After migrating I opened up tinker

2. I ran $user = App\User::firstOrCreate(['name' => 'test',]);

3. I ran $user2 = App\User::firstOrCreate(['name' => 'test',]);

4. I found $user == $user2 and running the second command didnt create a new instance. This is how firstOrCreate is supposed to work

5. I added public function setNameAttribute($value) to the User model and saved it and restarted tinker.

6. I reran the commands from 2 and 3 and every single time it returned the model that was created earlier and did not create a new one named Mr. Test.

7. I ran the commands again using the name miscbits and every time it created a new row instead of finding that Mr. miscbits already existed and fetched that instead of creating.

Is this the error you were talking about before? Screenshot below of step 6

[miscbits]

I was able to fix this issue with the following code.

    public function firstOrCreate(array $attributes)
    {
        $attributes = $this->model->newInstance()->fill($attributes)->attributesToArray();

        if (! is_null($instance = $this->where($attributes)->first())) {
            return $instance;
        }

        $instance = $this->model->newInstance($attributes);

        $instance->save();

        return $instance;
    }

The attributesToArray() method will get all the attributes after pushing them through any existing mutators. This fixes the values in attributes and the rest of the function can run as normal. This is a screenshot of the results after adding this line to the firstOrCreate method.

[mnabialek]

@miscbits Yes, this is the problem, but I don't know if your implementation is correct. In this case I think attributes will be mutated twice and in the sample mutator I showed it might cause the problem because you can finish with record with Mr Mr test

[miscbits]

hmmm @mnabialek I think I see the issue. The checks here return ok if you return a model but when you create a new one it will mutate the attributes twice. It seems that I need to just make a second variable and not edit the contents of $attributes

[fernandobandeira]

I do agree that this is a strange behavior however calling the mutators may lead to unexpected results. Also this is inconsistent, IMO we should remove the mutators and create a flag like applyMutators() to apply the mutators in all queries this way you would know that if you use that all your methods will be calling your mutators, not just some.

[themsaid]

I don't know, if we apply mutator on querying it'll be a bit strange.

User::whereName('jon')->get();

Will return a user with name Mr. jon instead! Not sure about that.

[themsaid]

Also mutators can be dynamic, right?

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

How would you deal with that?

[fernandobandeira]

@themsaid You're right, we can't apply the mutators when querying however at least firstOrNew and firstOrCreate should be modified to apply the mutators before searching them or not apply the mutators when inserting.

As of now if you have the 'Mr. '.$value mutator if $value exists in the database the mentioned methods will not create a new record as they should.
And there's also the duplication problem.

Also notice that the submitted PR is targeting the 5.2 branch so at least we should close that and target the 5.3 if there will indeed have a change on these functions.

[themsaid]

What about mutators with dynamic assignment?

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

As in the PR $this->model->newInstance($attributes)->attributesToArray(); will return only $value instead of $this->attributes['title'].$value if the title is not provided.

That's why I say the behaviour is not expected at all.

[miscbits]

The first or create method already applies the mutator when inserting hence the glitch that occurs. I think right now the unexpected behavior is the mutator creating an instance when it already exists in the data. I was wondering today if there would be contradictions with getter methods at this point though. Might be worth looking into. Just quickly thinking through it Idk if there is a use case that would be affected by the getter methods but it would be worth testing

[fernandobandeira]

I think that the taylor's suggestion on that PR will also fix the behavior described in your mutator example.

[miscbits]

@fernandobandeira yes I believe that is correct. I just tested it before sending the commit up and didn't get a glitch anymore.

[bulgariamitko]

i have a question about firstOrCreate, is there a way to NOT create a new record when the value is empty or null? I have a value which is an empty string and firstOrCreate its creating an new entry with an empty field for 'name'. my code looks like this
Person::firstOrCreate(['name' => $fetchedPerformance['stageDirector']]);
where $fetchedPerformance['stageDirector'] in this case is empty string => ''
is there a way to not create the entry?