[Form] Prevent model update if form validation fails #5480

webmozart · 2012-09-10T16:39:15Z

As of Symfony 2.1, each field supports custom constraints in the "constraints" option. Constraints in this option should (probably) prevent the appropriate field in the model from being updated. Updating the model before launching validation was repeatedly criticized in Symfony2. This change could give the best of both worlds.

Example:

<?php

$builder->add('ip', 'text', array(
    // setIp() will not be called if the regex fails
    'constraints' => new Regex('~^(\d{3}\.){3}\d{3}/\d{1,3}$~'),
));

I'm not sure yet whether this can be implemented at all. Thoughts?

rcambrj · 2012-09-21T13:02:40Z

I'd also like to see this behaviour implemented much like it was in symfony1.

I don't mind accepting new ideas, though, and I welcome the opportunity to teach why it is that invalid data is now settable on a model and how an application should behave to deal with this.

As per what I've read everywhere, invalid data should never be set on a model, but this is exactly what's happening.

See mailing list discussion - https://groups.google.com/forum/?fromgroups=#!topic/symfony2/kto13ZgH3CA

tkleinhakisa · 2012-09-21T13:11:59Z

+1 for this,

It is really not natural to have invalid data set on models when validation fails.
Request data should only go to the entity if validation is ok

Thx

stof · 2012-09-21T22:06:43Z

@tkleinhakisa the validator component is validating the entity, not the form.

rcambrj · 2012-09-24T08:40:43Z

@stof It would be great if you could expand your argument, preferably backing it up with any resources which might clear the air on this issue.

I'm more than keen to work with this approach, but I need to understand it before I can! These two pages leave the whole issue somewhat untouched but for one line:

http://symfony.com/doc/current/book/validation.html
http://symfony.com/doc/current/book/forms.html

As soon as bind() is called, the submitted data is transferred to the underlying object immediately. This happens regardless of whether or not the underlying data is actually valid.

This suggests that even the author is aware that this approach is non-standard enough to put a note in to warn the reader.

tkleinhakisa · 2012-09-24T08:42:53Z

Hi stof and thx for your comment.
I think it is misleading that the isValid method is called on the form object. Correct me if i'm wrong :

the form put the data from the request on the entity on bind
when calling isValid, the form triggers validation on the entity using the validator

Could it be possible for the form to "rollback" the entity to its original state if validation fails on that entity ? some kind of "unbind" method ?

stof · 2012-09-24T08:46:21Z

@tkleinhakisa the Validator is called during bind (it uses a POST_BIND listener on the form). isValid simply checks if some errors have been atatched to the form.

tkleinhakisa · 2012-09-24T09:12:59Z

@stof Thx for the explanation

What about "rollbacking" ? Do you think this could be done ? do you see any issue with that idea ? or is it in your opinion a wrong way to go ?

webmozart · 2012-10-08T14:35:25Z

I am currently implementing this ticket, but there are two possible strategies. Check the following example:

<?php
$builder->add('name', 'text', array(
    'constraints' => array(
        new NotBlank(),
        new MinLength(4),
    ),
));
$builder->add('ip', 'text', array(
    'constraints' => new Regex('~^(\d{3}\.){3}\d{3}/\d{1,3}$~'),
));

The first strategy is to prevent setName() from being called if the "name" field is invalid, and setIp() from being called if the "ip" field is invalid.
The second strategy is to prevent setName() and setIp() from being called if any of the fields is invalid

Strategy 2 prevents model updates completely if an error occurred. But it also has the big disadvantage that constraints defined in the model are completely ignored unless all constraints defined in the form are satisfied.

Opinions?

fabpot · 2012-10-08T14:42:34Z

It feels wrong to me. We would have two validation processes now: one for the constraints set on the entity properties and methods, and another one for the constraints set in the form directly. Even if we can explain that, it feels odd. I like the idea of being able to rollback to the previous values is something went wrong... and it should be possible.

everzet · 2012-10-08T14:46:51Z

Rollback sounds like a good option to me, but it should be optional, cuz sometimes you'll want to show incomplete values to user instead of giving him back empty entity.

webmozart · 2012-10-08T14:47:04Z

We cannot generically roll back side effects of the setters (for example internal counters that are incremented upon each call etc.).

webmozart · 2012-10-08T14:47:39Z

@everzet If a field is submitted with an incorrect value, that value is displayed again, it is just not written to the underlying entity.

stof · 2012-10-08T14:48:10Z

The downside of strategy 1 is that if you have a validation rule on the model comparing 2 fields (checking that the start date is before the end date for instance), it could give a wrong error message if the start date has been set but no the end date (because of an invalid date being passed) as it would compare it with the old end date which has nothing to do with the value seen by the suer in the form.

But anyway, preventing the setter to be called when the data transformer fails is important. The following case results in a fatal error when the date passed in the form cannot be parsed as it will try to inject null:

class Foo
{
    /** @Assert\NotBlank() */
    private $date;

    public function getDate()
    {
        return $this->date;
    }

    public function setDate(\DateTime $date)
    {
        $this->date = $date;
    }
}

everzet · 2012-10-08T15:01:56Z

@bschussek right... Stupid me. Thanks for reminding :)

webmozart · 2012-10-08T20:34:47Z

@stof This problem can be solved by creating a group sequence with the two dates in the first group and the combined check in the second group.

Can anyone come up with a smarter strategy than strategy 1? Any severe limitations with that?

rcambrj · 2012-10-08T23:07:05Z

As I suggested, the entity could be cloned (or an entity validatee class specified).

I know I'm not the only one to think this is a clean solution.

stof · 2012-10-09T00:53:00Z

@rcambridge If you clone the entity, you are not binding to the same object anymore. In case of Doctrine, it would mean you are creating a new object instead of editing an existing one.

rcambrj · 2012-10-09T08:56:03Z

@stof A fair point. It can be worked around, but I realise now if the core of the issue (not setting in the first place) is being solved, this becomes somewhat unnecessary.

@bschussek IMO it's the form that's being validated, so if the form is invalid by way of any constraints failing in its children, no data should be set.

webmozart · 2012-10-09T09:02:02Z

@rcambridge This was described in strategy 2, and also its limitation was described, i.e. that then the model constraints would only be validated if all the form constraints were valid. Would you want that?

rcambrj · 2012-10-09T09:04:42Z

@bschussek Yes I think that's an understandable and acceptable condition. It merely requires that the contraints are duplicated in the form.

Perhaps in the future, the form could inherit the model's constraints to avoid this.

schmittjoh · 2012-10-09T09:12:18Z

I'd guess that many people will use one of the two approaches exclusively anyway, i.e. either form constraints, or model constraints. For those, who want to use both, it seems like an acceptable condition to me, too.

Inheriting constraints is problematic for the same reason that a rollback is not easily implementable. Namely, there could be any side-effects caused by setting the data on the model.

Anyway, strategy 2) looks good to 8000 me.

webmozart · 2012-10-09T09:32:43Z

@schmittjoh But strategy 2 does not apply one of the approaches exclusively. It applies them sequentially: First the constraints in the form, and (if all of them are valid) then the constraints on the model.

We can define strategy 3 and 4 as

as above
as above
Don't evaluate model constraints if at least one form constraint exists in the tree.
Don't evaluate model constraints if some option is set (e.g. "data_constraints" => false). If that option is not set, strategy 1 is applied (2 and 3 would not make sense).

3 is problematic because the addition of a custom field types that uses the "constraints" option then disables model constraints for the whole form.

rcambrj · 2012-10-09T09:48:09Z

Makes little sense to me as I would expect to validate a form in its entirety
Does have the drawback of having a two-step validation process
Seems like an odd behaviour to me, not something I'd expect.
I could entertain an option to disable the second stage of validation, but I don't understand why strategies 2 or 3 otherwise don't make sense.

May I suggest that the model validator be changed to solve this problem? Why is this being worked around with two steps in the first place? :)

stof · 2012-10-09T09:53:42Z

@rcambridge The model validator validates the model. so you can only run it after binding, otherwise you are not validating the submitted values. You cannot run model constraint on something else than the model: some properties may not be in the form at all, some setters might have side effects and class-level constraints expect receiving the object to validate it.

rcambrj · 2012-10-09T09:55:01Z

@stof But it doesn't have to work that way. IMO the core of the problem is exactly that behaviour.

stof · 2012-10-09T09:58:33Z

@rcambridge If you are applying constraint on the model, why should they validate something else ?

And btw, some stuff require validating it on the model. For instance, the DoctrineBridge unique constraint checking the content of the DB needs to have the object, to be able to know that the object found is the same that the object being validated. Otherwise, editing the object would force you to modify the unique field each time to be valid.

rcambrj · 2012-10-09T10:10:18Z

@stof I'm all for model validation, but against setting invalid data. Two-step validation is the next best thing, but best avoided.

If there is a need to have data set on a model before validating, then the validator is behaving incorrectly, because invalid data should never be set on a model, IMO.

As I've mentioned, I'm eager to understand why it is that this approach has been used, so please enlighten me. Otherwise, it would be great if the validator could be amended so that it checks constraints before it sets the data :)

stof · 2012-10-09T10:13:25Z

@rcambridge if you are validating the model without setting the data in the model, it means that validating the model used the old data. This is simply useless.
And some stuff simply cannot be validated before setting the data when defining the constraint on the models: setters can have side effects, for instance putting a value in a second field too. This means that a validation rule using this second field would be wrong if applied before the setter is called.

rcambrj · 2012-10-09T10:16:43Z

@stof Would you mind showing me an example of another framework which sets invalid data on models? If I could see that it's not just symfony that's doing this then I might understand your approach better.

webmozart · 2012-10-09T10:23:48Z

@rcambridge The problem in other frameworks is that you have to completely duplicate the validation constraints in the model and the view (form) layer. This was solved in Symfony by the given approach. While this approach might not be perfect in all cases, it is convenient (and rapid) in many. That means, rather than putting the approach completely in question, we should find a way to solve the edge cases where this is not desired (i.e. constraint duplication is explicitly wanted).

rcambrj · 2012-10-09T12:00:20Z

@bschussek sounds like two options for maximum flexibility then: update_strategy and validation_strategy

stof · 2012-10-09T13:44:16Z

@bschussek The current behavior is not U1. If the form is not synchronized, it sets null in the property (resulting in a fatal error if the setter does not accept null values because of the typehint). U1 would be a better behavior than currently.

webmozart · 2012-10-09T15:28:13Z

@stof Maybe that used to be like that, but it is not anymore. Check PropertyPathMapper line 71.

stof · 2012-10-09T15:29:44Z

@bschussek then is it possible to have null as value for a synchronized required date field ?

webmozart · 2012-10-09T15:39:26Z

@stof Currently the setter would be called when the field is null. Not doing so would be U2 (or U3).

webmozart · 2012-10-09T15:58:38Z

@rcambridge I'm not sure whether we actually need these options. What you and also @fabpot want is to never update a model unless the complete form is valid. As described before, this is not possible, so the only way to achieve that original goal is by creating a separate form data model (or simply, a recursive array) and do the mapping between that model and your domain model yourself.

So apart from that use case, which other use cases justify

changing the current default update and validation strategy?
making the strategies configurable?

asm89 · 2012-10-09T18:31:43Z

What is the usecase for not updating the model if constraints fail? Maybe the biggest confusion comes from the fact that when calling $form->isValid() the data was already bound to the model? Should preventing the model update really be a goal of the form/validation component?

When using doctrine2 for example, getting an untouched entity if the form binding fails could/would be something like:
if (!$form->isValid()) $entityManager->refresh($model);

tkleinhakisa · 2012-10-10T07:22:10Z

@asm89 using refresh here makes an extra query to get exactly the same data you already had before binding the entity to the form. It's a solution but i don't think a good one

rcambrj · 2012-10-11T09:48:35Z

@bschussek need vs want :D

Right now, I can validate a form's data without setting anything on a model by building the form without the model and then manually setting, as you suggested. So I don't strictly need any of this.

As it stands, though, forms in sf2 require more work from me than forms in sf1.4. I would consider a version increase to be an improvement, which in the case of a framework should signify a decrease in required work and/or an increase in flexibility.

Take from that what you will :)

webmozart · 2012-10-11T15:19:44Z

I'll suggest another approach, which is more in line with what @fabpot and @rcambridge want, but very sensible because it is breaking BC quite a bit. It might have other benefits though.

The idea generally is to decouple data mapping from the form component a bit more, a bit like the ZF2 guys are doing it. Currently, when a form is bound, the data is passed around the form, reverse transformed and object instances are created throughout the process. The idea is to only assemble the form data into recursive arrays during binding, and serialize the recursive array into the object structure after binding (only for valid fields (U2), or only if the form is completely valid (U3)) and validate that object structure too.

(1) Binding and reverse transformation results in:

array(
    'name' => 'Bernhard',
    'birthDate' => DateTime Object(...),
    array(
        'street' => 'Foo Street',
    )
)

(2) Validation according to form constraints.
(3) Serialization into an object structure:

Person Object(
    [name] => 'Bernhard',
    [birthDate] => DateTime Object(...)
    [address] => Address Object(
        [street] => 'Foo Street',
    )
)

(4) Validation according to class constraints.

Required changes:

Each form not only needs to store its data, but also (optionally) the object that the data comes from and will be written to. This is where we break BC, because users will not be able to access get*Data() or form.vars.data anymore to access object instances, but getObject() (or whatever it would be called) and form.vars.object instead.
The "property_path", "by_reference", "mapped", "by_reference" and "empty_data" options of all forms need to be assembled to a serialization mapping.
setObject() must launch the deserialization using the assembled mapping, forward the result to setData() and recursively call itself in the form tree.
bind() on the root form must call updateObject() if the form is valid. That method should serialize the form data into an object and recursively call itself in the form tree.

With such a design, it should probably be possible to extract the serialization mapping of the form and reuse it for REST APIs without involving the complete (and slower) form processes.

jmikola · 2012-10-11T15:55:50Z

Reading through the thread and noting comments comparing Symfony 1.4 to 2.x, it seems that some might be missing the connection of our current implementation with JSR 303. Validation was completely decoupled from forms and models were responsible for defining their own constraints, so it was never a question of validating the models themselves.

Still, I'd reckon that most of us, myself included, have been bitten by invalid data remaining set on our models. A memorable bug at OpenSky was the user profile form allowing the user's session object to become corrupted, since the objects were one and the same thanks to Doctrine's persistence layer (cloning was the solution there :).

@bschussek: If I read the above correctly, constraints at the form level will require validation before anything is set on the model, and then we'll still keep model constraints to be validated after the data is bound to an underlying object? If so, that seems like a good compromise (noting the BC breaks) to appease folks looking for Symfony 1.4 behavior and those comfortable with the newer design.

rcambrj · 2012-10-11T16:34:17Z

@bschussek I'm struggling to understand the actual effects that your new proposal would have, would you mind explaining?

@jmikola I had a quick read through JSR303 but I couldn't find any information regarding implementation. It seemed to me as though the specification defines the annotations for the validation of a model's data, but not the process used to validate such data, including whether the data should be set on the model before it's validated.

schmittjoh · 2012-10-11T16:37:25Z

The reference implementation for JSR303 is the Hibernate Validator,
http://www.hibernate.org/subprojects/validator.html

On Thu, Oct 11, 2012 at 6:34 PM, Robert Cambridge
notifications@github.comwrote:

@bschussek https://github.com/bschussek I'm struggling to understand
the actual effects that your new proposal would have, would you mind
explaining?

@jmikola https://github.com/jmikola I had a quick read through JSR303
but I couldn't find any information regarding implementation. It seemed to
me as though the specification defines the annotations for the validation
of a model's data, but not the process used to validate such data,
including whether the data should be set on the model before it's validated.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/5480#issuecomment-9347517.

grEvenX · 2012-10-11T17:50:40Z

@rcambridge You asked for other frameworks; Propel ORM framework let's you set invalid data in a model, then call it's "validate" function:
http://www.propelorm.org/documentation/05-validators.html

boekkooi · 2012-10-12T19:06:24Z

Is it just me or is the idea of binding a (none detached) entity to a form just a bad idea?
Maybe people who are having this problem should take a look at using CQ(R)S?

webmozart · 2012-10-14T16:36:44Z

@rcambridge The effects, from what I can tell right now, would be that event listeners accessing the object of the form (via $event->getData() or $form->getData()) would break, because that method would not return the object anymore, but an array. Also, all controllers (and other classes) extracting the form's object via $form->getData() would break.

@jmikola Exactly. Ideally, the behavior of forms without form constraints should not change at all (apart from the noted BC breaks). Once a form has form constraints, we need to decide on one of the update strategies U2 and U3.

I don't really like my method name proposal setObject($object) and getObject() yet, because in reality the object can be any data source that can be unserialized into the form and back (JSON, XML, a DomDocument instance etc., depending on the serializer of the form).

stof · 2012-10-14T18:24:17Z

@bschussek or even an array, which would be weird for a method getObject :)

webmozart · 2012-10-14T20:06:30Z

@stof It could be, although it'd be rather unlikely, because you probably wouldn't serialize an array into another array.

stof · 2012-10-14T21:40:47Z

@bschussek if you bind your form to an array, what would you return from your getObject method otherwise ? the array would be the model data, just like your object is when binding with an object.

webmozart · 2012-10-15T07:01:52Z

More alternatives that come to mind:

binding some object/data source to a form:
- setInput()
- setDataSource()
- setDefault(s)()
retrieving the updated object/data source from the form:
- getOutput()
- getDataSource()
- getResult()

webmozart · 2014-10-16T10:46:52Z

I reread this thread again now and think that we are over-complicating things quite a bit. In fact, there are two very simple solutions to the problem:

If the Form is bounds to an entity, Doctrine (or whatever DB mapper) should validate entities before persisting them in a listener. Then it becomes impossible to persist invalid entities. The required "validation level" could be group "Default" by default, unless a @DefaultGroup annotation is set as proposed here.
If that is too risky, create a separate data model independent of the DB mapper (think CQRS). I often end up doing this since it makes the application code much saner.

This warrants new tickets. This ticket, though, can be closed IMO.

Isinlor · 2015-12-10T08:08:39Z

So, is there currently any solution for:

class Foo
{
    public function setDate(\DateTime $date)
    {
        $this->date = $date;
    }
}

// form component
(new Foo)->setDate('whatever');

// form component validate

besides doing the mapping on my own?

webda2l mentioned this issue Oct 22, 2012

[Form] Support dependent fields #5807

Closed

webmozart mentioned this issue Nov 8, 2012

[3.0] [Form] Setting "empty_data" to "" still results in NULL #5906

Closed

mvrhov mentioned this issue Jan 4, 2013

"Logged in as ..." displays invalid username during Profile Editing FriendsOfSymfony/FOSUserBundle#926

Open

webmozart mentioned this issue Jan 8, 2013

[Form] Rename "empty_data" to "data_factory" #5939

Closed

webmozart closed this as completed Oct 16, 2014

This was referenced Oct 16, 2014

[DX][Validation] Handling of Validation Groups #11880

Closed

[3.0] [Form] should rebind if data was changed in event and validation fails #6399

Closed

kherrera-ebsco mentioned this issue Mar 21, 2017

[DX][Validation] Form mapping invalid values to entity using type hints. #22096

Closed

Uh oh!

[Form] Prevent model update if form validation fails #5480

[Form] Prevent model update if form validation fails #5480

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!