Skip to content

[Form] Validate missing fields when $clearMissing is false #25331

@Seb33300

Description

@Seb33300
Contributor

When using the $clearMissing option set to false (when submitting a form with $form->submit()), validation constraints on missing fields are ignored.
Since the fields are in the form, I think they should be validated too even if they are missing on submit.

I reported this issue several years ago but it seems to be misunderstand: #9998
The example provided on the original issue is still valid to reproduce the issue.

Activity

Simperfit

Simperfit commented on Dec 6, 2017

@Simperfit
Contributor

Could you please add a test case failling so we can look at it and find a fix ?

Seb33300

Seb33300 commented on Dec 8, 2017

@Seb33300
ContributorAuthor

I am trying to create a test but I am not able to make it working.

In Symfony\Component\Form\Tests\Extension\Validator\Constraints\FormValidatorTest

I would like to create a form with a single field and submit it a blank value (in order to check if the NotBlank constraint is properly working)

Then we should have to change the submit by $form->submit(array(), false); to reproduce the issue.

public function testValidateConstraintsIfClearMissingIsFalse()
{
    $object = $this->getMockBuilder('\stdClass')->getMock();

    $constraint = new NotBlank(array('groups' => 'group2'));

    $options = array(
        'constraints' => array($constraint),
    );

    $form = $this->getBuilder('name', '\stdClass')
        ->setCompound(true)
        ->setDataMapper($this->getDataMapper())
        ->setData($object)
        ->getForm();

    $missing = $this->getBuilder('missing', '\stdClass', $options)->getForm();

    $form->add($missing);

    // Launch transformer
    $form->submit(array('missing' => ''));

    $this->expectValidateAt(0, 'data', $object, array('Default'));

    $this->expectValidateValueAt(1, 'data', $object, $constraint, 'Default');

    $this->validator->validate($form, new Form());

    $this->assertNoViolation();
}
xabbuh

xabbuh commented on Dec 14, 2017

@xabbuh
Member

@Seb33300 You can just create a small example project that allows to reproduce your issue if you struggle to come up with a real test case. We can then look into it and see if there's actually any work to do.

xabbuh

xabbuh commented on Dec 22, 2017

@xabbuh
Member

@Seb33300 I am closing for now, but please leave a comment when you have a reproducer and we can reopen.

Seb33300

Seb33300 commented on Dec 22, 2017

@Seb33300
ContributorAuthor

The project I created on my original issue is still valid to reproduce the issue: #9998 (comment)

reopened this on Jan 23, 2018
simonberger

simonberger commented on Feb 2, 2018

@simonberger
Contributor

This is a really annoying issue that is most likely the reason of a lot of posts around the net if you search for this problem topic.
If you need more input please let me know. This bug is really clear to me and shouldn't be too hard to reproduce/locate.

1 remaining item

Simperfit

Simperfit commented on Feb 2, 2018

@Simperfit
Contributor

@HeahDude Could you please look at this one ? it seems to be a really annoying bug.

HeahDude

HeahDude commented on Feb 2, 2018

@HeahDude
Contributor

This was reported many times in the past, and is not a bug. Changing this behavior is a big BC break.
This could only be done as an opt-in feature.
When patching an object it should not be in an invalide state in the first place, so only changed values are validated.
if a field modifies another one, the workaround is to add a callback to validate them as expected (business rule, Symfony cannot handle this).

simonberger

simonberger commented on Feb 2, 2018

@simonberger
Contributor

I understand the technical difficulties and also to break a previous (bad) behavior. But putting this to side and focus on the expectations of a user on this functionality I can't share the position to call this not a bug.
If I have a form and I add NotNull/NotBlank validators the expectations to get an error in any missing case can't be wrong.

Also from a security point this is sensitive. A HTML-Form that per default sends an empty input field going through all tests. A simple "hack" by removing this empty field from the post would break the validation of this application with unknown consequences.

HeahDude

HeahDude commented on Feb 2, 2018

@HeahDude
Contributor

Hello @simonberger, sorry I don't follow you, especially on the security part.

Let's put things in context :).

  • What does $clearMissing mean?

    true => a missing field will be submitted as null
    false => a missing field will be ignored

  • What do we expect from Form::submit()?

    To bind incoming data to the initial data.

  • When can we use $clearMissing as true?

    All the time, this is the default behavior. we update the whole initial data with the submitted one.
    In such case, it makes sense to validate the whole data.

  • When as false?

    Symfony does it internally to handle PATCH requests, because for this verb, only submitted data should be bound. This verb "should" be used to "update" an existing resource, meaning the initial integrity of this resource is not the responsibility of the framework at this point.
    In such case, it makes senses to validate only submitted fields.

From here, I ask you guys. What concrete use case does not fit with that? Then let's try to fix it.

simonberger

simonberger commented on Feb 6, 2018

@simonberger
Contributor

Thank you for your answer @HeahDude
I'll try to describe my problem with the current behavior and what in my opinion leads to the confusion regarding the parameter $clearMissing and the possible resulting misuse through us users.

What the different impact of the parameter values is:

$clearMissing === true
Adds missing request parameters set as null. ("Clear missing" is misleading in this context too)

$clearMissing === false
Ignores validation of required parameters

In my project we wanted to archive the following:

  • Ignore optional parameter. Those gets set by patching elsewhere -> tried setting $clearMissing to false
  • Validate required parameters in this creation step -> needs `$clearMissing' been set to false

We can just have one of those above requirements.
So my point is $clearMissing does two jobs in one parameter, the real intended mechanism to support patching can't be read from inline documentation

Note:
This documentation site shows all information and warnings perfectly https://symfony.com/doc/current/form/direct_submit.html
But just having the online documentation while the parameter name has near zero information content often isn't enough.

I know we can't do much here without breaking functionality. But for example the parameter name can be changed which would add clarity. I favor long parameter names for such a complex impact ;)
The conflict could be solved with another parameter but I fear that would be an ugly workaround while the clearMissing parameter has to keep his functionality.

HeahDude

HeahDude commented on Feb 6, 2018

@HeahDude
Contributor

Thanks @simonberger, now that I understand the use cases, I can tell you, indeed, this $clearMissing parameter is not intended to solve them.
Doing such things like "Ignore optional parameter" or "Validate required parameters in this creation step" should involve validation groups.
The validation is decoupled from the forms. One can validate his/her data anytime using its dedicated service, the form is just a kind of "data handler" which delegates that part (too much IMO, but that's another issue and I would like to change that in master in the future), and integrates it "seamlessly".

pribeirojtm

pribeirojtm commented on Jun 22, 2018

@pribeirojtm

Hi,
As I understood from this $clearMissing param, if I use:

$form->submit($request->request->get($form->getName()); //default is true

it behaves the same as:
$form->handleRequest($request);

which means that missing entity fields that are not submitted in the request, their values are set to null, which is not what I want when I am using an Edit Form, because normally I have many optional fields and some and not even rendered in the form because they are no intended to be modified.
So it seems that they are useful for Creation Forms. Because if I am editing a entity using forms, I should use:
$form->submit($request->request->get($form->getName(), false);
to avoid problems.

I'm I right?

The validation part of the entity is other related thing, that can be achieved manually by calling the validator on the entity, or using $form->isValid(). $clearMissing set to false does not fire validation on entity fields that are no submitted, they have to be manually validated, for example, right?

Thanks.

HeahDude

HeahDude commented on Jun 22, 2018

@HeahDude
Contributor

As I understood from this $clearMissing param, if I use:

$form->submit($request->request->get($form->getName()); //default is true

it behaves the same as:
$form->handleRequest($request);

true

which means that missing entity fields that are not submitted in the request, their values are set to null, which is not what I want when I am using an Edit Form, because normally I have many optional fields and some and not even rendered in the form because they are no intended to be modified.

In such case (clear missing is true), when editing, optional fields can mean two things:

  • the field value can remain empty or the field name does not have to be part of submitted data, and the form will set the property to null
  • the field is removed from the tree, then it won't be rendered and the form will not try to map null on the underlying property
HeahDude

HeahDude commented on Jun 22, 2018

@HeahDude
Contributor

I may need to precise about:

As I understood from this $clearMissing param, if I use:

$form->submit($request->request->get($form->getName()); //default is true

it behaves the same as:
$form->handleRequest($request);

the request handler also prevents submitting if the post max size is reached, and merges $request->files if needed. All that only when methods configured on the form and read from the request match.

xabbuh

xabbuh commented on Sep 23, 2018

@xabbuh
Member

Re-reading the issue I am going to close here. As @HeahDude explained this is the expected behaviour and we are not going to change that. Though, if someone likes to open a pull request to improve the documentation, we will happily review and merge that.

added 2 commits that reference this issue on Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @Seb33300@xabbuh@Simperfit@simonberger@HeahDude

        Issue actions

          [Form] Validate missing fields when $clearMissing is false · Issue #25331 · symfony/symfony