[Form] Disabled validation/violation mapping of unsubmitted forms #10567

webmozart · 2014-03-28T09:49:13Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#9998
License	MIT
Doc PR	-

This PR fixes submissions of PATCH form in the case that any of the non-submitted forms causes an error (such as not-null).

Tobion · 2014-03-28T10:15:45Z

Just to make sure, if a user removes an input field from a form manuelly (like firebug), which means the field is not submitted anymore, the field on the model will still be validated, or? Otherwise it would be a security problem.

stof · 2014-03-28T10:36:17Z

@Tobion what is not validated anymore are constraints attached on the field itself (and only for PATCH)

webmozart · 2014-03-28T11:53:00Z

@Tobion Good point. But that means that even for a PATCH request, one would have to specify which fields are submitted, leading the current implementation of PATCH requests (with the $clearMissing flag) ad absurdum...

webmozart · 2014-03-28T11:54:02Z

For reference, the test fails are caused by the Process component and are not related to this PR.

webmozart · 2014-03-28T11:59:56Z

Probably related: Patching like we do it now is convenient, but bogus according to the HTTP spec (according to @willdurand).

fabpot · 2014-03-30T07:44:36Z

So, what are we doing here? Close or merge this PR?

Tobion · 2014-03-30T13:48:27Z

This change doesn't seem to be for PATCH only but also for other methods?

webmozart · 2014-03-30T16:25:51Z

I thought this through again now. This change only applies if the $clearMissing flag is set to false (i.e. in PATCH requests), because otherwise all fields will always be submitted (and consequently validated).

That means, for PATCH requests it is possible to remove fields from the form's HTML. Those fields will neither be submitted nor validated.

@Tobion Sounds OK?

koemeet · 2014-03-30T17:00:20Z

Seems to me that this needs to work. Where clearMissing is false and the field something is not submitted.

public function postUsersAction()
{
    // processForm is a pseudo function, it will return a form instance
    $user = new User();
    if ($this->getUser()) {
        $user->setSomething($this->getUser()->getSomething());
    }

    return $this->processForm(new UserType(), $user);
}

Tobion · 2014-03-30T17:43:27Z

@webmozart I'm ok with it when it's only for PATCH. And according to http://www.w3.org/TR/html5/forms.html#attr-fs-method PATCH cannot be used in forms anyway. So this would be for API style only. And that our definition of PATCH is not what is was supposed to be (a diff approach with change rules), is a different story.

What I wonder in #9998

But a side effect of using $clearMissing is that fields witch are not submitted in the request are not validated by the $form->isValid() method.

Does he mean the opposite? Because that seems to be the new behavior and not the old.

webmozart · 2014-03-30T18:24:18Z

Judging from the test controller he posted he meant the opposite. :)

But a side effect of using $clearMissing is that fields witch are not submitted in the request are validated by the $form->isValid() method.

Tobion · 2014-03-30T22:49:18Z

👍

webmozart · 2014-03-31T08:21:09Z

@steffenbrem Although I see that this could happen, I'm not sure when your example would make sense. What you're saying is:

a form has fields A and B
whenever A is submitted, setB(something) is called
if I PATCH with only field A, errors on B are discarded

However, why is B present in your form when you call setB() programmatically anyway? It seems to me that in reality, the implementation would be more like:

a form has the field A
whenever A is submitted, setB(something) is called
errors of B are mapped to A
if I PATCH with field A, errors of B are displayed on A

This example shows that the PR is flawed in one sense: All fields (A and B in this example) should always be validated. However, errors mapped to unsubmitted fields should be ignored. I'll change the PR accordingly.

webmozart · 2014-03-31T08:32:32Z

@fabpot Updated, ready to merge.

…d forms (webmozart) This PR was merged into the 2.5-dev branch. Discussion ---------- [Form] Disabled validation/violation mapping of unsubmitted forms | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #9998 | License | MIT | Doc PR | - This PR fixes submissions of PATCH form in the case that any of the non-submitted forms causes an error (such as not-null). Commits ------- 9dfebd5 [Form] Disabled violation mapping of unsubmitted forms

Seb33300 · 2014-05-26T19:50:52Z

Hi, it seems that this fix is not included in the latest symfony release.
Is it normal?

wouterj · 2014-05-26T20:43:47Z

@Seb33300 this was merged into the 2.5-dev branch. It'll be included in the 2.5.0 stable release, which will be released at the end of this month.

iamluc · 2014-07-17T08:24:14Z

This commit has a side effect on UniqueEntityValidator with multiple fields.

By default, the first field of the constraint holds the violation.
But if this field is not submitted, then the violation is ignored.

A solution is to change the errorPath to an always submitted field, or to "" (to put the violation at the fields parent level).

stof · 2014-07-17T09:32:46Z

the validator does not know which fields are submitted or no (it runs on the object graph, not on the form).
Putting the error on the parent makes it harder to map errors to a field (but it might be needed when there is several fields though)

iamluc · 2014-07-17T09:59:32Z

Yes, "my" solution was a workaround for developpers who encountered the same problem.
Not a solution to the root of the bug.

This takes into account the changes to the `getErrors()` in Symfony 2.5 (the method rturns a `FormErrorIterator` instead of an array) as well as the fact that non-submitted forms do not accept errors since symfony#10567 anymore.

This PR was merged into the 2.7 branch. Discussion ---------- [Form] fix violation mapper tests | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17099 | License | MIT | Doc PR | This takes into account the changes to the `getErrors()` in Symfony 2.5 (the method rturns a `FormErrorIterator` instead of an array) as well as the fact that non-submitted forms do not accept errors since #10567 anymore. Commits ------- f87558d [Form] fix violation mapper tests

Seb33300 · 2016-02-20T13:11:45Z

@webmozart

Judging from the test controller he posted he meant the opposite. :)

I think you misunderstood what I meant in #9998
I meant that missing inputs should be validated like if they are not missing. (like the "name" input in my example). Because we can pass an entity already filed with the missing inputs to the form, so the form will validate the pre-filled entity.

This pull request didn't change anything and the issue I reported still exists. Missing inputs still be ignored.

It seems that you did the opposite of what I reported.

@webmozart

…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

linxlad · 2016-10-28T14:18:21Z

Just a heads up this broke our validation due to the unusual way we run our apps. The app has to redirect after form submissions (due to the fact the app is embedded in multiple websites) saving the values and errors in Redis then re-applying to the form after the redirect has completed but at that stage, the form isSubmitted returns false. This is due to not having the original form state (as it is a redirect the form object has to be created again from new). So when the errors are manually applied back in this block of code:

public function unserialize(Form $form, array $errors)
{
    $errorMap = $this->createErrorMap($errors);

    foreach ($errorMap as $field) {
        $formError = $this->createNewFormError($form, $field['error'], end($field['path']));
        $this->mapper->mapViolation($formError->getCause(), $form);
    }
}

the scope in mapViolation doesn't not meet the acceptsErrors check due to the fact the form object is no longer submitted.

When a form field is not embedded as part of a HTTP PATCH requests, its validation constraints configured through the `constraints` option must not be evaluated. The fix from symfony#10567 achieved this by not mapping their violations to the underlying form field. This however also means that constraint violations caused by validating the whole underlying data object will never cause the form to be invalid. This breaks use cases where some constraints may, for example, depend on the value of other properties that were changed by the submitted data.

…requests (xabbuh) This PR was merged into the 3.4 branch. Discussion ---------- [Form] do not validate non-submitted form fields in PATCH requests | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11493, #19788, #20805, #24453, #30011 | License | MIT | Doc PR | When a form field is not embedded as part of a HTTP PATCH requests, its validation constraints configured through the `constraints` option must not be evaluated. The fix from #10567 achieved this by not mapping their violations to the underlying form field. This however also means that constraint violations caused by validating the whole underlying data object will never cause the form to be invalid. This breaks use cases where some constraints may, for example, depend on the value of other properties that were changed by the submitted data. Commits ------- a60d802 do not validate non-submitted form fields in PATCH requests

webmozart mentioned this pull request Mar 28, 2014

[Bug] [Form] Fixed issue #9998 (partial data) #10515

Closed

[Form] Disabled violation mapping of unsubmitted forms

9dfebd5

fabpot merged commit 9dfebd5 into symfony:master Mar 31, 2014

webmozart deleted the issue9998 branch April 8, 2014 12:33

jmcclell mentioned this pull request Jul 30, 2014

[Form] PATCH requests should not succeed if constraints fail #11493

Closed

bwoodmansee mentioned this pull request Nov 20, 2014

[Form] ViolationMapper - Make form submission requirement optional #12528

Closed

xabbuh mentioned this pull request Feb 16, 2016

[Form] fix violation mapper tests #17820

Merged

egeloen mentioned this pull request Jun 1, 2016

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

Merged

xabbuh mentioned this pull request Feb 15, 2019

[Form] do not validate non-submitted form fields in PATCH requests #30265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Disabled validation/violation mapping of unsubmitted forms #10567

[Form] Disabled validation/violation mapping of unsubmitted forms #10567

webmozart commented Mar 28, 2014

Tobion commented Mar 28, 2014

stof commented Mar 28, 2014

webmozart commented Mar 28, 2014

webmozart commented Mar 28, 2014

webmozart commented Mar 28, 2014

fabpot commented Mar 30, 2014

Tobion commented Mar 30, 2014

webmozart commented Mar 30, 2014

koemeet commented Mar 30, 2014

Tobion commented Mar 30, 2014

webmozart commented Mar 30, 2014

Tobion commented Mar 30, 2014

webmozart commented Mar 31, 2014

webmozart commented Mar 31, 2014

Seb33300 commented May 26, 2014

wouterj commented May 26, 2014

iamluc commented Jul 17, 2014

stof commented Jul 17, 2014

iamluc commented Jul 17, 2014

Seb33300 commented Feb 20, 2016

linxlad commented Oct 28, 2016 •

edited

Loading

[Form] Disabled validation/violation mapping of unsubmitted forms #10567

[Form] Disabled validation/violation mapping of unsubmitted forms #10567

Conversation

webmozart commented Mar 28, 2014

Tobion commented Mar 28, 2014

stof commented Mar 28, 2014

webmozart commented Mar 28, 2014

webmozart commented Mar 28, 2014

webmozart commented Mar 28, 2014

fabpot commented Mar 30, 2014

Tobion commented Mar 30, 2014

webmozart commented Mar 30, 2014

koemeet commented Mar 30, 2014

Tobion commented Mar 30, 2014

webmozart commented Mar 30, 2014

Tobion commented Mar 30, 2014

webmozart commented Mar 31, 2014

webmozart commented Mar 31, 2014

Seb33300 commented May 26, 2014

wouterj commented May 26, 2014

iamluc commented Jul 17, 2014

stof commented Jul 17, 2014

iamluc commented Jul 17, 2014

Seb33300 commented Feb 20, 2016

linxlad commented Oct 28, 2016 • edited Loading

linxlad commented Oct 28, 2016 •

edited

Loading