Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form] Fixed handling groups sequence validation#36343
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9e61cd3 tod5c375bCompare932eb16 toc4ab608Comparesrc/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
c4ab608 toa2ea4b1Comparesrc/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.phpShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
b768608 toac17db3Comparegreedyivan commentedApr 12, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 |
xabbuh commentedApr 12, 2020
Can you provide a failing test case for what you have in mind? |
greedyivan commentedApr 12, 2020
There is a file with tests for that (https://github.com/symfony/symfony/pull/35556/files, All tests will fail because of |
faf06f1 to9aa6963CompareHeahDude commentedApr 15, 2020
Thanks@greedyivan for the review, I've add a test to cover nested array groups in sequence and push some fixes in9aa6963. |
greedyivan commentedApr 17, 2020
All tests from my PR passed. |
HeahDude commentedApr 17, 2020
Thank you for confirming. |
9aa6963 todfb61c2Comparenicolas-grekas commentedApr 18, 2020
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.
Uh oh!
There was an error while loading.Please reload this page.
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 toclose#35556 which tries to fix the same issue but afterwards in its implementation as said in#35556 (comment).