Can someone give me its opinion?

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

Thank you@egeloen.

@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 anon persisted plainPassword.

AppBundle\Entity\User:constraints:        -Symfony\Bridge\Doctrine\Validator\Constraints\UniqueEntity:emailproperties:firstname:            -NotBlank:~            -Type:stringlastname:            -NotBlank:~            -Type:stringemail:            -NotBlank:~            -Email:~plainPassword:            -NotBlank:~            -Type:string            -Length:min:4max: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.

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, refhttps://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 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.

ping@fabpot I really think this one should be reverted

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

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

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.

@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 onPRE_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.ghttps://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.

Our code breaks because of this, here is another case:
Consider an entity with a propertydateUntil and astate 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'sstate 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 thestate

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

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

@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

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

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 theentire 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 thePRE_SUBMIT event and if fieldA is true, then add a fake empty fieldB withsetData. 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.

@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 toViolationMapper::mapViolation but it would require to break theViolationMapperInterface...

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 anull 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())

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

When release 3.1.4 with this revert commit ?
Thanks

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

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

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