From fc35b3b37c03c762e8880bb66d998360cec3a8c4 Mon Sep 17 00:00:00 2001 From: Mohamed Said Date: Sun, 5 May 2019 11:29:46 +0200 Subject: [PATCH 1/6] fix many to many sync results with custom pivot model --- .../Concerns/InteractsWithPivotTable.php | 7 +++++- .../Database/EloquentBelongsToManyTest.php | 25 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php index f9fc89ebf845..3bdbb3086187 100644 --- a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php +++ b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php @@ -213,7 +213,12 @@ public function updateExistingPivot($id, array $attributes, $touch = true) */ protected function updateExistingPivotUsingCustomClass($id, array $attributes, $touch) { - $updated = $this->newPivot([ + $updated = (new $this->using)->setTable($this->table)->where([ + $this->foreignPivotKey => $this->parent->{$this->parentKey}, + $this->relatedPivotKey => $this->parseId($id), + ])->first()->fill($attributes)->isDirty(); + + $this->newPivot([ $this->foreignPivotKey => $this->parent->{$this->parentKey}, $this->relatedPivotKey => $this->parseId($id), ], true)->fill($attributes)->save(); diff --git a/tests/Integration/Database/EloquentBelongsToManyTest.php b/tests/Integration/Database/EloquentBelongsToManyTest.php index 64ecdf4a34ce..ac6822d17372 100644 --- a/tests/Integration/Database/EloquentBelongsToManyTest.php +++ b/tests/Integration/Database/EloquentBelongsToManyTest.php @@ -154,6 +154,31 @@ public function test_custom_pivot_class() $this->assertEquals(2, PostTagPivot::first()->tag_id); } + public function test_custom_pivot_class_using_sync() + { + Carbon::setTestNow('2017-10-10 10:10:10'); + + $post = Post::create(['title' => Str::random()]); + + $tag = TagWithCustomPivot::create(['name' => Str::random()]); + + $results = $post->tagsWithCustomPivot()->sync([ + $tag->id => ['flag' => 1] + ]); + + $this->assertNotEmpty($results['attached']); + + $results = $post->tagsWithCustomPivot()->sync([ + $tag->id => ['flag' => 1] + ]); + + $this->assertEmpty($results['updated']); + + $results = $post->tagsWithCustomPivot()->sync([]); + + $this->assertNotEmpty($results['detached']); + } + public function test_attach_method() { $post = Post::create(['title' => Str::random()]); From 589a5352970cf4045b1875aed48ac682b9f113f2 Mon Sep 17 00:00:00 2001 From: Mohamed Said Date: Sun, 5 May 2019 11:42:40 +0200 Subject: [PATCH 2/6] fix style --- tests/Integration/Database/EloquentBelongsToManyTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/Integration/Database/EloquentBelongsToManyTest.php b/tests/Integration/Database/EloquentBelongsToManyTest.php index ac6822d17372..3e87dea09fed 100644 --- a/tests/Integration/Database/EloquentBelongsToManyTest.php +++ b/tests/Integration/Database/EloquentBelongsToManyTest.php @@ -163,13 +163,13 @@ public function test_custom_pivot_class_using_sync() $tag = TagWithCustomPivot::create(['name' => Str::random()]); $results = $post->tagsWithCustomPivot()->sync([ - $tag->id => ['flag' => 1] + $tag->id => ['flag' => 1], ]); $this->assertNotEmpty($results['attached']); $results = $post->tagsWithCustomPivot()->sync([ - $tag->id => ['flag' => 1] + $tag->id => ['flag' => 1], ]); $this->assertEmpty($results['updated']); From 5b7657a15f3e8802b7ddc2d728a7998b72877930 Mon Sep 17 00:00:00 2001 From: Mohamed Said Date: Sun, 5 May 2019 16:38:28 +0200 Subject: [PATCH 3/6] load current models once --- .../Concerns/InteractsWithPivotTable.php | 38 ++++++++++++++----- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php index 3bdbb3086187..3a9891c282bb 100644 --- a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php +++ b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php @@ -4,10 +4,16 @@ use Illuminate\Database\Eloquent\Model; use Illuminate\Database\Eloquent\Collection; +use Illuminate\Database\Eloquent\Relations\Pivot; use Illuminate\Support\Collection as BaseCollection; trait InteractsWithPivotTable { + /** + * @var Collection + */ + private $current; + /** * Toggles a model (or models) from the parent. * @@ -88,11 +94,9 @@ public function sync($ids, $detaching = true) // First we need to attach any of the associated models that are not currently // in this joining table. We'll spin through the given IDs, checking to see // if they exist in the array of current ones, and if not we will insert. - $current = $this->newPivotQuery()->pluck( - $this->relatedPivotKey - )->all(); + $currentKeys = $this->getCurrent()->pluck($this->relatedPivotKey)->all(); - $detach = array_diff($current, array_keys( + $detach = array_diff($currentKeys, array_keys( $records = $this->formatRecordsList($this->parseIds($ids)) )); @@ -109,7 +113,7 @@ public function sync($ids, $detaching = true) // touching until after the entire operation is complete so we don't fire a // ton of touch operations until we are totally done syncing the records. $changes = array_merge( - $changes, $this->attachNew($records, $current, false) + $changes, $this->attachNew($records, $currentKeys, false) ); // Once we have finished attaching or detaching the records, we will see if we @@ -213,10 +217,12 @@ public function updateExistingPivot($id, array $attributes, $touch = true) */ protected function updateExistingPivotUsingCustomClass($id, array $attributes, $touch) { - $updated = (new $this->using)->setTable($this->table)->where([ - $this->foreignPivotKey => $this->parent->{$this->parentKey}, - $this->relatedPivotKey => $this->parseId($id), - ])->first()->fill($attributes)->isDirty(); + $current = $this->getCurrent() + ->where($this->foreignPivotKey, $this->parent->{$this->parentKey}) + ->where($this->relatedPivotKey, $this->parseId($id)) + ->first(); + + $updated = $current->fill($attributes)->isDirty(); $this->newPivot([ $this->foreignPivotKey => $this->parent->{$this->parentKey}, @@ -230,6 +236,20 @@ protected function updateExistingPivotUsingCustomClass($id, array $attributes, $ return (int) $updated; } + /** + * Get the existing records. + * + * @return \Illuminate\Support\Collection + */ + protected function getCurrent() + { + return $this->current ?: $this->newPivotQuery()->get()->map(function($record){ + $class = $this->using ? $this->using : Pivot::class; + + return (new $class)->setRawAttributes((array) $record, true); + }); + } + /** * Attach a model to the parent. * From 47a35c188b72edf85e8f5492578c740b22c0ad8e Mon Sep 17 00:00:00 2001 From: Mohamed Said Date: Sun, 5 May 2019 16:39:10 +0200 Subject: [PATCH 4/6] fix style --- .../Concerns/InteractsWithPivotTable.php | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php index 3a9891c282bb..f0b943d4125b 100644 --- a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php +++ b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php @@ -236,20 +236,6 @@ protected function updateExistingPivotUsingCustomClass($id, array $attributes, $ return (int) $updated; } - /** - * Get the existing records. - * - * @return \Illuminate\Support\Collection - */ - protected function getCurrent() - { - return $this->current ?: $this->newPivotQuery()->get()->map(function($record){ - $class = $this->using ? $this->using : Pivot::class; - - return (new $class)->setRawAttributes((array) $record, true); - }); - } - /** * Attach a model to the parent. * @@ -659,4 +645,18 @@ protected function getTypeSwapValue($type, $value) return $value; } } + + /** + * Get the existing records. + * + * @return \Illuminate\Support\Collection + */ + protected function getCurrent() + { + return $this->current ?: $this->newPivotQuery()->get()->map(function ($record) { + $class = $this->using ? $this->using : Pivot::class; + + return (new $class)->setRawAttributes((array) $record, true); + }); + } } From 3b765708e7ae7dcbf4081198c48c61d20eafc033 Mon Sep 17 00:00:00 2001 From: Mohamed Said Date: Sun, 5 May 2019 16:40:05 +0200 Subject: [PATCH 5/6] less changes --- .../Eloquent/Relations/Concerns/InteractsWithPivotTable.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php index f0b943d4125b..2dba48e02d15 100644 --- a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php +++ b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php @@ -94,9 +94,9 @@ public function sync($ids, $detaching = true) // First we need to attach any of the associated models that are not currently // in this joining table. We'll spin through the given IDs, checking to see // if they exist in the array of current ones, and if not we will insert. - $currentKeys = $this->getCurrent()->pluck($this->relatedPivotKey)->all(); + $current = $this->getCurrent()->pluck($this->relatedPivotKey)->all(); - $detach = array_diff($currentKeys, array_keys( + $detach = array_diff($current, array_keys( $records = $this->formatRecordsList($this->parseIds($ids)) )); @@ -113,7 +113,7 @@ public function sync($ids, $detaching = true) // touching until after the entire operation is complete so we don't fire a // ton of touch operations until we are totally done syncing the records. $changes = array_merge( - $changes, $this->attachNew($records, $currentKeys, false) + $changes, $this->attachNew($records, $current, false) ); // Once we have finished attaching or detaching the records, we will see if we From 35f6cc82a4d1affbc8b6f731403eec35ae7ef3c7 Mon Sep 17 00:00:00 2001 From: Mohamed Said Date: Sun, 5 May 2019 16:41:25 +0200 Subject: [PATCH 6/6] refactor --- .../Relations/Concerns/InteractsWithPivotTable.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php index 2dba48e02d15..9550053ef946 100644 --- a/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php +++ b/src/Illuminate/Database/Eloquent/Relations/Concerns/InteractsWithPivotTable.php @@ -217,12 +217,12 @@ public function updateExistingPivot($id, array $attributes, $touch = true) */ protected function updateExistingPivotUsingCustomClass($id, array $attributes, $touch) { - $current = $this->getCurrent() - ->where($this->foreignPivotKey, $this->parent->{$this->parentKey}) - ->where($this->relatedPivotKey, $this->parseId($id)) - ->first(); - - $updated = $current->fill($attributes)->isDirty(); + $updated = $this->getCurrent() + ->where($this->foreignPivotKey, $this->parent->{$this->parentKey}) + ->where($this->relatedPivotKey, $this->parseId($id)) + ->first() + ->fill($attributes) + ->isDirty(); $this->newPivot([ $this->foreignPivotKey => $this->parent->{$this->parentKey},