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

stof · 2012-02-14T16:37:43Z

Since the recent refactoring of the way default options are merged for a Form, setting a default value according to the value of another option does not work anymore for the hierarchy as the parent type will apply this default with its own default value even if the child type changed the default value. This is because the way the defaults are applied changed so the child type is applied only later.
A good way to see this is to define a type setting a default value for the data_class property. The empty_data default value set by the FieldType won't be set properly in the form (we would expect getting a closure crating an instance of the class).
This is a BC break and makes the type inheritance less powerful IMO.

/cc @bschussek

The text was updated successfully, but these errors were encountered:

merk · 2012-02-19T11:52:45Z

I have run into this with any custom collection types - they return arrays instead of the specified dataclass in each FormType. The workaround is to supply it as an option to the parent form type:

<?php
      $builder->add('parts', 'collection', array(
                'type' => new OrderPartType(),
                'allow_add' => true,
                'allow_delete' => true,
                'prototype' => true,
                'options' => array('data_class' => 'Ibms\\PartBundle\\Entity\\OrderPart'),
            ));

stof · 2012-02-19T12:07:24Z

~~This is true for any custom type setting a default value for the data_class.~~

manuelderuiter · 2012-02-26T14:23:49Z

Any update about this?

andreasschmidt · 2012-02-27T13:18:21Z

Spent about 6 hours looking for a bug in my code. Finally arrived here ;) Would be great if it will be fixed soon!

kriswallsmith · 2012-03-06T14:38:42Z

@stof can you be any more specific about the change that created this regression?

Burgov · 2012-03-06T14:42:49Z

#3512

stof · 2012-03-06T16:15:38Z

@kriswallsmith 88ef52d

craue · 2012-03-09T18:37:53Z

Could we get a fix for this soon? It's quite disturbing being unable to follow the master branch for almost a month now because of this issue.

jmikola · 2012-03-16T14:24:19Z

I just noticed that 88ef52d was implemented without altering any tests. @craue: can you come up with a failing test case for how this should work and submit that in a PR? That might give @bschussek (or anyone else) something to work with to attempt a fix.

craue · 2012-03-16T20:53:01Z

@jmikola: Unfortunately, I'm not familiar with writing such tests yet.

jmikola · 2012-03-22T16:31:34Z

Is simply reverting 88ef52d an option? Obviously not in symfony master, but in our own forks?

jmikola · 2012-03-22T23:55:54Z

I came up with a temporary work-around the data_class processing if anyone is interested: https://gist.github.com/2165584

This is hardly a proper fix, but this is working for me, as the most significant issues in my application the empty_data Closure no longer being created by the FieldType parent.

craue · 2012-03-23T11:08:13Z

Actually, I'd like to see it reverted in the master branch as it introduces a bug/regression which seems to be unfixable. I asked @bschussek about that some time ago but got no response yet.

stof · 2012-03-23T11:12:24Z

@craue we need to be sure that reverting it does not break the other changes in the Form component. So it should not be reverted in symfony master directly

stof · 2012-03-23T11:39:50Z

btw, I tried reverting this commit and it makes the testsuite fail so reverting only this commit is not the solution

craue · 2012-03-23T11:50:24Z

Just keeping it as-is is not satisfying either. ;)

stof · 2012-03-23T11:52:34Z

@craue true, but reverting the whole Form refactoring is not good either as it was fixing half of the pending Form issues.

umpirsky · 2012-04-01T11:11:49Z

We are unable to follow the master branch because of this bug :(

webmozart · 2012-04-03T16:19:14Z

I'm sorry for introducing this regression. Unfortunately we cannot simply revert the commit, since then we are facing a different bug. I will commit myself to finding a solution this and next week, even though this might have to be a BC breaking one.

jmikola · 2012-04-03T17:02:28Z

@bschussek Thanks for popping in. I haven't look at this at all, but it might save some time to examine #3512 before you attempt a fix. That PR has worked for a number of folks afflicted by this bug.

Burgov · 2012-04-03T18:04:43Z

@bschussek Good to see you're back. As @jmikola pointed out, I've created a PR (#3512) which fixes the issue without breaking any tests. Can you tell me which bug exactly you mean? The specific commit (88ef52d) that introduced the BC-break doesn't include any tests

webmozart · 2012-04-04T09:54:57Z

For reference, this issue was introduced in #3290, AFAIR also in order to fix #3299. Unfortunately the problem is a lot more complex than the solution in @Burgov's commit.

The problem is that we want to be able to declare the default value of options dynamically, depending on the value of some other option. To give an example:

Assume the option "empty_data" is set in type "FooType" depending on the value of option "data_class".

If we resolve options bottom-up (before 88ef52d), "FooType" does not see the "data_class" option set by its parent type.
If we resolve options top-down (like now), its parent does not see the "empty_data" option set by "FooType".

So both approaches are flawed. Declaring options as arrays is flawed and it has been so since symfony 1. We need some mechanism that

Resolves a dependency ("data_class") completely, taking all types in the hierarchy as well as the passed options into account, before
Resolving the dependent ("empty_data") completely, as above, before
Resolving dependents of the dependent etc.

This sounds complex, and it is. PHP however offers a simple, native solution that mostly achieves what we want:

class ParentType {
    function getDataClass() { }
    function getEmptyData() { }
}

class FooType extends ParentType {
    function getDataClass() {
         ...
         parent::getDataClass()
         ...
    }
    function getEmptyData() {
        if (getDataClass()) {
            ...
        }
    }
}

A different approach could be to replace the options array by an Options object that allows to set closures as options that are only resolved upon access. Then we could do something like:

function getDefaultOptions() { // note the lack of passed $options here
    return array(
        'empty_data' => function (Options $options, $currentValue) {
            if ($options['data_class'] == 'foo') { // only now "data_class" is resolved
                ...
            }
        }
    );
}

However, I'm not confident that such an approach would not uncover yet more problems. Clever ideas?

stof · 2012-04-04T12:45:55Z

@bschussek the issue with PHP inheritance is that it requires dropping the dynamic inheritance between types, which is a powerful feature of the Form component.

webmozart · 2012-04-04T12:53:55Z

@stof I'm aware of that. I'm currently experimenting with the other approach.

This demonstrates the issue described in symfony#3354. FieldType no longer has access to the child type's data_class option, which makes it unable to create the default closure for empty_data.

@fabpot

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.

This demonstrates the issue described in symfony/symfony#3354. FieldType no longer has access to the child type's data_class option, which makes it unable to create the default closure for empty_data.

stof mentioned this issue Feb 19, 2012

test for ensuring creating form returns data of type specified in data_c... #3383

Closed

stof mentioned this issue Feb 22, 2012

[Form] form options aren't passed properly #3327

Closed

Burgov mentioned this issue Mar 17, 2012

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

Closed

jmikola mentioned this issue Mar 23, 2012

[Form] Failing test for empty_data option BC break #3685

Closed

vicb mentioned this issue Mar 26, 2012

[Form] New items in collection form type are not hydrated to proper dataClass #3694

Closed

yethee mentioned this issue Apr 1, 2012

Form returns array instead of object, despite data_class option being properly set. #3753

Closed

webmozart mentioned this issue Apr 5, 2012

[Form] Fixed option support in Form component #3789

Merged

fabpot closed this as completed Apr 11, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

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

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!