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

[10.x] Fix firstOrNew on HasManyThrough relations #48542

Merged

Conversation

tonysm
Copy link
Contributor

@tonysm tonysm commented Sep 26, 2023

Fixed

  • Fixes the method signature and implementation of the firstOrNew method on HasManyThrough relations. The other implementations of the firstOrNew method all take the unique attributes plus an array of values to be merged on the new instance. This wasn't the case with the HasManyThrough relation. I noticed this as I was trying to identify the root cause on a bug in the updateOrCreate method. I've shared more context here.

@tonysm tonysm changed the title Fix firstOrNew on HasManyThrough relations [10.x] Fix firstOrNew on HasManyThrough relations Sep 26, 2023
@tonysm tonysm marked this pull request as ready for review September 26, 2023 02:41
@tonysm tonysm force-pushed the fix-first-or-new-on-has-many-through branch from 8a2ca69 to d8ad231 Compare September 26, 2023 14:57
@tonysm
Copy link
Contributor Author

tonysm commented Sep 26, 2023

Tests are failing, but they seem unrelated to the changes here. We can wait for a fix to land in the 10.x branch so I can rebase it here and get a green check before merging this one.

@driesvints
Copy link
Member

@tonysm these are fixed on 10.x, can you rebase?

@tonysm tonysm force-pushed the fix-first-or-new-on-has-many-through branch from d8ad231 to aec7422 Compare September 26, 2023 16:15
@tonysm
Copy link
Contributor Author

tonysm commented Sep 26, 2023

@driesvints done!

@taylorotwell taylorotwell merged commit 169c976 into laravel:10.x Sep 26, 2023
19 checks passed
timacdonald pushed a commit to timacdonald/framework that referenced this pull request Sep 27, 2023
* Fix the firstOrNew method on HasManyThrough relations

* Add a test for the firstOrNew method when record exists

* Invert the firstOrNew if statement
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.

3 participants