[8.x] firstOrCreate and firstOrNew should merge attributes correctly
#38346
Conversation
|
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. 🤷🏻♂️ |
|
@devcircus yeah, think I agree, the example above seems like misuse of the method, but the result was definitely unexpected |
|
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: What I had to do in my program was an uglier implementation: but it would have been super slick if I could just do: |
|
If this is accepted (to 9.x?), the same methods of classes |
|
@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 |
Thanks, will look into that once we have some definitive feedback on this 'issue' 👍 And yes, I think this should probably target 9.0 |
firstOrCreate and firstOrNew should merge attributes correctly
|
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? |
|
@taylorotwell should I target this at 9.0? |
|
@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. |
|
Okay, will keep the target as 8.0 |
|
OK - marking this as draft until other locations are updated. Please mark as ready for review when done. |
|
I have updated the Just looking at the framework/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php Lines 599 to 606 in a0021ab framework/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php Lines 616 to 623 in a0021ab 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 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 |
The #37337 pull request I linked in my first comment makes the
|
|
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 👍 |
|
Done the |
|
Thank you @JayBizzle for seeing this through! |
|
What's the point? This way of coding comes from: |
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...
Current Output
Expected Output?
Same "bug" exists with
firstOrCreate()More details here #38303
// ping @Sirfrummel