[Form] made sure the child type default options are passed to the parent when... #3512

Burgov · Mar 6, 2012

...calling the parent's getParent() method

Bug fix: Yes
Feature addition: No
Backwards compatibility break: Reverts an older BC-break
Symfony2 tests pass: Yes
Fixes the following tickets: #3694, #3354, #3753

With the merge of 88ef52d, a bug was introduced which I keep running into and also generated some issues: #3694, #3354, #3753

A good example of the bug is when you define a new Form Type which defines 'choice' as its parent and have it return 'expanded'=true and 'multiple'=true from the getDefaultOptions() method.

The buildForm of ChoiceType receives these options as expected and therefore adds a list of checkboxes to itself. The getParent method, on the other hand, does not receive these options. In fact, it doesn't receive a value for 'expanded' at all, so it just "guesses" the value to be false, after which it returns 'field' rather than 'form' as its parent. Because of this, a datamapper is never set to the form element, causing all the checkboxes to remain empty, even if you pass a value to it.

Other cases can be found in the issues mentioned above.

Basically the only workaround is to explicitly pass these options to the builder from within every controller and builder this type is used in, which of course results in a lot of non-DRY code.

This PR makes sure the options return from getDefaultOptions are passed to the getParent() method of the parent type and that all getDefaultOptions() methods get the default options of their children merged with the passed options, rather than its parents default options merged with passed options.

…n calling the parent's getParent() method

Burgov · Mar 17, 2012

This should fix #3354. Can anybody comment on it?

fixe · Mar 17, 2012

@Burgov, I've tried with this example and it still doesn't work

Burgov · Mar 25, 2012

@fixe: I reproduced your problem in a test (see latest commit), now let's see if we can fix it!

Burgov · Mar 25, 2012

I was finally able to refactor the code so that old and new tests all work now.

I did not yet update the comments in the code, but will do so if the PR is OK.

Let me know if you run into trouble with this version of the component

stof · Mar 30, 2012

@bschussek ping

Burgov · Mar 31, 2012

For back reference, a link to the original PR:

#3290

maoueh · Apr 2, 2012

I faced this problem today. I had to make a new field type based on 'choice'. I wanted to put these default options in my custom type:

  return array(
        'choices' => array(...),
        'multiple' => true,
        'expanded' => true,
        'value_strategy' => ChoiceList::COPY_CHOICE,
        'index_strategy' => ChoiceList::COPY_CHOICE,
    );

But to make it work correctly, I had to pass these options to my custom type when using it:

  return array(
        'multiple' => true,
        'expanded' => true,
    );

I tested your fix and I don't need to pass the array above anymore to my custom types, options from the getDefaultOptions are indeed pass to the parent default options.

Bottom line, your fix works for my personal case.

Regards,
Matt

Commits ------- 8329087 [Form] Moved calculation of ChoiceType options to closures 5adec19 [Form] Fixed typos cb87ccb [Form] Failing test for empty_data option BC break b733045 [Form] Fixed option support in Form component Discussion ---------- [Form] Fixed option support in Form component Bug fix: yes Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: #3354, #3512, #3685, #3694 Todo: - ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3354) This PR also introduces a new helper `DefaultOptions` for solving option graphs. It accepts default options to be defined on various layers of your class hierarchy. These options can then be merged with the options passed by the user. This is called *resolving*. The important feature of this utility is that it lets you define *lazy options*. Lazy options are specified using closures that are evaluated when resolving and thus have access to the resolved values of other (potentially lazy) options. The class detects cyclic option dependencies and fails with an exception in this case. For more information, check the inline documentation of the `DefaultOptions` class and the UPGRADE file. @fabpot: Might this be worth a separate component? (in total the utility consists of five classes with two associated tests) --------------------------------------------------------------------------- by beberlei at 2012-04-05T08:54:10Z "The important feature of this utility is that it lets you define lazy options. Lazy options are specified using closures" What about options that are closures? are those differentiated? --------------------------------------------------------------------------- by bschussek at 2012-04-05T08:57:35Z @beberlei Yes. Closures for lazy options receive a Symfony\Component\Form\Options instance as first argument. All other closures are interpreted as normal values. --------------------------------------------------------------------------- by stof at 2012-04-05T11:09:49Z I'm wondering if these classes should go in the Config component. My issue with it is that it would add a required dependency to the Config component and that the Config component mixes many different things in it already (the loader part, the resource part, the definition part...) --------------------------------------------------------------------------- by sstok at 2012-04-06T13:36:36Z Sharing the Options class would be great, and its more then one class so why not give it its own Component folder? Filesystem is just one class, and that has its own folder. Great job on the class bschussek :clap: --------------------------------------------------------------------------- by bschussek at 2012-04-10T12:32:34Z @fabpot Any input? --------------------------------------------------------------------------- by bschussek at 2012-04-10T13:54:13Z @fabpot Apart from the decision about the final location of DefaultOptions et al., could you merge this soon? This would make my work a bit easier since this one is a blocker. --------------------------------------------------------------------------- by fabpot at 2012-04-10T18:08:18Z @bschussek: Can you rebase on master? I will merge afterwards. Thanks.

made sure the child type default options are passed to the parent whe…

50e6218

…n calling the parent's getParent() method

jmikola mentioned this pull request Apr 3, 2012

[Form] Default values set according to another options don't work anymore for children #3354

Closed

webmozart mentioned this pull request Apr 5, 2012

[Form] Fixed option support in Form component #3789

Merged

fabpot closed this Apr 11, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Form] made sure the child type default options are passed to the parent when... #3512

[Form] made sure the child type default options are passed to the parent when... #3512

Uh oh!

Burgov commented Mar 6, 2012

Uh oh!

Burgov commented Mar 17, 2012

Uh oh!

fixe commented Mar 17, 2012

Uh oh!

Burgov commented Mar 25, 2012

Uh oh!

Burgov commented Mar 25, 2012

Uh oh!

stof commented Mar 30, 2012

Uh oh!

Burgov commented Mar 31, 2012

Uh oh!

maoueh commented Apr 2, 2012

Uh oh!

Uh oh!

Search code, repositories, users, issues, pull requests...

Uh oh!

[Form] made sure the child type default options are passed to the parent when... #3512

[Form] made sure the child type default options are passed to the parent when... #3512

Uh oh!

Conversation

Burgov commented Mar 6, 2012

Uh oh!

Burgov commented Mar 17, 2012

Uh oh!

fixe commented Mar 17, 2012

Uh oh!

Burgov commented Mar 25, 2012

Uh oh!

Burgov commented Mar 25, 2012

Uh oh!

stof commented Mar 30, 2012

Uh oh!

Burgov commented Mar 31, 2012

Uh oh!

maoueh commented Apr 2, 2012

Uh oh!

Uh oh!