-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Validator] Allow validating multiple groups in one GroupSequence step #19982
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
Conversation
5998dc4
to
5feca1c
Compare
5feca1c
to
31b609e
Compare
@fabpot Just found out that conditional validations on subforms are a real pain at the moment since the validation groups can only be defined on root forms. This PR provides a far better solution because then I can override the Default validation group by implementing GroupSequenceProvider. |
ping @HeahDude |
Simple and efficient 👍 I really like this parentheses' move feature :) |
Thank you @enumag. |
…oupSequence step (enumag) This PR was merged into the 3.2-dev branch. Discussion ---------- [Validator] Allow validating multiple groups in one GroupSequence step | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | To see what I've changed just look at the last commit. The other two are just refactoring without any effect. GroupSequenceProviderInterface seems to be the recommended solution for conditional validators (like when some properties should be required only if other property has a certain value). And it's a good solution honestly. Except for the fact that it's completely useless when I want to get validation violations for all groups at once and not just violations for the first group that caused any violations. ```php // If the User validation group causes any violations the Premium group will not be // validated at all even if the $this->isPremium() is true. class User implements GroupSequenceProviderInterface { public function getGroupSequence() { $groups = array('User'); if ($this->isPremium()) { $groups[] = 'Premium'; } return $groups; } } ``` To be honest I never found a use case for this step-by-step behavior. When user fills a form I want to show him all errors at once not just some subset. It's surprisingly easy to fix this. With just one changed line in RecursiveContextualValidator it's perfectly solveable: ```php // With this PR it is possible to do this and get violations for both groups like this. class User implements GroupSequenceProviderInterface { public function getGroupSequence() { $groups = array('User'); if ($this->isPremium()) { $groups[] = 'Premium'; } return [$groups]; // this line has changed } } ``` Commits ------- 31b609e [Validator] Allow validating multiple groups in one GroupSequence step 3847bad [Validator] Refactor tests 38b643a [Validator] Fix annotation
@fabpot Thank you! I was going through the code again and found one thing that doesn't seem right. I couldn't find any usage of @webmozart You were the one who originally added the cascadedGroup. Am I right about what I've written in the previous paragraph? If you can confirm I'm right about this I'll send another PR. Also cascadedGroup should be allowed to be an array as well which means changing |
@blazarecki I don't think so. That issue is about propagation of groups for the |
@enumag Can you open a new PR with what you have in mind? Not everyone is following already merged PRs so discussions here may get lost. |
@xabbuh Alright. Hopefully I can find some time this weekend. |
This PR was squashed before being merged into the 3.4 branch (closes #32044). Discussion ---------- [Validator] Fix GroupSequenceProvider annotation | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | The possibility was added in #19982, just forgot to fix this annotation back then. Commits ------- bf6d253 [Validator] Fix GroupSequenceProvider annotation
To see what I've changed just look at the last commit. The other two are just refactoring without any effect.
GroupSequenceProviderInterface seems to be the recommended solution for conditional validators (like when some properties should be required only if other property has a certain value). And it's a good solution honestly. Except for the fact that it's completely useless when I want to get validation violations for all groups at once and not just violations for the first group that caused any violations.
To be honest I never found a use case for this step-by-step behavior. When user fills a form I want to show him all errors at once not just some subset.
It's surprisingly easy to fix this. With just one changed line in RecursiveContextualValidator it's perfectly solveable: