From 892c9ba512087c5daa9dfafdf0a79d1d4a0a03fa Mon Sep 17 00:00:00 2001 From: mpyw Date: Mon, 28 Aug 2023 10:58:42 +0900 Subject: [PATCH 1/4] Make the `updateOrCreate` methods in relations use `firstOrCreate` Related to #48160, #48192 --- .../Eloquent/Relations/BelongsToMany.php | 18 ++++++------------ .../Eloquent/Relations/HasManyThrough.php | 10 +++++----- .../Eloquent/Relations/HasOneOrMany.php | 8 ++++---- 3 files changed, 15 insertions(+), 21 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php b/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php index b2e494478211..51d723b736f4 100755 --- a/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php +++ b/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php @@ -672,19 +672,13 @@ public function createOrFirst(array $attributes = [], array $values = [], array */ public function updateOrCreate(array $attributes, array $values = [], array $joining = [], $touch = true) { - if (is_null($instance = (clone $this)->where($attributes)->first())) { - if (is_null($instance = $this->related->where($attributes)->first())) { - return $this->create(array_merge($attributes, $values), $joining, $touch); - } else { - $this->attach($instance, $joining, $touch); - } - } - - $instance->fill($values); + return tap($this->firstOrCreate($attributes, $values), function ($instance) use ($values) { + if (! $instance->wasRecentlyCreated) { + $instance->fill($values); - $instance->save(['touch' => false]); - - return $instance; + $instance->save(['touch' => false]); + } + }); } /** diff --git a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php index ac5037185793..c0307e824af3 100644 --- a/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasManyThrough.php @@ -271,11 +271,11 @@ public function firstOrNew(array $attributes) */ public function updateOrCreate(array $attributes, array $values = []) { - $instance = $this->firstOrNew($attributes); - - $instance->fill($values)->save(); - - return $instance; + return tap($this->firstOrCreate($attributes, $values), function ($instance) use ($values) { + if (! $instance->wasRecentlyCreated) { + $instance->fill($values)->save(); + } + }); } /** diff --git a/src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php b/src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php index c748dd7c644d..9847a800bcb8 100755 --- a/src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php +++ b/src/Illuminate/Database/Eloquent/Relations/HasOneOrMany.php @@ -267,10 +267,10 @@ public function createOrFirst(array $attributes = [], array $values = []) */ public function updateOrCreate(array $attributes, array $values = []) { - return tap($this->firstOrNew($attributes), function ($instance) use ($values) { - $instance->fill($values); - - $instance->save(); + return tap($this->firstOrCreate($attributes, $values), function ($instance) use ($values) { + if (! $instance->wasRecentlyCreated) { + $instance->fill($values)->save(); + } }); } From 5e1d32f0c6f0501200506c850063e68704282a68 Mon Sep 17 00:00:00 2001 From: mpyw Date: Mon, 28 Aug 2023 12:04:44 +0900 Subject: [PATCH 2/4] Fix tests --- tests/Database/DatabaseEloquentHasManyTest.php | 12 +++++++++--- tests/Database/DatabaseEloquentMorphTest.php | 12 +++++++++--- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/Database/DatabaseEloquentHasManyTest.php b/tests/Database/DatabaseEloquentHasManyTest.php index b184dfdd14c4..caa8b1d8196a 100755 --- a/tests/Database/DatabaseEloquentHasManyTest.php +++ b/tests/Database/DatabaseEloquentHasManyTest.php @@ -227,7 +227,9 @@ public function testUpdateOrCreateMethodFindsFirstModelAndUpdates() $relation->getQuery()->shouldReceive('where')->once()->with(['foo'])->andReturn($relation->getQuery()); $relation->getQuery()->shouldReceive('first')->once()->with()->andReturn($model = m::mock(stdClass::class)); $relation->getRelated()->shouldReceive('newInstance')->never(); - $model->shouldReceive('fill')->once()->with(['bar']); + + $model->wasRecentlyCreated = false; + $model->shouldReceive('fill')->once()->with(['bar'])->andReturn($model); $model->shouldReceive('save')->once(); $this->assertInstanceOf(stdClass::class, $relation->updateOrCreate(['foo'], ['bar'])); @@ -236,11 +238,15 @@ public function testUpdateOrCreateMethodFindsFirstModelAndUpdates() public function testUpdateOrCreateMethodCreatesNewModelWithForeignKeySet() { $relation = $this->getRelation(); + $relation->getQuery()->shouldReceive('withSavepointIfNeeded')->once()->andReturnUsing(function ($scope) { + return $scope(); + }); $relation->getQuery()->shouldReceive('where')->once()->with(['foo'])->andReturn($relation->getQuery()); $relation->getQuery()->shouldReceive('first')->once()->with()->andReturn(null); - $relation->getRelated()->shouldReceive('newInstance')->once()->with(['foo'])->andReturn($model = m::mock(Model::class)); + $relation->getRelated()->shouldReceive('newInstance')->once()->with(['foo', 'bar'])->andReturn($model = m::mock(Model::class)); + + $model->wasRecentlyCreated = true; $model->shouldReceive('save')->once()->andReturn(true); - $model->shouldReceive('fill')->once()->with(['bar']); $model->shouldReceive('setAttribute')->once()->with('foreign_key', 1); $this->assertInstanceOf(Model::class, $relation->updateOrCreate(['foo'], ['bar'])); diff --git a/tests/Database/DatabaseEloquentMorphTest.php b/tests/Database/DatabaseEloquentMorphTest.php index 4870284090f2..bb9da590e7e0 100755 --- a/tests/Database/DatabaseEloquentMorphTest.php +++ b/tests/Database/DatabaseEloquentMorphTest.php @@ -302,8 +302,10 @@ public function testUpdateOrCreateMethodFindsFirstModelAndUpdates() $relation->getQuery()->shouldReceive('where')->once()->with(['foo'])->andReturn($relation->getQuery()); $relation->getQuery()->shouldReceive('first')->once()->with()->andReturn($model = m::mock(Model::class)); $relation->getRelated()->shouldReceive('newInstance')->never(); + + $model->wasRecentlyCreated = false; $model->shouldReceive('setAttribute')->never(); - $model->shouldReceive('fill')->once()->with(['bar']); + $model->shouldReceive('fill')->once()->with(['bar'])->andReturn($model); $model->shouldReceive('save')->once(); $this->assertInstanceOf(Model::class, $relation->updateOrCreate(['foo'], ['bar'])); @@ -312,13 +314,17 @@ public function testUpdateOrCreateMethodFindsFirstModelAndUpdates() public function testUpdateOrCreateMethodCreatesNewMorphModel() { $relation = $this->getOneRelation(); + $relation->getQuery()->shouldReceive('withSavepointIfNeeded')->once()->andReturnUsing(function ($scope) { + return $scope(); + }); $relation->getQuery()->shouldReceive('where')->once()->with(['foo'])->andReturn($relation->getQuery()); $relation->getQuery()->shouldReceive('first')->once()->with()->andReturn(null); - $relation->getRelated()->shouldReceive('newInstance')->once()->with(['foo'])->andReturn($model = m::mock(Model::class)); + $relation->getRelated()->shouldReceive('newInstance')->once()->with(['foo', 'bar'])->andReturn($model = m::mock(Model::class)); + + $model->wasRecentlyCreated = true; $model->shouldReceive('setAttribute')->once()->with('morph_id', 1); $model->shouldReceive('setAttribute')->once()->with('morph_type', get_class($relation->getParent())); $model->shouldReceive('save')->once()->andReturn(true); - $model->shouldReceive('fill')->once()->with(['bar']); $this->assertInstanceOf(Model::class, $relation->updateOrCreate(['foo'], ['bar'])); } From ebb3e889337c71ea15ebd01d47ffad364c384958 Mon Sep 17 00:00:00 2001 From: mpyw Date: Tue, 29 Aug 2023 11:16:15 +0900 Subject: [PATCH 3/4] Fix: correctly apply unused `$joining` and `$touch` --- src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php b/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php index 51d723b736f4..b48d58732406 100755 --- a/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php +++ b/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php @@ -672,11 +672,11 @@ public function createOrFirst(array $attributes = [], array $values = [], array */ public function updateOrCreate(array $attributes, array $values = [], array $joining = [], $touch = true) { - return tap($this->firstOrCreate($attributes, $values), function ($instance) use ($values) { + return tap($this->firstOrCreate($attributes, $values, $joining, $touch), function ($instance) use ($values, $touch) { if (! $instance->wasRecentlyCreated) { $instance->fill($values); - $instance->save(['touch' => false]); + $instance->save(compact('touch')); } }); } From 0c6ebdb19cb47adf5d940a418477676610e825a8 Mon Sep 17 00:00:00 2001 From: mpyw Date: Tue, 29 Aug 2023 13:06:02 +0900 Subject: [PATCH 4/4] Fix: `save()` should always accepts `['touch' => false]` --- src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php b/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php index b48d58732406..b90ff80cc6fb 100755 --- a/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php +++ b/src/Illuminate/Database/Eloquent/Relations/BelongsToMany.php @@ -672,11 +672,11 @@ public function createOrFirst(array $attributes = [], array $values = [], array */ public function updateOrCreate(array $attributes, array $values = [], array $joining = [], $touch = true) { - return tap($this->firstOrCreate($attributes, $values, $joining, $touch), function ($instance) use ($values, $touch) { + return tap($this->firstOrCreate($attributes, $values, $joining, $touch), function ($instance) use ($values) { if (! $instance->wasRecentlyCreated) { $instance->fill($values); - $instance->save(compact('touch')); + $instance->save(['touch' => false]); } }); }