From 7d195e3a580141b7d1488af2007c4df2b12357e8 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 11 Jan 2023 10:30:52 +0900 Subject: [PATCH 1/8] test: refactor: remove duplicate code --- .../Validation/DatabaseRelatedRulesTest.php | 182 +----------------- .../StrictRules/DatabaseRelatedRulesTest.php | 8 +- 2 files changed, 8 insertions(+), 182 deletions(-) diff --git a/tests/system/Validation/DatabaseRelatedRulesTest.php b/tests/system/Validation/DatabaseRelatedRulesTest.php index 42881bab444e..ec98605fba05 100644 --- a/tests/system/Validation/DatabaseRelatedRulesTest.php +++ b/tests/system/Validation/DatabaseRelatedRulesTest.php @@ -11,10 +11,7 @@ namespace CodeIgniter\Validation; -use CodeIgniter\Test\CIUnitTestCase; -use CodeIgniter\Test\DatabaseTestTrait; -use Config\Database; -use Config\Services; +use CodeIgniter\Validation\StrictRules\DatabaseRelatedRulesTest as StrictDatabaseRelatedRulesTest; use Tests\Support\Validation\TestRules; /** @@ -22,12 +19,9 @@ * * @group DatabaseLive */ -final class DatabaseRelatedRulesTest extends CIUnitTestCase +final class DatabaseRelatedRulesTest extends StrictDatabaseRelatedRulesTest { - use DatabaseTestTrait; - - private Validation $validation; - private array $config = [ + protected array $config = [ 'ruleSets' => [ Rules::class, FormatRules::class, @@ -44,174 +38,4 @@ final class DatabaseRelatedRulesTest extends CIUnitTestCase ], ], ]; - - protected function setUp(): void - { - parent::setUp(); - $this->validation = new Validation((object) $this->config, Services::renderer()); - $this->validation->reset(); - } - - public function testIsUniqueFalse(): void - { - Database::connect() - ->table('user') - ->insert([ - 'name' => 'Derek Travis', - 'email' => 'derek@world.com', - 'country' => 'Elbonia', - ]); - - $data = ['email' => 'derek@world.com']; - $this->validation->setRules(['email' => 'is_unique[user.email]']); - $this->assertFalse($this->validation->run($data)); - } - - public function testIsUniqueTrue(): void - { - $data = ['email' => 'derek@world.co.uk']; - $this->validation->setRules(['email' => 'is_unique[user.email]']); - $this->assertTrue($this->validation->run($data)); - } - - public function testIsUniqueIgnoresParams(): void - { - $db = Database::connect(); - $db - ->table('user') - ->insert([ - 'name' => 'Developer A', - 'email' => 'deva@example.com', - 'country' => 'Elbonia', - ]); - $row = $db->table('user') - ->limit(1) - ->get() - ->getRow(); - - $data = ['email' => 'derek@world.co.uk']; - $this->validation->setRules(['email' => "is_unique[user.email,id,{$row->id}]"]); - $this->assertTrue($this->validation->run($data)); - } - - public function testIsUniqueIgnoresParamsPlaceholders(): void - { - $this->hasInDatabase('user', [ - 'name' => 'Derek', - 'email' => 'derek@world.co.uk', - 'country' => 'GB', - ]); - - $row = Database::connect() - ->table('user') - ->limit(1) - ->get() - ->getRow(); - - $data = [ - 'id' => $row->id, - 'email' => 'derek@world.co.uk', - ]; - - $this->validation->setRules(['email' => 'is_unique[user.email,id,{id}]']); - $this->assertTrue($this->validation->run($data)); - } - - public function testIsUniqueByManualRun(): void - { - Database::connect() - ->table('user') - ->insert([ - 'name' => 'Developer A', - 'email' => 'deva@example.com', - 'country' => 'Elbonia', - ]); - - $this->assertFalse((new Rules())->is_unique('deva@example.com', 'user.email,id,{id}', [])); - } - - public function testIsNotUniqueFalse(): void - { - Database::connect() - ->table('user') - ->insert([ - 'name' => 'Derek Travis', - 'email' => 'derek@world.com', - 'country' => 'Elbonia', - ]); - - $data = ['email' => 'derek1@world.com']; - $this->validation->setRules(['email' => 'is_not_unique[user.email]']); - $this->assertFalse($this->validation->run($data)); - } - - public function testIsNotUniqueTrue(): void - { - Database::connect() - ->table('user') - ->insert([ - 'name' => 'Derek Travis', - 'email' => 'derek@world.com', - 'country' => 'Elbonia', - ]); - - $data = ['email' => 'derek@world.com']; - $this->validation->setRules(['email' => 'is_not_unique[user.email]']); - $this->assertTrue($this->validation->run($data)); - } - - public function testIsNotUniqueIgnoresParams(): void - { - $db = Database::connect(); - $db->table('user') - ->insert([ - 'name' => 'Developer A', - 'email' => 'deva@example.com', - 'country' => 'Elbonia', - ]); - - $row = $db->table('user') - ->limit(1) - ->get() - ->getRow(); - - $data = ['email' => 'derek@world.co.uk']; - $this->validation->setRules(['email' => "is_not_unique[user.email,id,{$row->id}]"]); - $this->assertFalse($this->validation->run($data)); - } - - public function testIsNotUniqueIgnoresParamsPlaceholders(): void - { - $this->hasInDatabase('user', [ - 'name' => 'Derek', - 'email' => 'derek@world.co.uk', - 'country' => 'GB', - ]); - - $row = Database::connect() - ->table('user') - ->limit(1) - ->get() - ->getRow(); - - $data = [ - 'id' => $row->id, - 'email' => 'derek@world.co.uk', - ]; - $this->validation->setRules(['email' => 'is_not_unique[user.email,id,{id}]']); - $this->assertTrue($this->validation->run($data)); - } - - public function testIsNotUniqueByManualRun(): void - { - Database::connect() - ->table('user') - ->insert([ - 'name' => 'Developer A', - 'email' => 'deva@example.com', - 'country' => 'Elbonia', - ]); - - $this->assertTrue((new Rules())->is_not_unique('deva@example.com', 'user.email,id,{id}', [])); - } } diff --git a/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php b/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php index 79c9c7d39e9f..b17319fe5951 100644 --- a/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php +++ b/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php @@ -21,14 +21,16 @@ /** * @internal * + * @no-final + * * @group DatabaseLive */ -final class DatabaseRelatedRulesTest extends CIUnitTestCase +class DatabaseRelatedRulesTest extends CIUnitTestCase { use DatabaseTestTrait; - private Validation $validation; - private array $config = [ + protected Validation $validation; + protected array $config = [ 'ruleSets' => [ Rules::class, FormatRules::class, From 7f789ace3f2d7793ccf0e0ff8b1cbe0ae5d648b6 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 11 Jan 2023 10:35:40 +0900 Subject: [PATCH 2/8] test: fix wrong Rules instance --- tests/system/Validation/DatabaseRelatedRulesTest.php | 5 +++++ .../Validation/StrictRules/DatabaseRelatedRulesTest.php | 9 +++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/system/Validation/DatabaseRelatedRulesTest.php b/tests/system/Validation/DatabaseRelatedRulesTest.php index ec98605fba05..e9abd11f83aa 100644 --- a/tests/system/Validation/DatabaseRelatedRulesTest.php +++ b/tests/system/Validation/DatabaseRelatedRulesTest.php @@ -38,4 +38,9 @@ final class DatabaseRelatedRulesTest extends StrictDatabaseRelatedRulesTest ], ], ]; + + protected function createRules() + { + return new Rules(); + } } diff --git a/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php b/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php index b17319fe5951..a1003899f8e9 100644 --- a/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php +++ b/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php @@ -55,6 +55,11 @@ protected function setUp(): void $this->validation->reset(); } + protected function createRules() + { + return new Rules(); + } + public function testIsUniqueFalse(): void { Database::connect() @@ -130,7 +135,7 @@ public function testIsUniqueByManualRun(): void 'country' => 'Elbonia', ]); - $this->assertFalse((new Rules())->is_unique('deva@example.com', 'user.email,id,{id}', [])); + $this->assertFalse($this->createRules()->is_unique('deva@example.com', 'user.email,id,{id}', [])); } public function testIsNotUniqueFalse(): void @@ -215,6 +220,6 @@ public function testIsNotUniqueByManualRun(): void 'country' => 'Elbonia', ]); - $this->assertTrue((new Rules())->is_not_unique('deva@example.com', 'user.email,id,{id}', [])); + $this->assertTrue($this->createRules()->is_not_unique('deva@example.com', 'user.email,id,{id}', [])); } } From 5ab8487749727a8eb0feca7e8047c994d5d81ef3 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 11 Jan 2023 10:38:42 +0900 Subject: [PATCH 3/8] test: add test for is_unique and int value --- .../StrictRules/DatabaseRelatedRulesTest.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php b/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php index a1003899f8e9..9b56dc3e3205 100644 --- a/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php +++ b/tests/system/Validation/StrictRules/DatabaseRelatedRulesTest.php @@ -138,6 +138,21 @@ public function testIsUniqueByManualRun(): void $this->assertFalse($this->createRules()->is_unique('deva@example.com', 'user.email,id,{id}', [])); } + public function testIsUniqueIntValueByManualRun(): void + { + Database::connect() + ->table('user') + ->insert([ + 'name' => 'Developer A', + 'email' => 'deva@example.com', + 'country' => 'Elbonia', + ]); + + $result = $this->createRules()->is_unique(1, 'user.id', []); + + $this->assertFalse($result); + } + public function testIsNotUniqueFalse(): void { Database::connect() From d7099ccdf874a5274af47df67c545a6692cbd0e1 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 11 Jan 2023 10:53:01 +0900 Subject: [PATCH 4/8] fix: TypeError in Rules::is_unique() TypeError : CodeIgniter\Validation\Rules::is_unique(): Argument #1 ($str) must be of type ?string, int given, called in /.../CodeIgniter4/system/Validation/StrictRules/Rules.php on line 154 /.../CodeIgniter4/system/Validation/Rules.php:126 --- system/Validation/Rules.php | 26 ++++++++++++------------- system/Validation/StrictRules/Rules.php | 20 ++++++++++++++++++- 2 files changed, 31 insertions(+), 15 deletions(-) diff --git a/system/Validation/Rules.php b/system/Validation/Rules.php index 40a5955b157d..c8284629d3b3 100644 --- a/system/Validation/Rules.php +++ b/system/Validation/Rules.php @@ -11,6 +11,7 @@ namespace CodeIgniter\Validation; +use CodeIgniter\Validation\StrictRules\Rules as StrictRules; use Config\Database; use InvalidArgumentException; @@ -19,6 +20,15 @@ */ class Rules { + private ?StrictRules $strictRules = null; + + private function createStrictRules(): void + { + if ($this->strictRules === null) { + $this->strictRules = new StrictRules(); + } + } + /** * The value does not match another field in $data. * @@ -125,21 +135,9 @@ public function in_list(?string $value, string $list): bool */ public function is_unique(?string $str, string $field, array $data): bool { - [$field, $ignoreField, $ignoreValue] = array_pad(explode(',', $field), 3, null); - - sscanf($field, '%[^.].%[^.]', $table, $field); - - $row = Database::connect($data['DBGroup'] ?? null) - ->table($table) - ->select('1') - ->where($field, $str) - ->limit(1); - - if (! empty($ignoreField) && ! empty($ignoreValue) && ! preg_match('/^\{(\w+)\}$/', $ignoreValue)) { - $row = $row->where("{$ignoreField} !=", $ignoreValue); - } + $this->createStrictRules(); - return $row->get()->getRow() === null; + return $this->strictRules->is_unique($str, $field, $data); } /** diff --git a/system/Validation/StrictRules/Rules.php b/system/Validation/StrictRules/Rules.php index 47dbee9b5ae7..1091b319f43f 100644 --- a/system/Validation/StrictRules/Rules.php +++ b/system/Validation/StrictRules/Rules.php @@ -151,7 +151,25 @@ public function in_list($value, string $list): bool */ public function is_unique($str, string $field, array $data): bool { - return $this->nonStrictRules->is_unique($str, $field, $data); + [$field, $ignoreField, $ignoreValue] = array_pad( + explode(',', $field), + 3, + null + ); + + sscanf($field, '%[^.].%[^.]', $table, $field); + + $row = Database::connect($data['DBGroup'] ?? null) + ->table($table) + ->select('1') + ->where($field, $str) + ->limit(1); + + if (! empty($ignoreField) && ! empty($ignoreValue) && ! preg_match('/^\{(\w+)\}$/', $ignoreValue)) { + $row = $row->where("{$ignoreField} !=", $ignoreValue); + } + + return $row->get()->getRow() === null; } /** From 5722ab0f3e0c0ddeea4eb2d3dfa2274f8877313d Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 11 Jan 2023 10:56:13 +0900 Subject: [PATCH 5/8] fix: TypeError in Rules::is_not_unique() --- system/Validation/Rules.php | 18 ++---------------- system/Validation/StrictRules/Rules.php | 22 +++++++++++++++++++++- 2 files changed, 23 insertions(+), 17 deletions(-) diff --git a/system/Validation/Rules.php b/system/Validation/Rules.php index c8284629d3b3..1e543a8fd858 100644 --- a/system/Validation/Rules.php +++ b/system/Validation/Rules.php @@ -95,23 +95,9 @@ public function greater_than_equal_to(?string $str, string $min): bool */ public function is_not_unique(?string $str, string $field, array $data): bool { - // Grab any data for exclusion of a single row. - [$field, $whereField, $whereValue] = array_pad(explode(',', $field), 3, null); - - // Break the table and field apart - sscanf($field, '%[^.].%[^.]', $table, $field); - - $row = Database::connect($data['DBGroup'] ?? null) - ->table($table) - ->select('1') - ->where($field, $str) - ->limit(1); - - if (! empty($whereField) && ! empty($whereValue) && ! preg_match('/^\{(\w+)\}$/', $whereValue)) { - $row = $row->where($whereField, $whereValue); - } + $this->createStrictRules(); - return $row->get()->getRow() !== null; + return $this->strictRules->is_not_unique($str, $field, $data); } /** diff --git a/system/Validation/StrictRules/Rules.php b/system/Validation/StrictRules/Rules.php index 1091b319f43f..ad21b6126696 100644 --- a/system/Validation/StrictRules/Rules.php +++ b/system/Validation/StrictRules/Rules.php @@ -117,7 +117,27 @@ public function greater_than_equal_to($str, string $min): bool */ public function is_not_unique($str, string $field, array $data): bool { - return $this->nonStrictRules->is_not_unique($str, $field, $data); + // Grab any data for exclusion of a single row. + [$field, $whereField, $whereValue] = array_pad( + explode(',', $field), + 3, + null + ); + + // Break the table and field apart + sscanf($field, '%[^.].%[^.]', $table, $field); + + $row = Database::connect($data['DBGroup'] ?? null) + ->table($table) + ->select('1') + ->where($field, $str) + ->limit(1); + + if (! empty($whereField) && ! empty($whereValue) && ! preg_match('/^\{(\w+)\}$/', $whereValue)) { + $row = $row->where($whereField, $whereValue); + } + + return $row->get()->getRow() !== null; } /** From 7b405d1309a89ac5d3e26f84e982bcb39f373f28 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 11 Jan 2023 11:03:37 +0900 Subject: [PATCH 6/8] fix: add type check --- system/Validation/StrictRules/Rules.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/system/Validation/StrictRules/Rules.php b/system/Validation/StrictRules/Rules.php index ad21b6126696..8aa8f85fc228 100644 --- a/system/Validation/StrictRules/Rules.php +++ b/system/Validation/StrictRules/Rules.php @@ -117,6 +117,10 @@ public function greater_than_equal_to($str, string $min): bool */ public function is_not_unique($str, string $field, array $data): bool { + if (is_object($str) || is_array($str)) { + return false; + } + // Grab any data for exclusion of a single row. [$field, $whereField, $whereValue] = array_pad( explode(',', $field), @@ -171,6 +175,10 @@ public function in_list($value, string $list): bool */ public function is_unique($str, string $field, array $data): bool { + if (is_object($str) || is_array($str)) { + return false; + } + [$field, $ignoreField, $ignoreValue] = array_pad( explode(',', $field), 3, From 7d4e0ebecef745be2be0599318b781f92e561f02 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 11 Jan 2023 11:07:13 +0900 Subject: [PATCH 7/8] fix: revert Traditional is_unique() is_not_unique() implementation If we use Strict Rules, the behavior may change. --- system/Validation/Rules.php | 44 +++++++++++++++++++++++++------------ 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/system/Validation/Rules.php b/system/Validation/Rules.php index 1e543a8fd858..40a5955b157d 100644 --- a/system/Validation/Rules.php +++ b/system/Validation/Rules.php @@ -11,7 +11,6 @@ namespace CodeIgniter\Validation; -use CodeIgniter\Validation\StrictRules\Rules as StrictRules; use Config\Database; use InvalidArgumentException; @@ -20,15 +19,6 @@ */ class Rules { - private ?StrictRules $strictRules = null; - - private function createStrictRules(): void - { - if ($this->strictRules === null) { - $this->strictRules = new StrictRules(); - } - } - /** * The value does not match another field in $data. * @@ -95,9 +85,23 @@ public function greater_than_equal_to(?string $str, string $min): bool */ public function is_not_unique(?string $str, string $field, array $data): bool { - $this->createStrictRules(); + // Grab any data for exclusion of a single row. + [$field, $whereField, $whereValue] = array_pad(explode(',', $field), 3, null); + + // Break the table and field apart + sscanf($field, '%[^.].%[^.]', $table, $field); + + $row = Database::connect($data['DBGroup'] ?? null) + ->table($table) + ->select('1') + ->where($field, $str) + ->limit(1); - return $this->strictRules->is_not_unique($str, $field, $data); + if (! empty($whereField) && ! empty($whereValue) && ! preg_match('/^\{(\w+)\}$/', $whereValue)) { + $row = $row->where($whereField, $whereValue); + } + + return $row->get()->getRow() !== null; } /** @@ -121,9 +125,21 @@ public function in_list(?string $value, string $list): bool */ public function is_unique(?string $str, string $field, array $data): bool { - $this->createStrictRules(); + [$field, $ignoreField, $ignoreValue] = array_pad(explode(',', $field), 3, null); + + sscanf($field, '%[^.].%[^.]', $table, $field); + + $row = Database::connect($data['DBGroup'] ?? null) + ->table($table) + ->select('1') + ->where($field, $str) + ->limit(1); + + if (! empty($ignoreField) && ! empty($ignoreValue) && ! preg_match('/^\{(\w+)\}$/', $ignoreValue)) { + $row = $row->where("{$ignoreField} !=", $ignoreValue); + } - return $this->strictRules->is_unique($str, $field, $data); + return $row->get()->getRow() === null; } /** From 76c8997c4984f521c755ff6e9ec230f7c5fed822 Mon Sep 17 00:00:00 2001 From: kenjis Date: Wed, 11 Jan 2023 11:15:44 +0900 Subject: [PATCH 8/8] style: break long lines --- system/Validation/Rules.php | 22 ++++++++++++++++++---- system/Validation/StrictRules/Rules.php | 10 ++++++++-- 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/system/Validation/Rules.php b/system/Validation/Rules.php index 40a5955b157d..93b86ea6b1dd 100644 --- a/system/Validation/Rules.php +++ b/system/Validation/Rules.php @@ -86,7 +86,11 @@ public function greater_than_equal_to(?string $str, string $min): bool public function is_not_unique(?string $str, string $field, array $data): bool { // Grab any data for exclusion of a single row. - [$field, $whereField, $whereValue] = array_pad(explode(',', $field), 3, null); + [$field, $whereField, $whereValue] = array_pad( + explode(',', $field), + 3, + null + ); // Break the table and field apart sscanf($field, '%[^.].%[^.]', $table, $field); @@ -97,7 +101,10 @@ public function is_not_unique(?string $str, string $field, array $data): bool ->where($field, $str) ->limit(1); - if (! empty($whereField) && ! empty($whereValue) && ! preg_match('/^\{(\w+)\}$/', $whereValue)) { + if ( + ! empty($whereField) && ! empty($whereValue) + && ! preg_match('/^\{(\w+)\}$/', $whereValue) + ) { $row = $row->where($whereField, $whereValue); } @@ -125,7 +132,11 @@ public function in_list(?string $value, string $list): bool */ public function is_unique(?string $str, string $field, array $data): bool { - [$field, $ignoreField, $ignoreValue] = array_pad(explode(',', $field), 3, null); + [$field, $ignoreField, $ignoreValue] = array_pad( + explode(',', $field), + 3, + null + ); sscanf($field, '%[^.].%[^.]', $table, $field); @@ -135,7 +146,10 @@ public function is_unique(?string $str, string $field, array $data): bool ->where($field, $str) ->limit(1); - if (! empty($ignoreField) && ! empty($ignoreValue) && ! preg_match('/^\{(\w+)\}$/', $ignoreValue)) { + if ( + ! empty($ignoreField) && ! empty($ignoreValue) + && ! preg_match('/^\{(\w+)\}$/', $ignoreValue) + ) { $row = $row->where("{$ignoreField} !=", $ignoreValue); } diff --git a/system/Validation/StrictRules/Rules.php b/system/Validation/StrictRules/Rules.php index 8aa8f85fc228..05d9e96a305b 100644 --- a/system/Validation/StrictRules/Rules.php +++ b/system/Validation/StrictRules/Rules.php @@ -137,7 +137,10 @@ public function is_not_unique($str, string $field, array $data): bool ->where($field, $str) ->limit(1); - if (! empty($whereField) && ! empty($whereValue) && ! preg_match('/^\{(\w+)\}$/', $whereValue)) { + if ( + ! empty($whereField) && ! empty($whereValue) + && ! preg_match('/^\{(\w+)\}$/', $whereValue) + ) { $row = $row->where($whereField, $whereValue); } @@ -193,7 +196,10 @@ public function is_unique($str, string $field, array $data): bool ->where($field, $str) ->limit(1); - if (! empty($ignoreField) && ! empty($ignoreValue) && ! preg_match('/^\{(\w+)\}$/', $ignoreValue)) { + if ( + ! empty($ignoreField) && ! empty($ignoreValue) + && ! preg_match('/^\{(\w+)\}$/', $ignoreValue) + ) { $row = $row->where("{$ignoreField} !=", $ignoreValue); }