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

[5.8] Fix wrong class being used when eager loading nullable MorphTo with withDefault() #27455

Merged
merged 1 commit into from
Feb 12, 2019

Conversation

bucaneer
Copy link
Contributor

@bucaneer bucaneer commented Feb 8, 2019

Resubmit of #27411, now without failing tests on master.

Fixes #27369 (and probably #24725)

Currently, MorphTo inherits the newRelatedInstanceFor() method from BelongsTo, which ignores the passed $parent argument and instantiates a related model based on the generalized relationship instance (without constraints) produced by the query builder . However, in the case of MorphTo relations, the no-constraint relationship instance does not know anything about related classes, and wrongly defaults to the parent model class. This commit simply makes use of the $parent argument to produce a related instance that is appropriate for the parent model instance.

Pre-existing tests of MorphTo withDefault() had to be modified because they relied on the old behavior of newRelatedInstanceFor() where it would always return the same predefined instance of the related model. Additionally, a new test was added to ensure that issue #27369 is really fixed.

@taylorotwell
Copy link
Member

So, this only actually takes affect if you have a record with NO polymorphic ID but it has a polymorphic type?

@bucaneer
Copy link
Contributor Author

bucaneer commented Feb 8, 2019

Correct. The use case is described in more detail in the associated issues.

@taylorotwell taylorotwell merged commit 5e0bc23 into laravel:master Feb 12, 2019
@bucaneer bucaneer deleted the 5.8_morphto_default branch February 12, 2019 14:35
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.

2 participants