[Form] Consider a violation even if the form is not submitted

[egeloen]

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?

[egeloen]

Can someone give me its opinion?

[fabpot]

Sounds reasonable to me. ping @symfony/deciders @webmozart

[fabpot]

Thank you @egeloen.

[natebrunette]

@fabpot Would it be possible to get this in a 3.x release as well?

[xabbuh]

@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.

[Abam]

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: 50

It 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.

[stof]

Well, why making it NotBlank if you want it to be optional ?

[Abam]

When creating a user it's mandatory.

[stof]

@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

[Abam]

That's what I said in my first comment. Before this BC Break we haven't to do this boilerplate.

[stof]

@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.

[Abam]

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.

[HeahDude]

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.

[jewome62]

Now on my project, the update from 3.1.1 to 3.1.2 break my form.
This is for me a BC, not a bugfix.

This is my case:

I have a form, serialized into json to send them in api.
If the form is send to api with PATCH, I take in consideration only the fields sent.
If I do not sent a field, It isn't use for update doctine entity.
So if I do not sent a field, It isn't use for constraint validation.

If I need send all data and all constraint, I use PUT method.

[HeahDude]

ping @fabpot I really think this one should be reverted

[egeloen]

Let me explain, the use case this PR fixes:

• I have a form with one field (given field1)
• I have an other field (given field2) which is only added when field1 has a specific value (given "foo") and when this field is added (fied1 === 'foo'), field2 is mandatory.

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...

[egeloen]

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...

[HeahDude]

I feel inclined to revert #10567 for that reason. If someone wants to PATCH an object, they must ensure that the object is in a valid state after the request by ensuring that the object is already valid before the PATCH operation:

• either the object is newly created - then the fields must be set to valid defaults before doing the PATCH
• or the object is loaded from an object store - then the object must be valid already

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.

[HeahDude]

@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 PRE_SUBMIT event then conditionally use $form->get('field')->addError(new FormError (...)), sometimes it's just more convenient than having a constraint with dependency on the entity itself through validation mapping (e.g https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php#L80) and that whatever the method is, even being able to check it too ;)
We use a simple callable but this could be in a proper listener class in order to be reused, validation of a form only happens thanks to a listener.

[oenie]

Our code breaks because of this, here is another case:
Consider an entity with a property dateUntil and a state property.

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.
It still is VALID in the sense that the dateUntil was correct at the time that it needed to be.
That validation is no longer important to further changes to the data.

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 :)

[HeahDude]

@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 ;)

[jewome62]

@HeahDude @oenie
I don't think we are right to vote, but this is @symfony/deciders to do this.
But I'm for reverting too

[xabbuh]

👍 for reverting this.

[natebrunette]

If this is reverted, could the issue of the form swallowing violations also be addressed?

[fabpot]

reverted

[HeahDude]

@fabpot thanks :)

@natebrunette is the scenario of #11493 covering your case?

[natebrunette]

@HeahDude No, here's my example:

I have an assertion that if Field A === true, Field B must exist
If a PATCH request is made changing Field A to true, but failing to submit B, I add a violation on Field B
This violation is then ignored because Field B was not part of the PATCH and the entity is persisted

[mattjanssen]

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?

[egeloen]

@natebrunette This is also my use case, and @HeahDude suggests me to add a listener on the PRE_SUBMIT event and if fieldA is true, then add a fake empty fieldB with setData. I personally find this behavior strange but I can live with it. The funny part is that the original issue which have triggered this "feature" is the opposite of what has been implemented.

[natebrunette]

@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.

[egeloen]

Maybe introducing an new form options and add a new param to ViolationMapper::mapViolation but it would require to break the ViolationMapperInterface...

[HeahDude]

Is there a workaround to validate the entire object on PATCH, even if not all of the fields were sent with the request?

I'd say yes, many:

• don't use PATCH to submit fully => validate fully,
• use a listener on PRE_SUBMIT, if a value is required whatever the method is, you can check it there and either add a null value for the required field in the submitted data array (see my comment above) or use $form->addError() (see my other comment above),
• in a controller handling PATCH method do $errors = $this->get('validator')->validate($form->getData())

[HeahDude]

I think this will be definitely solved with clear_missing as an option to override the PATCH behavior. See #17771

[jewome62]

When release 3.1.4 with this revert commit ?
Thanks

[oenie]

Thanks for the revert !
A merge into 2.8 would be welcome too :)

[HeahDude]

Every fixes are merged in upper branches, and patch releases come out every month, so just be patient a few days :)

[jewome62]

Is not emergency, it's just to forecast update on the scheduled day

[oenie]

@HeahDude: The Symfony release cycle is still a mystery to me ... thanks for enlightening me :)

[kalinick]

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

[xabbuh]

@kalinick Can you please open a new issue for your problem?

[kalinick]

Sure, #24453