[Form][2.3] Fixed Form::submit() and Form::setData() to react to dynamic form modifications #8828

webmozart · 2013-08-22T12:43:26Z

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

ref #3767, #3768, #4548, #8748
cc @Burgov

This PR ensures that fields that are added during the submission process of a form are submitted as well. For example:

$builder = $this->createFormBuilder()
    ->add('country', 'choice', ...)
    ->getForm();

$builder->get('country')->addEventListener(FormEvents::POST_SUBMIT, function (FormEvent $event) {
    $form = $event->getForm()->getParent();
    $country = $event->getForm()->getData();

    $form->add('province', 'choice', /* ... something with $country ... */);
});

Currently, the field "province" will not be submitted, because the submission loop uses foreach, which ignores changes in the underyling array.

Contrary to #8827 (for 2.2), this PR also allows dynamic modifications during setData():

$builder = $this->createFormBuilder()
    ->add('country', 'choice', ...)
    ->getForm();

$builder->get('country')->addEventListener(FormEvents::POST_SET_DATA, function (FormEvent $event) {
    $form = $event->getForm()->getParent();
    $country = $event->getData();

    $form->add('province', 'choice', /* ... something with $country ... */);
});

For 2.2, this fix is not possible, because it would require changing the signature DataMapperInterface::mapDataToForms($data, array $forms) to DataMapperInterface::mapDataToForms($data, array &$forms), which would be a BC break. For 2.3, this change is not necessary (instead of an array we now pass an iterator, which is passed by reference anyway).

Additionally, this PR ensures that setData() is called on every element of a form (except those inheriting their parent data) when setData() is called on the root element (i.e., during initialization). Currently, when some of the intermediate nodes (e.g. embedded forms) are submitted with an empty value, setData() won't be called on the nodes below (i.e. the fields of the embedded form) until get*Data() is called on them. If getData() is not called before createView(), any effects of *_DATA event listeners attached to those fields will not be visible in the view.

This PR was merged into the master branch. Discussion ---------- [Validator] added missing Estonian translations | Q | A | ------------- | --- | Fixed tickets | - | License | MIT Commits ------- fd2b260 [Validator] added missing Estonian translations

This PR was merged into the master branch. Discussion ---------- [DomCrawler] Fixed the Crawler::html() method for early PHP versions | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes, but not on travis (segfault) | Fixed tickets | symfony#7963 | License | MIT | Doc PR | - Node argument was added to the [`DOMDocument::saveHTML()`](http://php.net/manual/en/domdocument.savehtml.php) in PHP 5.3.6. See http://php.net/manual/en/domdocument.savehtml.php. It's not a nice looking solution, but seems to be the only option. Condition should be removed once PHP dependency goes over 5.3.6. Commits ------- a4e3ebf [DomCrawler] Fixed the Crawler::html() method for PHP versions earlier than 5.3.6.

…spatcher

This PR was merged into the master branch. Discussion ---------- changed all version deps to accept all upcoming Symfony versions Everything is in the title. Commits ------- b1c9fd2 removed versions in composer.json files f41ac06 changed all version deps to accepts all upcoming Symfony versions

…d a failing test for verbosity

It should not extend from abstract Output class because than it inherits the useless constructor arguments and applies formatting in write() needlessly.

…erface::write

…sing of decorated variable. Also we don't need to typecast to boolean as its already done by the formatter and its his responsibility

…ities) also add missing information

This PR was merged into the master branch. Discussion ---------- [Console] fix Output classes Please see commits. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT Commits ------- 940b788 [Console] use inheritdoc for Output classes bd61b92 [Console] fix phpdoc of Output classes (esp. regarding the new verbosities) 9dcc9fa [Console] fix abstract Output class that fasly claims to support guessing of decorated variable. 2628d88 [Console] the default type value should use the constant in OutputInterface::write ee0cc40 [Console] fix NullOutput a290f87 [Console] fix test for NullOutput that does not print anything and add a failing test for verbosity

This PR was merged into the master branch. Discussion ---------- [Form][Twig] Removed an extra table column in the "button_row" block template | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Commits ------- 6d5bc7a [Twig][Form] Removed extra table colunm in the button_row block template

This PR was merged into the master branch. Discussion ---------- Fixed the tests on PHP 5.3.3 | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#7754 | License | MIT | Doc PR | - PHPUnit with PHP 5.3.3 couldn't handle garbage collecting properly. Commits ------- 0617ed1 [Console] Removed the descriptor from data set providers.

This PR was merged into the master branch. Discussion ---------- Improvement composer.json Console Added suggest to composer.json. Commits ------- 5e6245f [ADD] Component_Console -add suggest in the composer.json to event-dispatcher

This PR was squashed before being merged into the master branch (closes symfony#8025). Discussion ---------- [BrowserKit] should not follow redirects if status code is not 30x | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | yes | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Currently, BrowserKit operates incorrectly. It follows "redirect" when `Location` header is present, but having just the header is not enough to perform redirection. [RFC-2616](http://tools.ietf.org/html/rfc2616#section-14.30) precisely says that the redirection should be performed only with `30x` status codes. This PR fixes the incorrect behaviour of BrowserKit and make it consist with both the RFC document and with other clients, used for example with Behat. I've found the issue while testing my application with Behat. I was returning `Location` header with `HTTP 201/Created` status code and was surprised that BrowserKit follows the redirection. This PR is for 2.3 version (master) of Symfony. Commits ------- 8f54da7 [BrowserKit] should not follow redirects if status code is not 30x

This PR was squashed before being merged into the master branch (closes symfony#8045). Discussion ---------- [Form] Add missing type hint Commits ------- 4dccee6 [Form] Add missing type hint

* 2.2: removed CHANGELOG for 2.0 as it is not maintained anymore

This reverts commit a6dd5db, reversing changes made to da6f190.

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes symfony#8060). Discussion ---------- [Validator] added missing Farsi translations | Q | A | ------------- | --- | Fixed tickets | - | License | MIT Farsi translations for the following validators - IBAN - ISBN - ISSN - Currency - EqualTo - IdenticalTo - NotEqualTo - NotIdenticalTo - GreaterThan - GreaterThanOrEqual - LessThan - LessThanOrEqual; Commits ------- 026f2ac [Validator] adding missing Farsi translations

This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closes symfony#8052). Discussion ---------- remove check for PHP bug symfony#50731 | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#8007 | License | MIT | Doc PR | Commits ------- 28534a6 remove check for PHP bug symfony#50731

[Validator] russian language update

* 2.2: [Process] Use a consistent way to reset data of the process latest run CS fix [HttpFoundation] Fixed removing a nonexisting namespaced attribute. [Validation] Fixed IdentityTranslator to pass correct Locale to MessageSelector SwiftMailerHandler in Monolog bridge now able to react to kernel.terminate event Conflicts: src/Symfony/Component/Process/Process.php

This PR was merged into the 2.3 branch. Discussion ---------- Include untrusted host in the exception message `Invalid *` error message without the actual value that triggered them are really unhelpful to debug issues. Commits ------- fd2f633 Include untrusted host in the exception message

* 2.2: [Locale] fixed build-data exit code in case of an error fixed request format of sub-requests when explicitely set by the developer (closes symfony#8787) Sets _format attribute only if it wasn't set previously by the user. Exclude little words of 'ee' to 'oo' plural transformation fixed the format of the request used to render an exception Fix typo in the check_path validator added a missing use statement (closes symfony#8808) fix for Process:isSuccessful() Conflicts: UPGRADE-3.0.md src/Symfony/Component/Locale/Resources/data/build-data.php

Conflicts: src/Symfony/Component/Form/Form.php src/Symfony/Component/Form/Tests/AbstractFormTest.php src/Symfony/Component/Form/Tests/CompoundFormTest.php src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php

Conflicts: src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php

…s called for all elements in the form tree during the initialization of the tree

…ifications of a form's children

Burgov · 2013-08-22T13:01:57Z

this looks very nice! I'm gonna try it out in one of our projects and see what happens

webmozart · 2013-08-22T13:18:49Z

The good thing is that you can rely on the model data (in this case of the "country" field) in both POST_SET_DATA and POST_SUBMIT. So in practice, you can use the same closure for both events:

$updateCountry = function (FormEvent $event) {
    $form = $event->getForm()->getParent();
    $country = $event->getForm()->getData();

    $form->add('province', 'choice', /* ... something with $country ... */);
};

$builder->get('country')->addEventListener(FormEvents::POST_SET_DATA, $updateCountry);
$builder->get('country')->addEventListener(FormEvents::POST_SUBMIT, $updateCountry);

webmozart · 2013-08-22T13:21:45Z

There is still an issue with the default value of choice fields (#8747) which requires you to add some boilerplate code over what I just wrote, but that'll hopefully be fixed soon.

Burgov · 2013-08-22T13:22:03Z

@bschussek yeah that is a big advantage. It might even be nice to be able to add two listeners add once. Something like

$builder->get('country')->addEventListener(array(FormEvents::POST_SET_DATA, FormEvents::POST_SUBMIT), function(FormEvent $e) { /*...*/ }

Burgov · 2013-08-22T13:25:50Z

In the example in your PR message, in your listener you use $event->getData(). I found this not to work: it's an empty array when the data is empty and it's the raw viewdata (e.g. an ID) when the data is submitted.

Using $event->getForm()->getData() does give the correct results. Is this by design?

I've successfully tested the following example (which happens to be only post submit, as it only applies to new entities):

[...]
$builder->add('billingCompany', new CompanyFinancialAutocompleteType(), array('label' => 'nitro.project.company', 'ajax_reload' => true));
$builder->addEventListener(FormEvents::PRE_SET_DATA, function(FormEvent $e) {
    if (null !== $e->getData()->getBillingCompany()) {
        $e->getForm()->remove('billingCompany');
    }
});
$builder->get('billingCompany')->addEventListener(FormEvents::POST_SUBMIT, function(FormEvent $e) {
    $billingCompany = $e->getForm()->getData();
    if (null === $billingCompany) {
        return;
    }
    $company = $billingCompany->getCompany();

    $form = $e->getForm()->getParent();
    $form->add('contactPerson', 'entity', array(
        'query_builder' => function($er) use ($company) {
            return $er->createQueryBuilder('p')
                ->leftJoin('p.companies', 'companyContacts')
                ->where('companyContacts.company = ?1')
                ->setParameter(1, $company)
                ;
        },
        'class' => 'SamsonNitroAddressBookBundle:Person',
        'empty_value' => 'Choose contact',
        'label' => 'nitro.project.contact_person'
    ));
    $form->add('contactAddress', 'entity', array(
        'choices' => $company->getAddresses()->toArray(),
        'class' => 'SamsonAddressBookBundle:Address',
        'empty_value' => 'Choose address',
        'label' => 'nitro.project.contact_address'
    ));
});
[...]

webmozart · 2013-08-22T13:43:53Z

Yes you're right, you need to use $event->getForm()->getData() in POST_SUBMIT listeners, because there the event data is given in view format. I corrected the examples above.

webmozart · 2013-08-22T13:49:01Z

One further limitation, as can be seen in your example, is that chained dependencies don't work, because you can't attach event listeners to Form instances, only to FormBuilders. Ideally, you'd add the "billingCompany" field from within the listener, only if the criteria for adding it matches - but then you can't add further event listeners to it. I hope to fix this issue in the future.

$builder->addEventListener(FormEvents::POST_SET_DATA, function (FormEvent $e) {
    if (/* condition */) {
        $form = $event->getForm();
        $form->add('billingCompany', ...);
        $form->get('billingCompany')->addEventListener(...); // BANG, not supported
    }
});

Burgov · 2013-08-22T14:04:47Z

I ran into that problem exactly, however I do think that it's beyond the scope of this PR. If I'm not mistaken, you should be able to achieve it using the FormFactory from the builder object. Simplifying that later on would be a nice idea though.

webmozart · 2013-08-22T14:11:45Z

Yes, you can use the factory, and you're also quite right that this is beyond the scope of this PR. I only want to give you an idea of what is coming :)

Conflicts: src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php

webmozart · 2013-08-23T13:52:08Z

@fabpot Any chance for merging #8827 and this PR before Sunday night?

fabpot · 2013-08-23T15:09:22Z

@bschussek sorry, I thought you were waiting for some sort of feedback first. Merging now.

@Burgov

This PR was merged into the 2.3 branch. Discussion ---------- [Form][2.3] Fixed Form::submit() and Form::setData() to react to dynamic form modifications | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - ref #3767, #3768, #4548, #8748 cc @Burgov This PR ensures that fields that are added during the submission process of a form are submitted as well. For example: ```php $builder = $this->createFormBuilder() ->add('country', 'choice', ...) ->getForm(); $builder->get('country')->addEventListener(FormEvents::POST_SUBMIT, function (FormEvent $event) { $form = $event->getForm()->getParent(); $country = $event->getForm()->getData(); $form->add('province', 'choice', /* ... something with $country ... */); }); ``` Currently, the field "province" will not be submitted, because the submission loop uses `foreach`, which ignores changes in the underyling array. Contrary to #8827 (for 2.2), this PR also allows dynamic modifications during `setData()`: ```php $builder = $this->createFormBuilder() ->add('country', 'choice', ...) ->getForm(); $builder->get('country')->addEventListener(FormEvents::POST_SET_DATA, function (FormEvent $event) { $form = $event->getForm()->getParent(); $country = $event->getData(); $form->add('province', 'choice', /* ... something with $country ... */); }); ``` For 2.2, this fix is not possible, because it would require changing the signature `DataMapperInterface::mapDataToForms($data, array $forms)` to `DataMapperInterface::mapDataToForms($data, array &$forms)`, which would be a BC break. For 2.3, this change is not necessary (instead of an array we now pass an iterator, which is passed by reference anyway). Additionally, this PR ensures that `setData()` is called on *every* element of a form (except those inheriting their parent data) when `setData()` is called on the root element (i.e., during initialization). Currently, when some of the intermediate nodes (e.g. embedded forms) are submitted with an empty value, `setData()` won't be called on the nodes below (i.e. the fields of the embedded form) until `get*Data()` is called on them. If `getData()` is *not* called before `createView()`, any effects of `*_DATA` event listeners attached to those fields will not be visible in the view. Commits ------- 7a34d96 Merge branch 'form-submit-2.2' into form-submit-2.3 cd27e1f [Form] Extracted ReferencingArrayIterator out of VirtualFormAwareIterator 3cb8a80 [Form] Added a test that ensures that setData() reacts to dynamic modifications of a form's children 07d14e5 [Form] Removed exception in Button::setData(): setData() is now always called for all elements in the form tree during the initialization of the tree b9a3770 [Form] Removed call to deprecated method 878e27c Merge branch 'form-submit-2.2' into form-submit-2.3 ccaaedf [Form] PropertyPathMapper::mapDataToForms() *always* calls setData() on every child to ensure that all *_DATA events were fired when the initialization phase is over (except for virtual forms) 19b483f [Form] Removed superfluous reset() call 5d60a4f Merge branch 'form-submit-2.2' into form-submit-2.3 00bc270 [Form] Fixed: submit() reacts to dynamic modifications of the form children

peterrehm · 2013-11-11T13:09:50Z

@Burgov: How have you fixed it with the chained dependent fields?

egeloen · 2013-11-11T13:46:37Z

@peterrehm Not sure if I understand what you're looking for but you can inject the form factory (via the builder) in your form listener & then rely on it in order to build dynamic field according to other one in the form listener. It is not perfect but allow you to attach listener to your new field which is not possible directly through the form event as explain by Bernard because we have a Form instance and not a FormBuilder one.

webmozart · 2013-11-11T14:03:14Z

I forgot last time that there in fact is a possibility to add event listeners, namely by using the form factory:

$factory = $builder->getFormFactory();

$builder->addEventListener(FormEvents::POST_SET_DATA, function (FormEvent $event) use ($factory) {
    if (/* condition */) {
        $form = $event->getForm();
        $form->add(
            $factory->createNamedBuilder('billingCompany', ...)
                ->addEventListener(...)
                ->getForm()
        );
    }
});

egeloen · 2013-11-11T14:29:41Z

@bschussek Just curious but can you explain why I have to pass auto_initialize to false when I use this use case?

webmozart · 2013-11-12T07:56:21Z

@egeloen That's because only root forms must be auto initialized. Usually, that option is automatically set for children that are added to a form, but in this case, the framework obviously doesn't know that the created form is going to be added to another form.

fabpot and others added 30 commits May 13, 2013 08:59

changed all version deps to accepts all upcoming Symfony versions

f41ac06

removed versions in composer.json files

b1c9fd2

[ADD] Component_Console -add suggest in the composer.json to event-di…

5e6245f

…spatcher

[Console] Removed the descriptor from data set providers.

0617ed1

[Console] fix test for NullOutput that does not print anything and ad…

a290f87

…d a failing test for verbosity

[Console] fix NullOutput

ee0cc40

It should not extend from abstract Output class because than it inherits the useless constructor arguments and applies formatting in write() needlessly.

[Console] the default type value should use the constant in OutputInt…

2628d88

…erface::write

[Console] fix abstract Output class that fasly claims to support gues…

9dcc9fa

…sing of decorated variable. Also we don't need to typecast to boolean as its already done by the formatter and its his responsibility

[Console] fix phpdoc of Output classes (esp. regarding the new verbos…

bd61b92

…ities) also add missing information

[Console] use inheritdoc for Output classes

940b788

[Twig][Form] Removed extra table colunm in the button_row block template

6d5bc7a

merged branch ramonornela/master (PR symfony#8029)

4edbf3c

This PR was merged into the master branch. Discussion ---------- Improvement composer.json Console Added suggest to composer.json. Commits ------- 5e6245f [ADD] Component_Console -add suggest in the composer.json to event-dispatcher

tweaked previous merge (refs symfony#8029)

1f8a1b7

[BrowserKit] should not follow redirects if status code is not 30x

8f54da7

[Form] Add missing type hint

4dccee6

merged branch armetiz/master (PR symfony#8045)

a6dd5db

This PR was squashed before being merged into the master branch (closes symfony#8045). Discussion ---------- [Form] Add missing type hint Commits ------- 4dccee6 [Form] Add missing type hint

Merge branch '2.2'

ffccc1a

* 2.2: removed CHANGELOG for 2.0 as it is not maintained anymore

Revert "merged branch armetiz/master (PR symfony#8045)"

1231d47

This reverts commit a6dd5db, reversing changes made to da6f190.

adding missing Farsi translations

204e23e

remove check for PHP bug symfony#50731

95f356b

Update validators.ru.xlf

742cdb3

[Validator] russian language update

fabpot and others added 9 commits August 21, 2013 09:58

Include untrusted host in the exception message

fd2f633

Merge branch 'form-submit-2.2' into form-submit-2.3

5d60a4f

Conflicts: src/Symfony/Component/Form/Form.php src/Symfony/Component/Form/Tests/AbstractFormTest.php src/Symfony/Component/Form/Tests/CompoundFormTest.php src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php

Merge branch 'form-submit-2.2' into form-submit-2.3

878e27c

Conflicts: src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php

[Form] Removed call to deprecated method

b9a3770

[Form] Removed exception in Button::setData(): setData() is now alway…

07d14e5

…s called for all elements in the form tree during the initialization of the tree

[Form] Added a test that ensures that setData() reacts to dynamic mod…

3cb8a80

…ifications of a form's children

Merge branch 'form-submit-2.2' into form-submit-2.3

7a34d96

Conflicts: src/Symfony/Component/Form/Util/VirtualFormAwareIterator.php

fabpot merged commit 7a34d96 into symfony:2.3 Aug 23, 2013

This was referenced Aug 24, 2013

Dynamic forms after pr8827 2.3 symfony/symfony-docs#2928

Closed

[RFC] Let's fix the Form Component #8888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form][2.3] Fixed Form::submit() and Form::setData() to react to dynamic form modifications #8828

[Form][2.3] Fixed Form::submit() and Form::setData() to react to dynamic form modifications #8828

webmozart commented Aug 22, 2013

Burgov commented Aug 22, 2013

webmozart commented Aug 22, 2013

webmozart commented Aug 22, 2013

Burgov commented Aug 22, 2013

Burgov commented Aug 22, 2013

webmozart commented Aug 22, 2013

webmozart commented Aug 22, 2013

Burgov commented Aug 22, 2013

webmozart commented Aug 22, 2013

webmozart commented Aug 23, 2013

fabpot commented Aug 23, 2013

peterrehm commented Nov 11, 2013

egeloen commented Nov 11, 2013

webmozart commented Nov 11, 2013

egeloen commented Nov 11, 2013

webmozart commented Nov 12, 2013

[Form][2.3] Fixed Form::submit() and Form::setData() to react to dynamic form modifications #8828

[Form][2.3] Fixed Form::submit() and Form::setData() to react to dynamic form modifications #8828

Conversation

webmozart commented Aug 22, 2013

Burgov commented Aug 22, 2013

webmozart commented Aug 22, 2013

webmozart commented Aug 22, 2013

Burgov commented Aug 22, 2013

Burgov commented Aug 22, 2013

webmozart commented Aug 22, 2013

webmozart commented Aug 22, 2013

Burgov commented Aug 22, 2013

webmozart commented Aug 22, 2013

webmozart commented Aug 23, 2013

fabpot commented Aug 23, 2013

peterrehm commented Nov 11, 2013

egeloen commented Nov 11, 2013

webmozart commented Nov 11, 2013

egeloen commented Nov 11, 2013

webmozart commented Nov 12, 2013