[8.x] firstOrCreate and firstOrNew should merge attributes correctly

[JayBizzle]

Not sure if this is a bug, but at the same time, it seems like it doesn't work as you'd expect, so thought I'd create this PR for some feedback.

I guess you could argue this is not the correct use case for the firstOrNew() method, e.g. why check for an existing value, then change that value to something new. Either way, it was still unexpected.

Consider the following example...

$company = Company::firstOrNew(
	['name' => 'Does not exist'],
	['name' => 'New Company'],
);

dd($company);

Current Output

=> App\Models\Company {
    name: "Does not exist",
}

Expected Output?

=> App\Models\Company {
    name: "New Company",
}

Same "bug" exists with firstOrCreate()

More details here #38303

// ping @Sirfrummel

[devcircus]

Seems like there should be a check in place that doesn't even allow this. The name "firstOr" has no meaning when doing it this way.

The first parameter not only defines what you're looking for, but also, any additional attributes in the first argument should be used in creating the model if one wasn't found. Any duplicate attributes with different values in the second argument are ignored.

I'd argue there should be a check for duplicate attributes in each argument, with different values, and if found, throw an exception, because it's no longer a "firstOr" situation at that point. 🤷🏻‍♂️

[JayBizzle]

@devcircus yeah, think I agree, the example above seems like misuse of the method, but the result was definitely unexpected

[Sirfrummel]

Thank you for opening the PR. I disagree it's a mis use of the method though. Think of the example code I gave in the discussion:
$project = Project::firstOrCreate([ 'uuid' => $request->uuid ], [ 'uuid' => Str::uuid() ] );
The gist here is that there are completely valid reasons why you want to do a lookup based on provided user input, however you should be able to override that if you want in the CREATE parameters.

What I had to do in my program was an uglier implementation:

 $project = Project::firstOrNew(
        ['uuid' => $request->uuid],
        [
          'name' => $request->name
        ]
      );
      if (!$project->id) {
        $project->uuid = Str::uuid();
        $project->save();
      }

but it would have been super slick if I could just do:
$project = Project::firstOrCreate([ 'uuid' => $request->uuid ], [ 'uuid' => Str::uuid() , 'name' => $request->name] );
for example.
The fact is, there may be some parameters you use to FIND , but you don't actually want to trust them for CREATE (or in this example, have the ability to override).

[derekmd]

If this is accepted (to 9.x?), the same methods of classes HasOneOrMany and BelongsToMany (#37337 to 9.x) should also be updated.

[JayBizzle]

@Sirfrummel You could have written that code like this...

if(! Project::where('uuid', $request->uuid)->exists()) {
	Project::create([
		'uuid' => Str::uuid(),
        'name' => $request->name,
    ]);
}

The question is, was the firstOr*() method the correct tool to choose for this task? I can see why you expected it to work though, hence the discussion in this PR 👍

[JayBizzle]

If this is accepted (to 9.x?), the same methods of classes HasOneOrMany and BelongsToMany (9.x) should also be updated.

Thanks, will look into that once we have some definitive feedback on this 'issue' 👍

And yes, I think this should probably target 9.0

[taylorotwell]

Hmm, yeah, I read the documentation about this method and, although this seems like a strange use-case, it does seem like it should work. Can you update the other methods per @derekmd's comments?

[JayBizzle]

@taylorotwell should I target this at 9.0?

[taylorotwell]

@JayBizzle I'm not sure if that's necessary. As far as I can tell, the method is not following its documented behavior, which would make this a bug fix.

[JayBizzle]

Okay, will keep the target as 8.0

[taylorotwell]

OK - marking this as draft until other locations are updated. Please mark as ready for review when done.

[JayBizzle]

I have updated the firstOr*() methods on the HasOneOrMany relationship.

Just looking at the firstOr*() methods on the BelongsToMany relationship and it seems to me that those methods don't currently behave like their base Model, or HasOneOrMany counterparts. There is no option to pass a seconds array of values that you want to use to create a new instance or persist to the database.

framework/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php

Lines 599 to 606 in a0021ab

	public function firstOrNew(array $attributes)
	{
	if (is_null($instance = $this->where($attributes)->first())) {
	$instance = $this->related->newInstance($attributes);
	}
	return $instance;
	}

framework/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php

Lines 616 to 623 in a0021ab

	public function firstOrCreate(array $attributes, array $joining = [], $touch = true)
	{
	if (is_null($instance = $this->where($attributes)->first())) {
	$instance = $this->create($attributes, $joining, $touch);
	}
	return $instance;
	}

I guess my question is...is it worth updating these methods when the bug doesn't exists and those methods are not functionally the same as the ones found in Builder and HasOneOrMany?

I think if we want those methods to be functionally equivalent, then that is a separate issue.

Unless I am missing something? 😕

Thoughts?

Ping @derekmd @taylorotwell

[derekmd]

Just looking at the firstOr*() methods on the BelongsToMany relationship

The #37337 pull request I linked in my first comment makes the firstOrCreate() breaking change to add a method argument. If this is merged to 8.x, a second master branch 9.x pull request would have to be created to make that method use array_merge() instead of + append.

firstOrNew() would also need a new method argument so that's a 9.x change.

[JayBizzle]

Ah yes, sorry @derekmd I totally missed the link to that PR. It makes sense now. Will do some more work on this tomorrow, thanks 👍

[JayBizzle]

Done the BelongsToMany relation changes in a PR targeting 9.0 here - #38367

[Sirfrummel]

Thank you @JayBizzle for seeing this through!

[jay-youngn]

What's the point?
Why force all developers to accept his habits?
Your feature caused thousands of BUG to projects in the production environment.

This way of coding comes from:
https://github.com/laravel/framework/blob/v5.3.0-RC1/src/Illuminate/Database/Eloquent/Builder.php#L253