From 17cf51113d74c1e64ddd272d16fe40b073b1907e Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Thu, 19 Nov 2020 17:02:53 +0100 Subject: [PATCH 1/2] Avoid no-op database query in Model::destroy() with empty ids --- src/Illuminate/Database/Eloquent/Model.php | 15 +++++++++----- tests/Database/DatabaseEloquentModelTest.php | 21 ++++++++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/Illuminate/Database/Eloquent/Model.php b/src/Illuminate/Database/Eloquent/Model.php index f6f90b544391..acede7c91308 100644 --- a/src/Illuminate/Database/Eloquent/Model.php +++ b/src/Illuminate/Database/Eloquent/Model.php @@ -1061,22 +1061,27 @@ protected function insertAndSetId(Builder $query, $attributes) */ public static function destroy($ids) { - // We'll initialize a count here so we will return the total number of deletes - // for the operation. The developers can then check this number as a boolean - // type value or get this total count of records deleted for logging, etc. - $count = 0; - if ($ids instanceof BaseCollection) { $ids = $ids->all(); } $ids = is_array($ids) ? $ids : func_get_args(); + // Avoid a no-op database query by bailing early + if (count($ids) === 0) { + return 0; + } + // We will actually pull the models from the database table and call delete on // each of them individually so that their events get fired properly with a // correct set of attributes in case the developers wants to check these. $key = ($instance = new static)->getKeyName(); + // We'll initialize a count here so we will return the total number of deletes + // for the operation. The developers can then check this number as a boolean + // type value or get this total count of records deleted for logging, etc. + $count = 0; + foreach ($instance->whereIn($key, $ids)->get() as $model) { if ($model->delete()) { $count++; diff --git a/tests/Database/DatabaseEloquentModelTest.php b/tests/Database/DatabaseEloquentModelTest.php index 2d3f3e10ebdf..b9f9758cd031 100755 --- a/tests/Database/DatabaseEloquentModelTest.php +++ b/tests/Database/DatabaseEloquentModelTest.php @@ -293,6 +293,16 @@ public function testDestroyMethodCallsQueryBuilderCorrectlyWithCollection() EloquentModelDestroyStub::destroy(new Collection([1, 2, 3])); } + public function testDestroyMethodCallsQueryBuilderCorrectlyWithMultipleArgs() + { + EloquentModelDestroyStub::destroy(1, 2, 3); + } + + public function testDestroyMethodCallsQueryBuilderCorrectlyWithEmptyIds() + { + EloquentModelEmptyDestroyStub::destroy([]); + } + public function testWithMethodCallsQueryBuilderCorrectly() { $result = EloquentModelWithStub::with('foo', 'bar'); @@ -2400,6 +2410,17 @@ public function newQuery() } } +class EloquentModelEmptyDestroyStub extends Model +{ + public function newQuery() + { + $mock = m::mock(Builder::class); + $mock->shouldReceive('whereIn')->never(); + + return $mock; + } +} + class EloquentModelHydrateRawStub extends Model { public static function hydrate(array $items, $connection = null) From 777e40a35193384fbc0f0be2c3900151a61df999 Mon Sep 17 00:00:00 2001 From: Benedikt Franke Date: Thu, 19 Nov 2020 17:11:50 +0100 Subject: [PATCH 2/2] Expect the returned count to be 0 --- tests/Database/DatabaseEloquentModelTest.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/Database/DatabaseEloquentModelTest.php b/tests/Database/DatabaseEloquentModelTest.php index b9f9758cd031..114c4ae6f5c2 100755 --- a/tests/Database/DatabaseEloquentModelTest.php +++ b/tests/Database/DatabaseEloquentModelTest.php @@ -300,7 +300,8 @@ public function testDestroyMethodCallsQueryBuilderCorrectlyWithMultipleArgs() public function testDestroyMethodCallsQueryBuilderCorrectlyWithEmptyIds() { - EloquentModelEmptyDestroyStub::destroy([]); + $count = EloquentModelEmptyDestroyStub::destroy([]); + $this->assertSame(0, $count); } public function testWithMethodCallsQueryBuilderCorrectly()