CodeIgniter4 icon indicating copy to clipboard operation
CodeIgniter4 copied to clipboard

fix: validation custom error with asterisk field

Open ping-yee opened this issue 1 year ago • 6 comments

Description Fixes #6245 Supersedes #6340

Checklist:

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

ping-yee avatar Aug 06 '22 14:08 ping-yee

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 avatar Aug 06 '22 14:08 iRedds

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

iRedds avatar Aug 06 '22 14:08 iRedds

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 avatar Aug 07 '22 01:08 kenjis

These methods have way too many parameters. I don't know that adding a new one is worse than a breaking change though. Definitely we should break this up somehow in version 5. If anyone can think of a way to handle this now as backwards-compatible I'm open to it.

MGatner avatar Aug 07 '22 11:08 MGatner

@iRedds @kenjis I have been changed declaration of methods to follow PSR-12 and added the comment out of $originalField parameter. @MGatner I agree with your thoughts that the methods have too many parameters.

ping-yee avatar Aug 08 '22 15:08 ping-yee

@ping-yee Add a description to the changelog.

iRedds avatar Aug 10 '22 03:08 iRedds

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 avatar Aug 11 '22 08:08 kenjis

@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 :(

ping-yee avatar Aug 12 '22 17:08 ping-yee

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

kenjis avatar Aug 12 '22 19:08 kenjis

@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 :(

ping-yee avatar Aug 13 '22 07:08 ping-yee

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.

kenjis avatar Aug 13 '22 08:08 kenjis

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!

ping-yee avatar Aug 13 '22 09:08 ping-yee

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.

kenjis avatar Aug 13 '22 09:08 kenjis

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

ping-yee avatar Aug 13 '22 09:08 ping-yee

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.

kenjis avatar Aug 13 '22 10:08 kenjis

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 avatar Aug 15 '22 08:08 ping-yee