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

Fix: Validation. Error key for field with asterisk #5609

Merged
merged 6 commits into from
Feb 15, 2022

Conversation

iRedds
Copy link
Collaborator

@iRedds iRedds commented Jan 23, 2022

Description
Now, when validating a multidimensional array using a wildcard, only the last error is saved. Therefore, it is impossible to determine which field the error belongs to.

// data
[
    'users' => [
        'friends' => [
            ['name' => 'Jim'],
            ['name' => 'James'],
            ['name' => ''],
        ]
    ]
]

// rules
['users.*.name' => 'required|min_length[4]']

// errors
['users.*.name' => 'The users.*.name field is required.']

This PR changes behavior. Errors have a corresponding key.

[
    'users.friends.0.name' => 'The users.*.name field must be at least 4 characters in length.',
    'users.friends.2.name' => 'The users.*.name field is required.',
]

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 the bug Verified issues on the current code behavior or pull requests that will fix them label Jan 24, 2022
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.

This looks good to me.

@kenjis
Copy link
Member

kenjis commented Jan 25, 2022

The error keys (and values) will be changed. It should be documented in changelog.

@iRedds
Copy link
Collaborator Author

iRedds commented Jan 25, 2022

@kenjis added

@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Jan 25, 2022
@kenjis
Copy link
Member

kenjis commented Jan 25, 2022

Isn't this PR a breaking change?
At least, the output of getError() and hasError() will change, because the key will change.
If a user has code like if ($this->validation->hasError('id_user.*')) { ... }, it breaks.

The following test will fail.

--- a/tests/system/Validation/ValidationTest.php
+++ b/tests/system/Validation/ValidationTest.php
@@ -851,6 +851,10 @@ final class ValidationTest extends CIUnitTestCase
             'name_user.2'             => 'The name_user.* field may only contain alphabetical characters.',
             'contacts.friends.0.name' => 'The contacts.*.name field is required.',
         ], $this->validation->getErrors());
+        $this->assertSame(
+            'The id_user.* field must contain only numbers.',
+            $this->validation->getError('id_user.*')
+        );
     }
 
     public function testRulesForSingleRuleWithSingleValue(): void

@kenjis
Copy link
Member

kenjis commented Jan 25, 2022

How about these?

  • hasError('users.*.name') returns true the same as before.
  • getError('users.*.name') returns something like "The users.*.name field must be at least 4 characters in length.\nThe users.*.name field is required.".

@iRedds
Copy link
Collaborator Author

iRedds commented Jan 25, 2022

I'm not sure about the behavior of getError('users.*.name')
@paulbalandan Can you express your opinion?

@paulbalandan
Copy link
Member

Technically, this is a potentially breaking change. However, I believe the chances this change will break apps is low. So, this must be disclosed in the changelog with a caveat for those using getError('users.*.name'') to check their code.

@kenjis
Copy link
Member

kenjis commented Jan 26, 2022

If I want to check the result, do I write code like this?

if (
    $this->hasError(''users.friends.0.name')
    || $this->hasError(''users.friends.1.name')
    || $this->hasError(''users.friends.2.name')
) {
    ...
}

@iRedds
Copy link
Collaborator Author

iRedds commented Jan 26, 2022

@kenjis Wait, I was only talking about getError().

@kenjis kenjis added the stale Pull requests with conflicts label Feb 15, 2022
@kenjis
Copy link
Member

kenjis commented Feb 15, 2022

@iRedds Can you rebase and resolve conflicts?

Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
Signed-off-by: Andrey Pyzhikov <5071@mail.ru>
@iRedds iRedds force-pushed the fix/validation-asterisk-error branch from d515242 to 1336539 Compare February 15, 2022 07:51
@iRedds
Copy link
Collaborator Author

iRedds commented Feb 15, 2022

@kenjis Yes i can

@kenjis kenjis removed the stale Pull requests with conflicts label Feb 15, 2022
@kenjis kenjis merged commit 827c360 into codeigniter4:develop Feb 15, 2022
@kenjis
Copy link
Member

kenjis commented Feb 15, 2022

@iRedds Thanks!

@iRedds iRedds deleted the fix/validation-asterisk-error branch February 15, 2022 08:50
@kenjis
Copy link
Member

kenjis commented Jul 8, 2022

@iRedds @paulbalandan

Now the error array is like this:

[
    'users.friends.0.name' => 'The users.*.name field must be at least 4 characters in length.',
    'users.friends.2.name' => 'The users.*.name field is required.',
]

But would it be easier to use?

[
    'users.*.name' => [
        0 => 'The users.*.name field must be at least 4 characters in length.',
        2 => 'The users.*.name field is required.',
    ]
]

What do you think?

@iRedds
Copy link
Collaborator Author

iRedds commented Jul 9, 2022

@kenjis Here we lose the friends key.
What will happen for such a data structure?

'users' => [
    'friends' => [
        0 => [
            'name' => 'correct',
        ],
    ],
    'enemies' => [
        0 => [
            'name' => null,
        ],
    ],
]

Although my example looks like an attempt to pull an owl on a globe.
But it seems to me that such an option should be envisaged.

@kenjis
Copy link
Member

kenjis commented Jul 9, 2022

I don't know we need to support such a use case, but then:

[
    'users.*.name' => [
        'friends' => [ 
            0 => 'The users.*.name field must be at least 4 characters in length.',
            2 => 'The users.*.name field is required.',
        ],
    ]
]

@iRedds
Copy link
Collaborator Author

iRedds commented Jul 9, 2022

Since I don't use CI. It's hard for me to imagine how it would be convenient for a real project to provide errors.
Wouldn't that make it difficult to get errors?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities bug Verified issues on the current code behavior or pull requests that will fix them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants