[Form] Failing test for empty_data option BC break#3685
Closed
jmikola wants to merge 1 commit intosymfony:masterfrom
jmikola:form-empty-data
Closed
[Form] Failing test for empty_data option BC break#3685jmikola wants to merge 1 commit intosymfony:masterfrom jmikola:form-empty-data
jmikola wants to merge 1 commit intosymfony:masterfrom
jmikola:form-empty-data
Conversation
This demonstrates the issue described in #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.
Contributor
|
Just to clarify this: the issue is not only about |
Contributor
Author
Contributor
|
Is assertEquals the right choice here? $author wont === $form->getData().. |
Member
|
@merk |
Contributor
|
I made a similar test and implemented a fix in #3512 (I saw this PR only after I wrote the test) |
Contributor
Author
Contributor
|
Thanks for this test Jeremy. I will merge it once I'm resolving this issue. |
Contributor
Author
|
FYI: #3789 includes this commit, so we can close this once that is merged. |
Contributor
|
@jmikola: AFAIK this PR auto-closes once the commit is included in master. |
fabpot
added a commit
that referenced
this pull request
Apr 11, 2012
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: -  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 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 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.
This demonstrates the issue described in #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.
We're still waiting for @bschussek to chime in on #3354, so there is no rush to merge this. Most users are expecting
data_classto be all that's necessary to facilitate creation of a new form data object -- it's not clear if that is still the intended functionality after the recent form BC breaks, or if those breaks were an oversight.If it is to remain intended functionality, I think we can keep this test around after the issue is resolved.