[Validator][GroupSequences][Form] filter out excess violations while validating form field constraints with sequence of groups #35556
Conversation
b6c71db to
b5fb135
Compare
src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
Outdated
Show resolved
Hide resolved
...Symfony/Component/Form/Tests/Extension/Validator/Type/FormTypeValidatorExtensionProvider.php
Outdated
Show resolved
Hide resolved
…with sequence of groups
b5fb135 to
6be9d50
Compare
| @@ -65,6 +65,9 @@ public function validate($form, Constraint $formConstraint) | |||
|
|
|||
| if ($groups instanceof GroupSequence) { | |||
| $validator->atPath('data')->validate($form->getData(), $constraints, $groups); | |||
|
|
|||
| $this->filterFormFieldsGroupSequenceViolations($groups); | |||
There was a problem hiding this comment.
What about iterating the groups provided by the group sequence instead and check after each iteration whether or not the number of violations increased? We would then stop iterating once the number of violations changed.
With the current solution validators for constraints in "later" groups are still executed which they are not supposed to be.
There was a problem hiding this comment.
Yes, I thought about that.
Fields that are validated in FromValidator have a form field order that is not related to the group validation order. Thus, in general, at this stage, we know almost nothing about the need for validation of the field. We know nothing about any other form fields that have been or will be validated, as well as of their groups.
The second point is that if we try to filter constraints by group before validation, we must reproduce all the logic of the group sequence, which is incorporated in the validator itself and does not belong to FromValidator extension. But this is not an ultimate solution, because we do not know which field with which groups go further. And we must filter out violations after each verification anyway.
Only filtering violations is the lesser evil here.
|
Thank you for starting the work on this @greedyivan. I am closing here in favour of #36343. |
This PR was merged into the 3.4 branch. Discussion ---------- [Form] Fixed handling groups sequence validation | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | FIx #9939 (comment), Fix #35556 | License | MIT | Doc PR | ~ This is not the same as the original issue fixed by #36245, that was reported in #9939 (comment). The form also fails to cascade sequence validation properly because each nested field is validated against the sequence, and one can fail at a step independently from another which could failed in another step. I've added a lot of tests to ensure this is working properly and tested in a website skeleton too. This PR aims to close #35556 which tries to fix the same issue but afterwards in its implementation as said in #35556 (comment). Commits ------- dfb61c2 [Form] Fixed handling groups sequence validation
This PR was merged into the 3.4 branch. Discussion ---------- [Form] Fixed handling groups sequence validation | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | FIx symfony/symfony#9939 (comment), Fix #35556 | License | MIT | Doc PR | ~ This is not the same as the original issue fixed by #36245, that was reported in symfony/symfony#9939 (comment). The form also fails to cascade sequence validation properly because each nested field is validated against the sequence, and one can fail at a step independently from another which could failed in another step. I've added a lot of tests to ensure this is working properly and tested in a website skeleton too. This PR aims to close #35556 which tries to fix the same issue but afterwards in its implementation as said in symfony/symfony#35556 (comment). Commits ------- dfb61c204c [Form] Fixed handling groups sequence validation
Because all form fields are validated independently by design, sequence of groups of constraints somehow need to be restored after each validation to remove all excesses violations that must be omitted by the sequence rules.
Tests for some cases were added.
Any comments are welcome.