[Form] Consider a violation even if the form is not submitted #18935
Conversation
|
Can someone give me its opinion? |
|
Sounds reasonable to me. ping @symfony/deciders @webmozart |
|
Thank you @egeloen. |
…ted (egeloen) This PR was merged into the 2.7 branch. Discussion ---------- [Form] Consider a violation even if the form is not submitted | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | yes (only for the behavior) | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11493 | License | MIT | Doc PR | Hey! I'm currently implementing an API using the form component in order to validate the payload sent (in conjonction with the FOSRestBundle). Unfortunatelly, we dig into an issue about the PATCH request which don't map some of our validation rules to the form. Basically, the violations are lost in the middle of the process. ### Use case We have an entity with the following fields "type", "image" & "video". The field "type"can be either "default", "image" or "video" and then accordingly we use the appropriate field (none for the "default" type, video for the "video" type and image for the "image" type. Then, in our form, we change the validation groups according to our entity type in order to make the "image" field mandatory if the type is "image" and the same for the video field if the type is "video". ### Current behavior The current behavior (since 2.5) seems to not propages a violation to a form if this form is not submitted but in our use case, changing the field "type" via a PATCH request triggers some new validation which should be reported to end user (inform that a field (video or image) is missing in the PATCH request). ### Expected behavior The current behavior was introduced in #10567 but IMO, this update is a bug as suggested by @webmozart in #11493 (comment) Instead, the form component should still map validation errors to the form even if the field was not submitted. If the initial data is not valid, then your initial data was buggy from the beginning but the form should not accept it and instead of silently ignoring the errors, end users should be informed and fix it. WDYT? Commits ------- c483a0f [Form] Consider a violation even if the form is not submitted
|
@fabpot Would it be possible to get this in a 3.x release as well? |
|
@natebrunette All lower branches are merged up into all still maintained branches on a regular basis. So this will be included in the next patch releases of 2.7, 2.8, 3.0, and 3.1. |
|
This change introduices a BC Break and there is no warning in the release changelog. #19252. Why do Symfony does not respect its BC promise ? The previous behavior war handy while handling entity with a non persisted plainPassword. AppBundle\Entity\User:
constraints:
- Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity: email
properties:
firstname:
- NotBlank: ~
- Type: string
lastname:
- NotBlank: ~
- Type: string
email:
- NotBlank: ~
- Email: ~
plainPassword:
- NotBlank: ~
- Type: string
- Length:
min: 4
max: 50It was pretty easy to change the entity fields without specifying the plainPassword (using PATCH). Now it's mandatory unless we use validation groups and exclude the notBlank constraint when updating the user. This breaks all applications that try to change their user profiles using PATCH. |
|
Well, why making it NotBlank if you want it to be optional ? |
|
When creating a user it's mandatory. |
|
@Abam then put the NotBlank constraint in a separate validation group, applied only when creating a user. This is exactly what validation groups are for |
|
That's what I said in my first comment. Before this BC Break we haven't to do this boilerplate. |
|
@Abam everytime we fix a bug, it can impact people relying on this buggy behavior instead of using the supported ways of solving the same use case. If we go this way, we would never fix bugs. |
|
If a field is not submitted, it's not validated. I personnaly don't consider it as a buggy behavior. This was committed as a bug fix by the way #10567. |
|
I agree, that's not the form responsibility to validate preset data, it was designed that way, ref https://github.com/symfony/symfony/pull/10567/files#diff-7b14a00bf598c6870d7e3556f4bb4ff5R297. |
|
Now on my project, the update from 3.1.1 to 3.1.2 break my form. This is my case: I have a form, serialized into json to send them in api. If I need send all data and all constraint, I use PUT method. |
|
ping @fabpot I really think this one should be reverted |
|
Let me explain, the use case this PR fixes:
Now, you send a PATCH request with "foo" as field1 value. According to my rules, the field2 is added and according to my validation, it also becomes mandatory. Without this patch, this is not possible to validate the field 2 (as it has not been sent), but this is defintively not expected in my case... Even, if I add custom constraints directly on the form, the validation is not mapped to my form (lost in the violation mapper). I'm not against reverting this PR but then, a workaround solution should be proposed in order to support this use case... |
|
Btw, I propose the PR because @webmozart explains it does a mistake when adding this feature, see #11493 (comment) as well as @stof #11493 (comment). It also goes against the original issue reported, see #10567 (comment) If you read it, you will see that the original issue is basically the opposite of what has been implemented originally... |
The validation on the loaded object in PATCH request should not be the form responsibility. For your use case I think you should handle this in a PRE_SUBMIT event if it's not already the case. |
|
@egeloen there is not one way to handle things, we have a use case where in a form type we check submitted data and pre set data on |
|
Our code breaks because of this, here is another case: A user can submit an entity before a certain valid date, being 2 days before its dateUntil property. He/she does so correctly, the validation is OK. A backend user now needs to set this entity's state property to 'accepted'. When trying to do so, the system returns an error, telling us that it can no longer change the state because of the date-check, although it has only changed the state In this particular case, the data was valid when submitted. This small, one line change now forces us into changing a lot of code, having to introduce validation groups, where previously validation was implicitly (not) run. We vote for a revert :) |
|
@oenie Unless you use a PATCH request for your backend user, you should really think about using validation groups in such cases (that's not too much code), although I'm for reverting this one ;) |
|
👍 for reverting this. |
|
If this is reverted, could the issue of the form swallowing violations also be addressed? |
|
reverted |
|
@fabpot thanks :) @natebrunette is the scenario of #11493 covering your case? |
|
@HeahDude No, here's my example: I have an assertion that if Field A === true, Field B must exist |
|
We've merged the reverted PR into our project, and our tests are now failing because of what @natebrunette has mentioned. Is there a workaround to validate the entire object on PATCH, even if not all of the fields were sent with the request? |
|
@natebrunette This is also my use case, and @HeahDude suggests me to add a listener on the |
|
@egeloen That could work with extremely thorough testing, however, handling each case individually seems very prone to error. I would much prefer a standard way to inform symfony I would like the entire object validated. |
|
Maybe introducing an new form options and add a new param to |
I'd say yes, many:
|
|
I think this will be definitely solved with |
|
When release 3.1.4 with this revert commit ? |
|
Thanks for the revert ! |
|
Every fixes are merged in upper branches, and patch releases come out every month, so just be patient a few days :) |
|
Is not emergency, it's just to forecast update on the scheduled day |
|
@HeahDude: The Symfony release cycle is still a mystery to me ... thanks for enlightening me :) |
* 2.7: [VarDumper] Various minor fixes & cleanups Revert "bug #18935 [Form] Consider a violation even if the form is not submitted (egeloen)" [HttpKernel] Add missing SsiFragmentRendererTest Fixes the calendar in constructor to handle null
* 2.8: [VarDumper] Various minor fixes & cleanups Revert "bug #18935 [Form] Consider a violation even if the form is not submitted (egeloen)" [HttpKernel] Add missing SsiFragmentRendererTest [DoctrineBridge] Fix exception message and tests after misresolved merge Fixes the calendar in constructor to handle null
* 3.1: [VarDumper] Various minor fixes & cleanups Revert "bug #18935 [Form] Consider a violation even if the form is not submitted (egeloen)" [Config] Fix DirectoryResourceTest for symlinks [HttpKernel] Add missing SsiFragmentRendererTest [DoctrineBridge] Fix exception message and tests after misresolved merge Fixes the calendar in constructor to handle null
|
I have the same problem with $form->$isSubmitted(). The problem appear in validation groups. If field is not submitted, but mentioned in validation group. Validator found constraint, but form not map it. It can be solved by object direct validation. But here appear problem how combine form errors and object. Also i should validate object twice. Can $form->isSubmitted check be optional? Or ViolationMapper added to FormTypeValidatorExtension as service |
|
@kalinick Can you please open a new issue for your problem? |
|
Sure, #24453 |
Hey!
I'm currently implementing an API using the form component in order to validate the payload sent (in conjonction with the FOSRestBundle). Unfortunatelly, we dig into an issue about the PATCH request which don't map some of our validation rules to the form. Basically, the violations are lost in the middle of the process.
Use case
We have an entity with the following fields "type", "image" & "video". The field "type"can be either "default", "image" or "video" and then accordingly we use the appropriate field (none for the "default" type, video for the "video" type and image for the "image" type. Then, in our form, we change the validation groups according to our entity type in order to make the "image" field mandatory if the type is "image" and the same for the video field if the type is "video".
Current behavior
The current behavior (since 2.5) seems to not propages a violation to a form if this form is not submitted but in our use case, changing the field "type" via a PATCH request triggers some new validation which should be reported to end user (inform that a field (video or image) is missing in the PATCH request).
Expected behavior
The current behavior was introduced in #10567 but IMO, this update is a bug as suggested by @webmozart in #11493 (comment) Instead, the form component should still map validation errors to the form even if the field was not submitted. If the initial data is not valid, then your initial data was buggy from the beginning but the form should not accept it and instead of silently ignoring the errors, end users should be informed and fix it.
WDYT?