Skip to content

[Form] form contains no children in PRE_SET_DATA and POST_SET_DATA events #4552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

MDrollette
Copy link
Contributor

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: N/A
Todo: N/A
License of the code: MIT

When the Form constructor was refactored here 2996340 it changed when setData events are fired. They fire before the children are added in the builder here:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/FormBuilder.php#L206.

This fix calls setData after the children are added so the setData events have access to the form children.

@travisbot
Copy link

This pull request passes (merged 3a583f3 into 0995b1f).

@MDrollette
Copy link
Contributor Author

This means setData() is called twice with data, whereas before, the setData call in the constructor contained null (

$this->setData(null);
). I'm not sure which would be the desired behavior.

@acasademont
Copy link
Contributor

I don't know if this is the way to go but definetely, PRE_SET_DATA events are now called BEFORE the children are added in the builder.

My subscriber made some children read_only (now disabled), or not depending on the data present. By default users are not able to change the name, but in some conditions, they can.

    public function buildForm(FormBuilderInterface $builder, array $options)
    {
        $subscriber = new ProductFieldsSubscriber($builder->getFormFactory());
        $builder->add('name', 'text', array('disabled' => true))
    }
    public function preSetData(DataEvent $event)
    {
        $data = $event->getData();
        $form = $event->getForm();

        if (null === $data) {
            return;
        }

        if (!$data->getId()) {
            $form->add($this->factory->createNamed('name', 'text', null, array('disabled' => false)));
        }
    }

Now, in SF 2.1, the presetData function is called before the builder adds its children so the code in the preSetData is pretty useless :(

@webmozart
Copy link
Contributor

@acasademont You should move the add() call from buildForm() to preSetData(). I.e., preSetData() should either add a disabled or an enabled field.

You can never assume whether add() is called before or after setData(). Your event listeners should be able with any of these cases.

@webmozart webmozart closed this Jul 6, 2012
@kostiklv
Copy link

kostiklv commented Aug 3, 2012

I'm in process of upgrading existing 2.0 project to 2.1 and here is the issue I'm facing. We have our own FormType which adds a bunch of fields and a subscriber. The subscriber listens to PRE_SET_DATA and in the event handler it was removing some of the fields depending on particular values in form data. (It is a checkout form with shipping & billing addresses, but for some payment types billing address is not required, so we were removing these fields completely).
So, this doesn't work anymore because the form contains no children during PRE_SET_DATA.

Extrapolating @bschussek answer above, I can see how to solve this by moving the whole logic of building the form into PRE_SET_DATA, i.e. do not add any optional fields in the buildForm of my type, but add all required fields in the PRE_SET_DATA listener, depending on the data. IMO this would be pretty ugly workaround.

So, what's the right way of handling this?

Also, I have looked into ResizeFormListener and it's also using $form->remove() in its preSetData() method. So, it seems to assume that add() is called before setData() contrary to what @bschussek says above.

Anyway, I think it's worth mentioning that PRE_SET_DATA (and former SET_DATA) event listeners are now getting a form w/o children in the UPGRADE-2.1 guide (unless it's going to change before release).

@ehibes
Copy link

ehibes commented Oct 9, 2012

@kostiklv So what way did you use to make what you wanted (I've got the same problem) ?

@kostiklv
Copy link

kostiklv commented Oct 9, 2012

IIRC we did the workaround I explained above (i.e. moved the building of the form to PRE_SET_DATA instead of buildForm).

@bschussek - would you mind looking into it?

@marcospassos
Copy link

I think it's a problem. Using the event to add the required fields, besides being unsightly and inappropriate, the field order is lost. To me it makes sense that the form needs to be built before its data are defined.

@kix
Copy link
Contributor

kix commented Feb 8, 2013

I think it's quite necessary to document the form event subscribers' inability to remove form fields somehow: I've wasted a pretty good amount of time trying to figure out why a $formEvent->getForm->all() call in an event subscriber would return an empty array, wouldn't delete any of the form children and wouldn't throw any exception. It was just me being lucky enough to stumble upon this thread.

@jeremylivingston
Copy link
Contributor

+1 for better documentation or an explanation of how this is supposed to work.

It seems that removing form elements in a PRE_SET_DATA listener is only sometimes possible. I have cases where using embedded forms will make removing fields possible, but others where I can't. The use cases should be thoroughly outlined in the documentation so it's clear when you can and can't use this practice.

@MDrollette
Copy link
Contributor Author

For anyone following/finding this issue, it's been resolved here #7878

@acasademont
Copy link
Contributor

great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants