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.7] Fix wrong class being used when eager loading nullable MorphTo with withDefault() #27411

Merged
merged 4 commits into from
Feb 5, 2019

Conversation

bucaneer
Copy link
Contributor

@bucaneer bucaneer commented Feb 4, 2019

Fixes #27369

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.

This is a resubmit of PR #27382 with updated tests. 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.

Justas Lavišius added 4 commits February 1, 2019 14:56
MorphTo::withDefault() now always creates a fresh model instance, so
we can't know the exact output in advance for use in assertSame().
Instead, use assertEquals() to check that two instances of a model
have the same attributes.
@taylorotwell taylorotwell merged commit f6c83df into laravel:5.7 Feb 5, 2019
@bucaneer bucaneer deleted the 5.7_morphto_default branch February 6, 2019 07:46
@driesvints
Copy link
Member

FYI: we reverted this because this conflicted on master. You're free to re-send this to master if you like with passing tests.

@bucaneer
Copy link
Contributor Author

bucaneer commented Feb 7, 2019

The conflict on master is due to the change in 4fd8a16 - renaming BelongsTo::$relation to BelongsTo::$relationName.

I could resubmit this PR to 5.7 using the method BelongsTo::getRelation() instead of BelongsTo::$relation which would pass tests on both 5.7 and master, and then make a separate one-liner PR on master to replace the call to (now deprecated) BelongsTo::getRelation() with BelongsTo::getRelationName(). Would that be OK?

@driesvints
Copy link
Member

@bucaneer I tried to fix it by using the proper name but I was still getting failing tests so I believe the problem lies deeper. I believe this is best solved with a PR to 5.8 as we want to avoid breaking things too much now that we're close to the new release.

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