-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Fix] BelongsToMany sync does't use configured keys #2667
Conversation
This comment was marked as spam.
This comment was marked as spam.
@@ -76,7 +76,7 @@ protected function setWhere() | |||
{ | |||
$foreign = $this->getForeignKey(); | |||
|
|||
$this->query->where($foreign, '=', $this->parent->getKey()); | |||
$this->query->where($foreign, '=', $this->parent->{$this->parentKey}); |
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.
When a custom parentKey
is defined, the custom parentKey
's value must be searched in the foreign keys list instead of _id
.
@@ -116,7 +116,7 @@ public function sync($ids, $detaching = true) | |||
]; | |||
|
|||
if ($ids instanceof Collection) { | |||
$ids = $ids->modelKeys(); | |||
$ids = $this->parseIds($ids); |
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.
As mentioned in #2666, $ids->modelKeys()
returns _id
field or primary key. However, $this->parseIds($ids)
returns relatedKey
which is the primary key by default.
@@ -190,10 +190,10 @@ public function attach($id, array $attributes = [], $touch = true) | |||
|
|||
$query = $this->newRelatedQuery(); | |||
|
|||
$query->whereIn($this->related->getKeyName(), (array) $id); | |||
$query->whereIn($this->relatedKey, (array) $id); |
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.
As I said in the previous comment, relatedKey
is $this->related->getKeyName()
by default but respects our override.
|
||
// Attach the new parent id to the related model. | ||
$query->push($this->foreignPivotKey, $this->parent->getKey(), true); | ||
$query->push($this->foreignPivotKey, $this->parent->{$this->parentKey}, true); |
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.
The $this->parentKey
is replaced with $this->parent->getKey()
to respect the custom parentKey
field.
@@ -255,36 +260,6 @@ public function testBelongsToMany(): void | |||
$this->assertCount(1, $client->users); | |||
} | |||
|
|||
public function testSyncBelongsToMany() |
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 merged this test with testBelongsToManySync
test. because I see no reason to keep them separate and I think it's cleaner now.
Rename skills relationship name to skillsWithCustomRelatedKey.
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.
LGTM.
Thank you @hans-thomas. |
Hi there,
In this PR, I fix #2666 issue. There were other bugs in using a
BelongsToMany
relationship with customrelatedKey
orparentKey
that I will explain in the comments.