-
Notifications
You must be signed in to change notification settings - Fork 11.1k
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.5] Completed BelongsToMany Parent Key Change #21044
Conversation
This resolves the problem of the relation not making use of the parentKey for the parent Models.
This resolves the issue where BelongsToMany.php makes use of parentKey, but this concern (InteractsWithPivotTable) does not make use of parentKey at all, and instead still uses the old $this->parent->getKey() rather than $this->parent->{$this->parentKey} This references this change e4f8884 which has been superceded by 00c2c5a There also requires a change in BelongsToMany to resolve Eager Loading.
@Upperfoot can you please share more details on what this is fixing? Prefer if you share some code. |
So for instance I have the following relation, last parameter is my parentKey
This is my Pivot Table (user_profile_services) This is my Parent Table (user_permanent_profiles) This is my Related Table (services) The last commit solves the problem of loading the relations in correctly in the first place (relations do not pull through the results as expected), the question I pose is why would the where statement use the parent model key (getKey() returns id on the Parent Model) where I specified it to use user_id by the parentKey parameter on belongsToMany - see https://github.com/laravel/framework/pull/21044/files#diff-1da60d0ef7f0ce6900996a80e2e30fa5L442 The first commit solves the problem of matching the relations after eager loading them, again it still tries to make use of getKey() (id) rather than the parentKey I specified (user_id) in the belongsToMany relation, and because of this the results are never matched and no results are returned. I've tested against all my other belongsToMany relations and this does not break anything, but instead solves the above problem I have, it is a simple matter of making use of the parentKey variable where appropriate, and the use of getKey() in the places I have changed are artifacts of the last implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is ok
@Upperfoot can you please add some integration tests to your PR? in |
Please make another PR with integration tests. |
This resolves the issue where BelongsToMany.php makes use of parentKey, but this concern (InteractsWithPivotTable) does not make use of parentKey at all, and instead still uses the old $this->parent->getKey() rather than $this->parent->{$this->parentKey}
This references this change illuminate/database@e4f8884 which has been superceded by illuminate/database@00c2c5a
This also includes a change to the BelongsToMany Relation to allow for the correct loading of Eager Loaded relations by making use of the parentKey for parent models.