Skip to content
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

feat: add Validation Strict Rules #5445

Merged
merged 22 commits into from
Jan 13, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Dec 8, 2021

Description
Supersedes #5402
Fixes #5374

Add the following classes:

  • CodeIgniter\Validation\StrictRules\CreditCardRules
  • CodeIgniter\Validation\StrictRules\FileRules
  • CodeIgniter\Validation\StrictRules\FormatRules
  • CodeIgniter\Validation\StrictRules\Rules

How to check the code:

cd system/Validation/
diff -u CreditCardRules.php StrictRules/CreditCardRules.php
diff -u FileRules.php StrictRules/FileRules.php
diff -u FormatRules.php StrictRules/FormatRules.php
diff -u Rules.php StrictRules/Rules.php

cd -
cd tests/system/Validation/
diff -u CreditCardRulesTest.php StrictRules/CreditCardRulesTest.php
diff -u DatabaseRelatedRulesTest.php StrictRules/DatabaseRelatedRulesTest.php
diff -u FileRulesTest.php StrictRules/FileRulesTest.php
diff -u FormatRulesTest.php StrictRules/FormatRulesTest.php
diff -u RulesTest.php StrictRules/RulesTest.php
diff -u ValidationTest.php StrictRules/ValidationTest.php

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added enhancement PRs that improve existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them labels Dec 8, 2021
@kenjis
Copy link
Member Author

kenjis commented Dec 8, 2021

The behavior changes are only the following:

$ diff -u FormatRulesTest.php StrictRules/FormatRulesTest.php
--- FormatRulesTest.php	2021-12-08 11:01:42.000000000 +0900
+++ StrictRules/FormatRulesTest.php	2021-12-08 11:33:22.000000000 +0900
@@ -9,9 +9,10 @@
  * the LICENSE file that was distributed with this source code.
  */
 
-namespace CodeIgniter\Validation;
+namespace CodeIgniter\Validation\StrictRules;
 
 use CodeIgniter\Test\CIUnitTestCase;
+use CodeIgniter\Validation\Validation;
 use Config\Services;
 use Generator;
 use Tests\Support\Validation\TestRules;
@@ -837,21 +838,19 @@
 
     public function integerInvalidTypeDataProvider(): Generator
     {
-        // TypeError : CodeIgniter\Validation\FormatRules::integer(): Argument #1 ($str) must be of type ?string, array given
-        // yield 'array with int' => [
-        // [555],
-        // false,
-        // ];
-
-        // TypeError : CodeIgniter\Validation\FormatRules::integer(): Argument #1 ($str) must be of type ?string, array given
-        // yield 'empty array' => [
-        // [],
-        // false,
-        // ];
+        yield 'array with int' => [
+            [555],
+            false,  // true in v4.1.5 and earlier
+        ];
+
+        yield 'empty array' => [
+            [],
+            false,  // true in v4.1.5 and earlier
+        ];
 
         yield 'bool true' => [
             true,
-            true,  // incorrect
+            false,  // true in v4.1.5 and earlier
         ];
 
         yield 'bool false' => [

@kenjis kenjis mentioned this pull request Dec 8, 2021
5 tasks
Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am quite uncomfortable with the sheer amount of duplicated code. Too much of a maintenance burden updating both strict and non-strict versions.

Since the changes here are mostly adding declare(strict_types=1) and adding some type safety checks, I suggest using composition. Something like this (for example, in Credit Card Rules):

<?php

namespace CodeIgniter\Validation\StrictRules;

use CodeIgniter\Validation\CreditCardRules as NonstrictCreditCardRules;

class CreditCardRules
{
    private $nonStrictCreditCardRules;

    public function __construct()
    {
        $this->nonstrictCreditCardRules = new NonstrictCreditCardRules();
    }

    public function valid_cc_number($ccNumber, string $type): bool
    {
        if (! is_string($ccNumber)) {
            return false;
        }

        return $this->nonstrictCreditCardRules->valid_cc_number($ccNumber, $type);
    }
}

If a non-strict version's method cannot be used entirely unmodified, then we proceed overriding it instead.

@kenjis
Copy link
Member Author

kenjis commented Jan 6, 2022

@paulbalandan Refactored with composition and inheritance.

Copy link
Member

@paulbalandan paulbalandan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking much better! I left a few comments on the type hinting and docblocks.

system/Validation/StrictRules/FormatRules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/FormatRules.php Show resolved Hide resolved
system/Validation/StrictRules/FormatRules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/FormatRules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/FormatRules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/FormatRules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/FormatRules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/FormatRules.php Show resolved Hide resolved
system/Validation/StrictRules/FormatRules.php Outdated Show resolved Hide resolved
system/Validation/StrictRules/Rules.php Show resolved Hide resolved
@kenjis
Copy link
Member Author

kenjis commented Jan 7, 2022

I changed alpha_space 351544b
Because I don't think it accepts null.

Why does alpha_space in NonStrict Rule accept null?
Other alpha_* does not accept. Is it a bug?

The behavior changes of this PR:

$ diff -u FormatRulesTest.php StrictRules/FormatRulesTest.php
--- FormatRulesTest.php	2022-01-07 09:35:57.000000000 +0900
+++ StrictRules/FormatRulesTest.php	2022-01-07 13:21:17.000000000 +0900
@@ -9,9 +9,10 @@
  * the LICENSE file that was distributed with this source code.
  */
 
-namespace CodeIgniter\Validation;
+namespace CodeIgniter\Validation\StrictRules;
 
 use CodeIgniter\Test\CIUnitTestCase;
+use CodeIgniter\Validation\Validation;
 use Config\Services;
 use Generator;
 use Tests\Support\Validation\TestRules;
@@ -472,7 +473,7 @@
         yield from [
             [
                 null,
-                true,
+                false,  // true in Traditional Rule
             ],
             [
                 self::ALPHABET,
@@ -837,21 +838,19 @@
 
     public function integerInvalidTypeDataProvider(): Generator
     {
-        // TypeError : CodeIgniter\Validation\FormatRules::integer(): Argument #1 ($str) must be of type ?string, array given
-        // yield 'array with int' => [
-        // [555],
-        // false,
-        // ];
-
-        // TypeError : CodeIgniter\Validation\FormatRules::integer(): Argument #1 ($str) must be of type ?string, array given
-        // yield 'empty array' => [
-        // [],
-        // false,
-        // ];
+        yield 'array with int' => [
+            [555],
+            false,  // TypeError in Traditional Rule
+        ];
+
+        yield 'empty array' => [
+            [],
+            false,  // TypeError in Traditional Rule
+        ];
 
         yield 'bool true' => [
             true,
-            true,  // incorrect
+            false,  // true in Traditional Rule
         ];
 
         yield 'bool false' => [

user_guide_src/source/libraries/validation.rst Outdated Show resolved Hide resolved
user_guide_src/source/libraries/validation.rst Outdated Show resolved Hide resolved
user_guide_src/source/libraries/validation.rst Outdated Show resolved Hide resolved
user_guide_src/source/libraries/validation.rst Outdated Show resolved Hide resolved
user_guide_src/source/libraries/validation.rst Outdated Show resolved Hide resolved
kenjis and others added 6 commits January 13, 2022 10:03
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
Co-authored-by: John Paul E. Balandan, CPA <51850998+paulbalandan@users.noreply.github.com>
@kenjis kenjis merged commit 1ca7d77 into codeigniter4:develop Jan 13, 2022
@kenjis kenjis deleted the add-validation-StrictRules branch January 13, 2022 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Verified issues on the current code behavior or pull requests that will fix them enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Validation rules integer and numeric accepts boolean
2 participants