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.5] Completed BelongsToMany Parent Key Change #21044

Merged
merged 2 commits into from
Sep 7, 2017

Conversation

Upperfoot
Copy link
Contributor

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.

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 Upperfoot changed the title Completed BelongsToMany Parent Key Change [5.5] Completed BelongsToMany Parent Key Change Sep 6, 2017
@themsaid
Copy link
Member

themsaid commented Sep 7, 2017

@Upperfoot can you please share more details on what this is fixing? Prefer if you share some code.

@Upperfoot
Copy link
Contributor Author

Upperfoot commented Sep 7, 2017

@themsaid

So for instance I have the following relation, last parameter is my parentKey

$this->belongsToMany(Service::class, 'user_profile_services', 'user_id', 'service_id', 'user_id');

This is my Pivot Table (user_profile_services)
id, user_id, service_id

This is my Parent Table (user_permanent_profiles)
id, user_id, title, description

This is my Related Table (services)
id, title

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.

Copy link
Member

@themsaid themsaid left a 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

@themsaid
Copy link
Member

themsaid commented Sep 7, 2017

@Upperfoot can you please add some integration tests to your PR? in Illuminate\Tests\Integration\Database\EloquentBelongsToManyTest

@taylorotwell taylorotwell merged commit 0a2d871 into laravel:5.5 Sep 7, 2017
@taylorotwell
Copy link
Member

Please make another PR with integration tests.

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.

None yet

3 participants