[Form] Fixed handling groups sequence validation #36343
Conversation
9e61cd3 to
d5c375b
Compare
932eb16 to
c4ab608
Compare
src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
Show resolved
Hide resolved
c4ab608 to
a2ea4b1
Compare
src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
Show resolved
Hide resolved
b768608 to
ac17db3
Compare
|
This solution ruins the fields hierarchy in form errors and returns And it does not meet all of the sequence of groups requirements (e.g., when all the groups included in each array are validated). As it described here from |
|
Can you provide a failing test case for what you have in mind? |
There is a file with tests for that (https://github.com/symfony/symfony/pull/35556/files, All tests will fail because of |
faf06f1 to
9aa6963
Compare
|
Thanks @greedyivan for the review, I've add a test to cover nested array groups in sequence and push some fixes in 9aa6963. |
|
All tests from my PR passed. |
|
Thank you for confirming. |
9aa6963 to
dfb61c2
Compare
|
Thank you @HeahDude. |
This PR was squashed before being merged into the 3.4 branch (closes #36627). Discussion ---------- [Validator] fix lazy property usage. | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #36343 | License | MIT | Doc PR | This attempts to fix a large regression introduced in #36343, which broke recursing values returned from `getter` Constraints, because they are now wrapped in in a `LazyProperty`. The `LazyProperty` needs to be evaluated because some checks are done on the type of `$value`, i.e `is_array` etc... in `validateGenericNode`. I'm concerned that the original PR didn't really add sufficient test coverage for the introduction of `LazyProperty`, and I'm not 100% sure that I've caught all the cases where the `instanceof` check are needed in this PR. For the tests, I added the `@dataProvider getConstraintMethods` to every test that hit the problem area of code. ~~The only issue is that my fixed has broken the test introduced in #36343, `testGroupedMethodConstraintValidateInSequence`.~~ ~~I think I need @HeahDude to help me work through this. Maybe there is a more simple solution, one that doesn't require doing `instanceof LazyPropery` checks in multiple places, because this feels very brittle.~~ EDIT: fixed that test. Commits ------- 281861e [Validator] fix lazy property usage.
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).