-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 cannot handle array item #5405
fix: Validation cannot handle array item #5405
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is a bit hard to review on mobile but what I see all looks good, and the tests are great.
Your User Guide write-up is very thorough, but I would rather see that in the changelogs (i.e. user_guide_src/source/changelogs/v4.1.6.rst) and keep the Upgrading section as actual tasks that need to be done when upgrading. A simple sentence with reference to the longer explanation in the changelog would be good, like "Due to a bug fix Validation now might ____ (see ____.rst), so make sure you ____ to any code calling it."
Will this conflict with the Strict Validation changes? |
43757f8
to
9115994
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc changes look good! Since this is a breaking change I'd like it if you could get another reviewer. I'll ping the team on Slack.
…has actual tasks that need to be done when upgrading
9115994
to
0485bb7
Compare
} | ||
} else { | ||
// Process single field | ||
$this->processRules($field, $setup['label'] ?? $field, $values, $rules, $data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this one is the same with L145, I think we can combine this with that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I remove the if
statement from L143, tests in tests/system/Validaton/
passes.
But I'm not sure removing it causes no problem.
What input makes $values
empty array?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added continue
in the if
to fix the bug that $this->processRules()
run twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What input makes
$values
empty array?
This is raised in an issue report. I'll check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read the comment and the fix PR.
But I'm not sure we can remove the if ($values === []) { ... }
.
I feel it is safer not to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@paulbalandan I want to update upgrade_416.rst
, so I merge this for now.
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>
Description
Supersedes #5402
Fixes #5368
Checklist: