Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Form][2.3] Fixed Form::submit() and Form::setData() to react to dynamic form modifications#8828
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This PR was merged into the master branch.Discussion----------[Validator] added missing Estonian translations| Q | A| ------------- | ---| Fixed tickets | -| License | MITCommits-------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. Seehttp://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.
…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.
…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 classesPlease see commits.| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MITCommits-------940b788 [Console] use inheritdoc for Output classesbd61b92 [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::writeee0cc40 [Console] fix NullOutputa290f87 [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 ConsoleAdded 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 (closessymfony#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 (closessymfony#8045).Discussion----------[Form] Add missing type hintCommits-------4dccee6 [Form] Add missing type hint
* 2.2: removed CHANGELOG for 2.0 as it is not maintained anymore
This PR was submitted for the master branch but it was merged into the 2.3 branch instead (closessymfony#8060).Discussion----------[Validator] added missing Farsi translations| Q | A| ------------- | ---| Fixed tickets | -| License | MITFarsi 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 (closessymfony#8052).Discussion----------remove check for PHP bugsymfony#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 bugsymfony#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 eventConflicts: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 (closessymfony#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 (closessymfony#8808) fix for Process:isSuccessful()Conflicts:UPGRADE-3.0.mdsrc/Symfony/Component/Locale/Resources/data/build-data.php
Conflicts:src/Symfony/Component/Form/Form.phpsrc/Symfony/Component/Form/Tests/AbstractFormTest.phpsrc/Symfony/Component/Form/Tests/CompoundFormTest.phpsrc/Symfony/Component/Form/Util/VirtualFormAwareIterator.php
Conflicts:src/Symfony/Component/Form/Extension/Core/DataMapper/PropertyPathMapper.php
…s called for all elements in theform tree during the initialization of the tree
…ifications of a form's children
Contributor
Burgov commentedAug 22, 2013
this looks very nice! I'm gonna try it out in one of our projects and see what happens |
ContributorAuthor
webmozart commentedAug 22, 2013
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); |
ContributorAuthor
webmozart commentedAug 22, 2013
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. |
Contributor
Burgov commentedAug 22, 2013
@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) {/*...*/ } |
Contributor
Burgov commentedAug 22, 2013
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',newCompanyFinancialAutocompleteType(),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' ));});[...] |
ContributorAuthor
webmozart commentedAug 22, 2013
Yes you're right, you need to use |
ContributorAuthor
webmozart commentedAug 22, 2013
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 $builder->addEventListener(FormEvents::POST_SET_DATA,function (FormEvent$e) {if (/* condition */) {$form =$event->getForm();$form->add('billingCompany', ...);$form->get('billingCompany')->addEventListener(...);// BANG, not supported }}); |
Contributor
Burgov commentedAug 22, 2013
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. |
ContributorAuthor
webmozart commentedAug 22, 2013
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
ContributorAuthor
webmozart commentedAug 23, 2013
Member
fabpot commentedAug 23, 2013
@bschussek sorry, I thought you were waiting for some sort of feedback first. Merging now. |
fabpot added a commit that referenced this pull requestAug 23, 2013
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,#8748cc@BurgovThis 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.3cd27e1f [Form] Extracted ReferencingArrayIterator out of VirtualFormAwareIterator3cb8a80 [Form] Added a test that ensures that setData() reacts to dynamic modifications of a form's children07d14e5 [Form] Removed exception in Button::setData(): setData() is now always called for all elements in the form tree during the initialization of the treeb9a3770 [Form] Removed call to deprecated method878e27c Merge branch 'form-submit-2.2' into form-submit-2.3ccaaedf [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() call5d60a4f Merge branch 'form-submit-2.2' into form-submit-2.300bc270 [Form] Fixed: submit() reacts to dynamic modifications of the form children
This was referencedAug 24, 2013
Contributor
peterrehm commentedNov 11, 2013
@Burgov: How have you fixed it with the chained dependent fields? |
egeloen commentedNov 11, 2013
@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. |
ContributorAuthor
webmozart commentedNov 11, 2013
I forgot last time that there in factis 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 commentedNov 11, 2013
@bschussek Just curious but can you explain why I have to pass |
ContributorAuthor
webmozart commentedNov 12, 2013
@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. |
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Labels
None yet
50 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:
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():For 2.2, this fix is not possible, because it would require changing the signature
DataMapperInterface::mapDataToForms($data, array $forms)toDataMapperInterface::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 onevery element of a form (except those inheriting their parent data) whensetData()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) untilget*Data()is called on them. IfgetData()isnot called beforecreateView(), any effects of*_DATAevent listeners attached to those fields will not be visible in the view.