[Form] PATCH requests should not succeed if constraints fail

[jmcclell]

If I construct two objects Foo and Bar, such that:

class Foo
{
    /** @var Bar */
    public $bar;
}

class Bar
{
}

whereby Foo hasOne Bar, I would expect that a form constructed with a data_class of Foo would validate all properties of Foo which have constraints regardless of whether or not a default value for $bar is set. Instead, the form passes validation even if other fields in Foo which have constraints should fail, provided that I have set a default value for $foo->bar.

It's easier to demonstrate than explain:

https://github.com/jmcclell/recursive-validator-bug-test-case

In this repo, there is a single test class at src/Acme/DemoBundle/Tests/Controller/DemoControllerTest.php with two test cases. One hits a route where I do not define a default value for Foo::bar and one where I do. Both test cases provide an empty request to the route. I am expecting a 400 response in both cases, but as you can see, where I set the default I get a 200 response, even though Foo::Baz does not have its constraints met.

This means that if I were to place business logic inside the Form::isVaid() conditional which depends on the entities being validated, I am going to get entities that I cannot trust to be valid if the client sends a malformed (in this case, empty) request.

Curiously, if I run the object through the Validator manually, it performs as expected and returns the proper list of validation errors. It appears to be specific to form validation.

I ran into this bug with a specific setup of mine, so you'll notice a few things:

1. CSRF protection is disabled (my project is a RESTful API which is AJAX only)
2. Forms have no names (my client does not wrap its request in a namespace)
3. I am using submit() vs handleRequest() because in my specific case I need to strip out extra fields because forms do not support extra fields yet
4. I am using annotated constraints, in the test case you can find them in the src/Acme/DemoBundle/Entity folder for both the Foo and Bar entites

[jakzal]

Very good bug report! Thanks for taking your time to write this down and prepare reproducible scenario. We'll have a look at it soon.

[jmcclell]

Thanks, Jakub. Looking forward to seeing what the root cause of this is. It happens in both the legacy and new validation APIs. I tried tracking it down with no success.

[peterrehm]

I just looked into it. The problem is with your submit call().

At first the currently recommended way is

 $form->handleRequest($request);

When you replace your submit call with this you get the invalid statement.

You can get some more information about the actual submit here: http://symfony.com/doc/current/cookbook/form/direct_submit.html

You have submitted with the following call:

    $form->submit($request->request->all(), false);

You can actually get this to work with

    $form->submit($request->request->all());

This issue seems to be the second parameter of submit(). It defines that missing data
is being set to null if no data is being passed in the request. However it makes
actually no sense since the value is than actually null. https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/FormInterface.php#L264

I figured out if you validate manually after the submit with

   $this->get('validator')->validate($foo);

You get the proper error.

[jmcclell]

The reason I am using submit() instead of handleRequest() is because I need to filter out extra fields. Currently, forms do not support extra fields. Due to constraints I have in my project, the client does not send me its request wrapped in any kind of envelope. That is: the form fields are not contained under a form name namespace. Thus, all my form names are null. If I use handleRequest, extra fields (such as _format) will cause the form to barf. Submit is my only option until forms support an option to ignore extra fields.

As for the second parameter: the second parameter is clearMissing. In other words, if I don't send a field, the form should assume I mean to null it if $clearMissing = true. I don't want that behavior because I am supporting partial updates. I can't support partial updates if clearMissing = true.

So yes, if I run the entity through the validator manually I get the proper error. If I enable clearMissing I get the proper error. But, I still believe (strongly) that this is a bug because it should still work with clearMissing = false.

Can you convince me otherwise based on my requirements of supporting extra fields and needing to do partial updates?

[jmcclell]

@peterrehm I have managed to far simplify the test case, now that better I understand what is going on. It seems that this bug is purely a matter of calling submit() with an empty request which causes the form to enter some sort of invalid internal state whereby isValid() will return true regardless of the values of the underlying data_class. Please see the updated test case:

https://github.com/jmcclell/recursive-validator-bug-test-case

This test case demonstrates it with a single entity object and two text fields.

[jmcclell]

#10567

This is the change that introduced this bug/scenario. Because clearMissing = false and the data is empty, the field type (which is itself just a form) is not submitted, thus errors aren't applied to it because of conditions in the ValidationMapper which require that the form has been submitted.

#9998

This is the original bug which describes a similar situation to mine. I think my bug is an edge case to that bug. In this bug, he wants to make sure that data that is set programmatically still gets validated even if it isn't submitted as part of the form. I also want this. The edge case I seem to have stumbled upon is that empty requests cause the form to not report any validation errors.

I believe the proper behavior for a form which is bound to an entity would be to validate the underlying entity in its entirety for the given validation group(s). This works will full updates and it works with partial updates (PATCH) provided you load your initial entity from its source and use the form to override fields, which is what makes sense to me. What seems to be happening now is broken behavior, imho. It's possible that I'm not considering one or more other valid scenarios that my suggestion breaks, however.

[peterrehm]

It is actually strange behaviour and I userstand your use case.

These two change actually leads to the fact that unsubmitted forms are ignored:

https://github.com/symfony/symfony/pull/10567/files#diff-7b14a00bf598c6870d7e3556f4bb4ff5R300
https://github.com/symfony/symfony/pull/10567/files#diff-ca5e25b47f3ecc94cd557946aeb486c6R561

@webmozart Is this behaviour really intended? How about a setting to specifically define the behavior? I think usually the entire object should be validated, even if you submit only a few fields.

@jmcclell A solution for the meantime would be to validate the object manually. Than you can ensure for your case to have valid objects.

[jmcclell]

Thanks peterrehm. Currently I validate once at the controller level via the form so that I can return form errors back to the user as JSON (using FOSRestBundle in this API) and then I validate a second time at the service-level to catch this issue. It's not ideal but it affords me protection until the form behavior is sussed out.

[peterrehm]

I don't know how the FOSRestBundle works exactly, but I think you could add some way to return only your custom validation errors in the controller as JSON. So this would make it easy for you to change the behavior if symfony changes.

[webmozart]

Hm, I wonder why I implemented #10567 the way I did. I guess idea was to enable PATCH requests on objects with failing constraints on non-submitted fields. However, this goes against the logic of the form's validation behavior otherwise, namely that an entity must leave the form in a valid state. Forms usually fail also if constraints are violated on fields that are not mapped in the form.

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

Then any error after the PATCH must result from the patched data and hence requires correction.

Letting an invalid object be returned from "valid" forms and persisting that object sounds completely irrational to me. I'll let this settle over the weekend.

[jmcclell]

Aye. I agree with your current view on how the behavior should work. If I am to rely on the form component for validation, I need to be able to rely on it wholly and be able to safely assume that it will always honor all constraints.

As for your original idea, perhaps there is another way to go about that, but I am not sure it is really necessary. I believe in most cases people will be loading the original entity anyway in order to perform the PATCH operation, and any entity that already exists should already be in a valid state. One would hope, at least.

[peterrehm]

@webmozart I would agree to your comment that the object should be in a valid state before the patch or at least after the patch.

[cblegare]

I have found a work-around for some use-cases of this issue using a somewhat-dirty-hack.

As of Symfony 2.4, if you set the fields you really want validated with error_bubbling => true, validation errors will be added to the form's parent (see Form::addError). One of these parents will be submitted (the root form is always submitted, isn't it ?) and the error will be added to a submitted field, hence the check for $child->isSubmitted() && !$child->isValid() found in Form::isValid will pass and the error can be reported.

This let your invalid objects be effectively invalid, but wrongs the validation error's location in the form tree.

This results in a behavior somewhat similar to adding a superfluous $this->get('validator')->validate($foo); call as stated above by @peterrehm : Correct validation behavior but incorrect error locations.

In my case, the immediate parent form can be entirely ignored safely, hence I can stop the bubbling at this point. The validation error is then reported near the actual error location, which is the best I could manage to achieve.

[EDIT]

To circumvent this issue, you can use a event listener that forces the submission of null values the same way it would be done with $clearMissing = false.

/** 
 * $builder->add(...) the fields that MUST be validated properly
 */

$builder->addEventListener(
    // PRE_SUBMIT lets you set the data to be submitted
    FormEvents::PRE_SUBMIT,
    function (FormEvent $event) use ($builder) {
        $submittedData = $event->getData();
        // The already submitted data is kept  unchanged
        $dataToSubmit = $submittedData;
        foreach ($builder->all() as $name => $subType) {
            /** @var FormBuilderInterface $subType */
            // Here, the 'required' value determine whether this field must be validated properly
            if ($subType->getRequired() && !array_key_exists($name, $submittedData)) {
                // Force submission of a null value, exactly the same way
                // it would be done with $clearMissiing = false
                $dataToSubmit[$name] = null;
            }
        }
        $event->setData($dataToSubmit);
    }
);

This solution is only intended to be a temporary patch in use-cases where this issue is a blocker.

[stof]

@webmozart I agree with you that it does not make sense

[guilhermeblanco]

I'd add to this list the issues I've faced while trying to make GET requests to work: https://gist.github.com/guilhermeblanco/287e67fff9d1dcbc2f13

[Paxton]

Any updates on this? This behaviour is still present as far as I am aware.

[devantoine]

+1

It's been more than two years this issue has been raised. With REST API being more and more used, lot of people with face this issue.

[xabbuh]

Can you confirm that the changes proposed in #30265 will fix this issue?