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 custom error with asterisk field #6352

Closed

Conversation

ping-yee
Copy link
Contributor

@ping-yee ping-yee commented Aug 6, 2022

Description
Fixes #6245
Supersedes #6340

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

@iRedds
Copy link
Collaborator

iRedds commented Aug 6, 2022

I would like to ask you to change the declaration of methods according to PSR-12 (there is an example here: https://www.php-fig.org/psr/psr-12/#45-method-and-function-arguments). Because the string is longer than 120 characters.

@iRedds
Copy link
Collaborator

iRedds commented Aug 6, 2022

@kenjis @MGatner What do you think if the three arguments ($field, $label, $originalField) are replaced with an array or a DTO?

Improve CSRF protection (for Shield CSRF security fix)
docs: the session_id acquisition method that is not described codeigniter4#5586
@kenjis kenjis added bug Verified issues on the current code behavior or pull requests that will fix them breaking change Pull requests that may break existing functionalities labels Aug 6, 2022
Comment on lines +379 to +380
'foo.0.bar' => 'Required',
'foo.1.bar' => 'Required',
Copy link
Member

@kenjis kenjis Aug 6, 2022

Choose a reason for hiding this comment

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

Are you really okay with key like 'foo.0.bar'?
It seems a bit difficult to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But as I known, validation service change the asterisk field to like 'foo.0.bar'.
As Issue #6245 show that.

@@ -203,7 +203,7 @@ public function check($value, string $rule, array $errors = []): bool
* @param array|null $rules
* @param array $data
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param array $data
* @param array $data The array of data to validate, with `DBGroup`.
* @param string|null $originalField The original asterisk field name like "foo.*.bar".

@kenjis
Copy link
Member

kenjis commented Aug 7, 2022

What do you think if the three arguments ($field, $label, $originalField) are replaced with an array or a DTO?

It seems this fix needs BC break, so I'm fine with the change.

@kenjis
Copy link
Member

kenjis commented Aug 11, 2022

v4.2.3 has been released. See https://github.com/codeigniter4/CodeIgniter4/releases
This change will be included in v4.2.4.

Please rebase and add changelog v4.2.4.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

@kenjis kenjis added the stale Pull requests with conflicts label Aug 11, 2022
@ping-yee
Copy link
Contributor Author

ping-yee commented Aug 12, 2022

@kenjis Is this normal? or I do the wrong thing when I rebase my branch...
If I do the wrong thing, please tell me how can I fix it :(

@kenjis
Copy link
Member

kenjis commented Aug 12, 2022

@ping-yee This is not normal. You did something wrong.
What commands did you run?
And please don't use git merge.

@ping-yee
Copy link
Contributor Author

ping-yee commented Aug 13, 2022

@kenjis I use sourcetree to manage my git flow.
I syncing my repository first, and run rebase onto devlop branch.
After I resolve the conflicts, run rebase onto devlop branch again(Maybe there is a problem with this step).
Then I push the commit fa442b4 and get the results.
Should I re-open the new PR or do something to fix this?
I feel so sorry, this is my first time to contribute :(

@kenjis
Copy link
Member

kenjis commented Aug 13, 2022

I syncing your repository first, and run rebase onto devlop branch.

My repository? Do you mean https://github.com/kenjis/Codeigniter4 ?
If so, my repository has nothing to do with you.

After I resolve the conflicts, run rebase onto devlop branch again(Maybe there is a problem with this step).

You did rebase onto develop twice.
I don't know why your branch is like this.

Should I re-open the new PR or do something to fix this?

If you drop the commits other than you did for this PR (the commits marked below) with git rebase -i,
you can revert the branch state. Then rebase again correctly.

Screenshot 2022-08-13 16 56 26

But you don't use git command, I can't support you.

If you can re-open the new PR, that is also fine.

@ping-yee
Copy link
Contributor Author

ping-yee commented Aug 13, 2022

I syncing your repository first, and run rebase onto devlop branch.

@kenjis I say the wrong word. Not your repository, my repository is right word.
Thanks for your help and I'll use command to rebase my git flow!

@kenjis
Copy link
Member

kenjis commented Aug 13, 2022

Okay, if syncing your repository means this https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#syncing-your-repository , you just did rebase twice.

In my understanding, if you did rebase twice, the result is the same. The second rebase does nothing.
So I still have no clue for the status of your mess branch.

@ping-yee
Copy link
Contributor Author

ping-yee commented Aug 13, 2022

Would it be that I don't syncing my repo before I commit my change?
I'll try to fix the git flow later.

@kenjis
Copy link
Member

kenjis commented Aug 13, 2022

Would it be that I don't syncing my repo before I commit my change?

Sorry, I don't get your situation.
I thought you synced your repository, and did rebase twice.
But you say you did not sync?

After all, the current your branch is abnormal state.
There are two commits that are the same content.
git rebase never creates a new commit. It just change the existing commits.

Either you did something I couldn't anticipate, or Sourcetree ran strange commands that I couldn't anticipate.
In any case, I do not know of any way to get the branch into this state.

@ping-yee
Copy link
Contributor Author

I think my git flow is too mess to fixed, so I decide to re-open the PR.
Thanks for your help and teach again! @kenjis

@ping-yee ping-yee closed this Aug 15, 2022
@ping-yee ping-yee deleted the fix-validation-custom-error branch September 12, 2022 07:42
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 stale Pull requests with conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Custom validation error for a field with an asterisk.
6 participants