[Form] Cleaned up the Form API #4387

webmozart · 2012-05-23T19:06:06Z

Bug fix: no
Feature addition: no
Backwards compatibility break: YES
Symfony2 tests pass: yes
Fixes the following tickets: #3855, #3879, #4342, #4371, #4375
Todo: -

This PR cleans up the Form API as described in the referenced tickets in order to stabilize and freeze this API in the future. BC is kept wherever possible. After this PR, form types are expected to follow the following interface:

<?php

class MyType extends AbstractType
{
    public function buildForm(FormBuilderInterface $builder, array $options)
    {
    }

    public function buildView(FormViewInterface $view, FormInterface $form, array $options)
    {
    }

    public function finishView(FormViewInterface $view, FormInterface $form, array $options)
    {
    }

    public function setDefaultOptions(OptionsResolverInterface $resolver)
    {
    }

    public function getParent()
    {
        return 'form';
    }

    public function getName()
    {
        return 'my_type';
    }
}

Note that the options are now passed to buildView and finishView (formerly buildViewBottomUp) as well, reducing the need for creating form attributes in most cases.

travisbot · 2012-05-23T19:07:44Z

This pull request fails (merged 277f5f78 into e023807).

stof · 2012-05-23T19:12:21Z

UPGRADE-2.1.md

+    ```
+
+  * A third argument $options was added to the methods `buildView()` and
+    `buildViewBottomUp()` in `FormTypeInterface` and `FormTypeExtensionInterface`.


buildViewBottomUp should be left unmodified IMO as it is deprecated anyway. People wanting to use the new argument to access the options could simply use the new finishView method instead

buildViewBottomUp was removed completely. Do you think there's any point in leaving it in? People need to adapt their form types anyway.

webmozart · 2012-05-23T19:51:40Z

The PR now also contains the fix for #4342.

travisbot · 2012-05-23T19:51:55Z

This pull request fails (merged 13d284ba into e023807).

Tobion · 2012-05-23T20:12:56Z

src/Symfony/Component/Form/CHANGELOG.md

+   FormView
+ * [BC BREAK] the method `buildViewBottomUp` was renamed to `finishView` in
+   FormTypeInterface and FormTypeExtensionInterface
+ * [BC BREAK] the options array is now passed as last argument of the


This isn't bc break, or?

It is a BC break because it changes the signature of the interfaces

travisbot · 2012-05-24T06:55:35Z

This pull request fails (merged 5aba0778 into e023807).

travisbot · 2012-05-24T06:56:28Z

This pull request fails (merged 00c4f7e into b07fb3c).

travisbot · 2012-05-24T12:26:25Z

This pull request fails (merged 73cd9f8 into b07fb3c).

webmozart · 2012-05-24T12:27:32Z

The FormView changes described in #4371 are now contained as well. Invitation for final review.

webmozart · 2012-05-24T12:36:26Z

UPGRADE-2.1.md

+
+    Instead of `getChildren` and `hasChildren`, you should now use `all` and
+    `count` instead.
+


travisbot · 2012-05-24T14:03:10Z

This pull request fails (merged e1f502b into b07fb3c).

Tobion · 2012-05-24T14:13:46Z

src/Symfony/Component/OptionsResolver/OptionsResolverInterface.php

+     *
+     * @return OptionsResolverInterface The resolver instance.
+     *
+     * @throws Exception\OptionDefinitionException When trying to pass default values.


I think this exception shouldn't be enforced in the interface.
The implementation might throw this exception so people get warned about false usage. On the other hand, it also throws false positives:
Why am I not allowed to pass array('mykey' => 'optionname')? It limits the use unnecessarily. And the exception message is also wrong in this case, as I'm not trying to pass default values.
Anyway, IMO it should not be in the interface.

I think it is more important to prevent false usage than to require the casual use of array_values().

My point is, having a string key is not false usage per se. And the implementation shouldn't (and doesn't) care about the array key (no need vor array_values). For example currently you allow to use array(5 => 'option1', 89398 => 'option2') (non consecutive intergers), but array('5' => 'option1', '89398' => 'option2') (same with string key) raises exception.

cordoval · 2012-05-25T03:26:05Z

any ETA @bschussek ? I want to use it for tackling the problem of dynamic selects city-state
here http://www.craftitonline.com/2011/08/symfony2-ajax-form-republish/

I am told:
"getDefaultOptions is changed to setDefaultOptions which will allow you to set depenedent values based on other forms"

so I am most interested +300!

webmozart · 2012-05-25T06:08:53Z

@cordoval I think you misunderstood this. The OptionsResolver allows you to set options dependent on other options, but of the same field. Also, this is already possible before this merge by setting an option to a closure as described in the README of the OptionsResolver component.

travisbot · 2012-05-25T06:35:53Z

This pull request fails (merged b61cc55 into b07fb3c).

vicb · 2012-05-25T06:42:24Z

@bschussek great changes ! I have just sent you a PR with some modifs related to deprecated features. I'll rebase and submit the other one we have already discussed.

travisbot · 2012-05-25T07:16:18Z

This pull request passes (merged e18830d into b07fb3c).

cordoval · 2012-05-25T07:19:07Z

@bschussek what is already possilble @ "Also, this is already possible before"

I am confused could you link to what you are referring to please?

travisbot · 2012-05-25T07:22:07Z

This pull request passes (merged 20c02a7 into b07fb3c).

webmozart · 2012-05-25T07:22:29Z

public function getDefaultOptions()
{
    return array(
        'a' => 'foo',
        'b' => function (Options $options) {
            return 'bar' === $options['a'] ? 'x' : 'y';
        }
    );
}

…option values

… be passed as scalars

…er $resolver) in FormTypeInterface

…p FormTypeInterface and FormTypeExtensionInterface

…med[Builder]()

…nterface

…ound". The opposite is now "simple".

…rmBuilder

travisbot · 2012-05-25T10:38:04Z

This pull request passes (merged bc15e2d into 45849ce).

@cordoval

Commits ------- bc15e2d [Form] Some code cleanup 94f6f77 Restructured Form section of UPGRADE 3d800af [Form] Remove usages of deprecated features ee803cd [Form] Renamed setVars() to addVars() in FormViewInterface 1c4f632 [Form] Fixed API docs and usage of FormBuilderInterface instead of FormBuilder 2e6cdd1 [Form] Inverted the logic of "single_control" and renamed it to "compound". The opposite is now "simple". 98a7c0c [Form] Consolidated FormInterface, FormBuilderInterface and FormViewInterface 8e128fc [Form][OptionsResolver] Fixed typos 877d8f7 [Form] Reversed the order of $type and $name in FormFactory::createNamed[Builder]() 33fecca [Form] Merged various form events and added class FormEvent bec8015 [Form] Renamed client and application format to view and model format 8cae328 [Form] setDefaultOptions() is now coded against OptionsResolverInterface 1ecddbc [OptionsResolver] Renamed recommended method to setDefaultOptions() dc2fa9d [OptionsResolver] Added OptionsResolverInterface 2cd99e8 [Form] Added FormBuilderInterface and FormViewInterface and cleaned up FormTypeInterface and FormTypeExtensionInterface 0ef4066 [Form] Options are now passed to buildView() and buildViewBottomUp() 027259e [Form] Changed getDefaultOptions() to setDefaultOptions(OptionsResolver $resolver) in FormTypeInterface b4e8bcf [OptionsResolver] Relaxed tests to check that allowed values can also be passed as scalars 97de004 [OptionsResolver] Added option type validation capabilities 0af5f06 [OptionsResolver] Added method setFilters() for augmenting the final option values Discussion ---------- [Form] Cleaned up the Form API Bug fix: no Feature addition: no Backwards compatibility break: **YES** Symfony2 tests pass: yes Fixes the following tickets: #3855, #3879, #4342, #4371, #4375 Todo: - This PR cleans up the Form API as described in the referenced tickets in order to stabilize and freeze this API in the future. BC is kept wherever possible. After this PR, form types are expected to follow the following interface: ```php <?php class MyType extends AbstractType { public function buildForm(FormBuilderInterface $builder, array $options) { } public function buildView(FormViewInterface $view, FormInterface $form, array $options) { } public function finishView(FormViewInterface $view, FormInterface $form, array $options) { } public function setDefaultOptions(OptionsResolverInterface $resolver) { } public function getParent() { return 'form'; } public function getName() { return 'my_type'; } } ``` Note that the options are now passed to `buildView` and `finishView` (formerly `buildViewBottomUp`) as well, reducing the need for creating form attributes in most cases. --------------------------------------------------------------------------- by travisbot at 2012-05-23T19:07:44Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1414486) (merged 277f5f78 into e023807). --------------------------------------------------------------------------- by bschussek at 2012-05-23T19:51:40Z The PR now also contains the fix for #4342. --------------------------------------------------------------------------- by travisbot at 2012-05-23T19:51:55Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1414932) (merged 13d284ba into e023807). --------------------------------------------------------------------------- by travisbot at 2012-05-24T06:55:35Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1419776) (merged 5aba0778 into e023807). --------------------------------------------------------------------------- by travisbot at 2012-05-24T06:56:28Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1419783) (merged 00c4f7e into b07fb3c). --------------------------------------------------------------------------- by travisbot at 2012-05-24T12:26:25Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1421748) (merged 73cd9f8 into b07fb3c). --------------------------------------------------------------------------- by bschussek at 2012-05-24T12:27:32Z The FormView changes described in #4371 are now contained as well. Invitation for final review. --------------------------------------------------------------------------- by travisbot at 2012-05-24T14:03:10Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1422116) (merged e1f502b into b07fb3c). --------------------------------------------------------------------------- by cordoval at 2012-05-25T03:26:05Z any ETA @bschussek ? I want to use it for tackling the problem of dynamic selects city-state here http://www.craftitonline.com/2011/08/symfony2-ajax-form-republish/ I am told: "getDefaultOptions is changed to setDefaultOptions which will allow you to set depenedent values based on other forms" so I am most interested +300! --------------------------------------------------------------------------- by bschussek at 2012-05-25T06:08:53Z @cordoval I think you misunderstood this. The OptionsResolver allows you to set options dependent on other options, but of the same field. Also, this is already possible before this merge by setting an option to a closure as described in the README of the OptionsResolver component. --------------------------------------------------------------------------- by travisbot at 2012-05-25T06:35:53Z This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1430534) (merged b61cc55 into b07fb3c). --------------------------------------------------------------------------- by vicb at 2012-05-25T06:42:24Z @bschussek great changes ! I have just sent you a PR with some modifs related to deprecated features. I'll rebase and submit the other one we have already discussed. --------------------------------------------------------------------------- by travisbot at 2012-05-25T07:16:18Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1430699) (merged e18830d into b07fb3c). --------------------------------------------------------------------------- by cordoval at 2012-05-25T07:19:07Z @bschussek what is already possilble @ "Also, this is already possible before" I am confused could you link to what you are referring to please? --------------------------------------------------------------------------- by travisbot at 2012-05-25T07:22:07Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1430727) (merged 20c02a7 into b07fb3c). --------------------------------------------------------------------------- by bschussek at 2012-05-25T07:22:29Z ``` public function getDefaultOptions() { return array( 'a' => 'foo', 'b' => function (Options $options) { return 'bar' === $options['a'] ? 'x' : 'y'; } ); } --------------------------------------------------------------------------- by travisbot at 2012-05-25T10:38:04Z This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1431903) (merged bc15e2d into 45849ce).

stof reviewed May 23, 2012
View reviewed changes

Tobion reviewed May 23, 2012
View reviewed changes

webmozart reviewed May 24, 2012
View reviewed changes

Tobion reviewed May 24, 2012
View reviewed changes

webmozart added 3 commits May 25, 2012 12:28

[OptionsResolver] Added method setFilters() for augmenting the final …

0af5f06

…option values

[OptionsResolver] Added option type validation capabilities

97de004

[OptionsResolver] Relaxed tests to check that allowed values can also…

b4e8bcf

… be passed as scalars

webmozart and others added 17 commits May 25, 2012 12:28

[Form] Changed getDefaultOptions() to setDefaultOptions(OptionsResolv…

027259e

…er $resolver) in FormTypeInterface

[Form] Options are now passed to buildView() and buildViewBottomUp()

0ef4066

[Form] Added FormBuilderInterface and FormViewInterface and cleaned u…

2cd99e8

…p FormTypeInterface and FormTypeExtensionInterface

[OptionsResolver] Added OptionsResolverInterface

dc2fa9d

[OptionsResolver] Renamed recommended method to setDefaultOptions()

1ecddbc

[Form] setDefaultOptions() is now coded against OptionsResolverInterface

8cae328

[Form] Renamed client and application format to view and model format

bec8015

[Form] Merged various form events and added class FormEvent

33fecca

[Form] Reversed the order of $type and $name in FormFactory::createNa…

877d8f7

…med[Builder]()

[Form][OptionsResolver] Fixed typos

8e128fc

[Form] Consolidated FormInterface, FormBuilderInterface and FormViewI…

98a7c0c

…nterface

[Form] Inverted the logic of "single_control" and renamed it to "comp…

2e6cdd1

…ound". The opposite is now "simple".

[Form] Fixed API docs and usage of FormBuilderInterface instead of Fo…

1c4f632

…rmBuilder

[Form] Renamed setVars() to addVars() in FormViewInterface

ee803cd

[Form] Remove usages of deprecated features

3d800af

Restructured Form section of UPGRADE

94f6f77

[Form] Some code cleanup

bc15e2d

fabpot merged commit bc15e2d into symfony:master May 25, 2012

craue mentioned this pull request May 25, 2012

[Form] warning in ViolationMapper #4411

Closed


		Instead of `getChildren` and `hasChildren`, you should now use `all` and
		`count` instead.

Uh oh!

[Form] Cleaned up the Form API #4387

[Form] Cleaned up the Form API #4387

Uh oh!

Conversation

webmozart commented May 23, 2012

Uh oh!

travisbot commented May 23, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

webmozart commented May 23, 2012

Uh oh!

travisbot commented May 23, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented May 24, 2012

Uh oh!

travisbot commented May 24, 2012

Uh oh!

travisbot commented May 24, 2012

Uh oh!

webmozart commented May 24, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

travisbot commented May 24, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cordoval commented May 25, 2012

Uh oh!

webmozart commented May 25, 2012

Uh oh!

travisbot commented May 25, 2012

Uh oh!

vicb commented May 25, 2012

Uh oh!

travisbot commented May 25, 2012

Uh oh!

cordoval commented May 25, 2012

Uh oh!

travisbot commented May 25, 2012

Uh oh!

webmozart commented May 25, 2012

Uh oh!

travisbot commented May 25, 2012

Uh oh!

Uh oh!