[Form] Fixed handling groups sequence validation #36343
Conversation
HeahDude
force-pushed
the
fix/form-validate_sequences
branch
from
9e61cd3
to
d5c375b
Compare
HeahDude
force-pushed
the
fix/form-validate_sequences
branch
3 times, most recently
from
932eb16
to
c4ab608
Compare
src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
Show resolved
Hide resolved
HeahDude
force-pushed
the
fix/form-validate_sequences
branch
from
c4ab608
to
a2ea4b1
Compare
src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php
Show resolved
Hide resolved
HeahDude
force-pushed
the
fix/form-validate_sequences
branch
8 times, most recently
from
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 |
HeahDude
force-pushed
the
fix/form-validate_sequences
branch
from
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. |
HeahDude
force-pushed
the
fix/form-validate_sequences
branch
from
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).