[Form] Fix FormView API #4371

webmozart · 2012-05-22T15:38:24Z

In the course of the Form API clean-up, I would like to propose a fix that unfortunately has a pretty bad impact, BC-wise.

The API of FormView is inconsistent with the API of FormBuilder and Form in the following aspect:

<?php

// FormBuilder
$builder->add($child);
$builder->get('childName');
$builder->setAttribute('foo', 'bar');
$builder->getAttribute('foo');

// FormInterface
$form->add($child); // or $form[] = $child
$form->get('childName'); // or $form['childName']
// no setAttribute()
$form->getAttribute('foo');

// FormView
$view->addChild($child); // or $view[] = $child
$view->getChild('childName'); // or $view['childName']
$view->set('foo', 'bar');
$view->get('foo');
$view->all(); // or $view->getVars()

I think that this is confusing and should be made consistent before marking the API as stable.

<?php

$view->add($child); // or $view[] = $child
$view->get('childName'); // or $view['childName']
$view->setVar('foo', 'bar');
$view->getVar('foo');
$view->setVars(array(
    'foo' => ...
    'bar' => ...
));
$view->getVars();

Impact:

FormTypes setting variables in the FormView must be adapted
PHP templates must be adapted (to access getVar() instead of get())

Thoughts?

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

beberlei · 2012-05-22T15:55:36Z

tough change tbh. Makes it pretty hard to have a bundle work on 2.0 and 2.1

Seldaek · 2012-05-22T15:58:08Z

(Random thoughts warning)

Kinda sucks for the template change. I guess the other one won't affect too many people. Is it really a big hurdle? Most people don't interact with the view apart from templates.

Also setVar sounds weird. setParameter might be more familiar.

Another random idea (no idea about feasibility since I don't know FormView or the rendering process well enough): What about exposing a $view->getData() or such that returns a ParameterBag-style object. That object could then be passed to the templates, so the get() call in templates still work. That might work if templates never need to call anything else on the view object than the data.

stof · 2012-05-22T16:05:32Z

Depending of how you write your form theme, you may not be affected: vars are available as variables in the template. For instance, I doubt that the core themes would need much changes (the FormExtension for Twig and the FormHelper for PhpEngine would of course require changes).

@Seldaek templates definitely need the FormView, not a ParameterBag. It is what is pass to all form_* functions in your Twig templates (and available as the form variable in the theme)

vendethiel · 2012-05-22T16:14:35Z

Just a side-note : I'm +1 on doing bc breaks now, rather than little by little

webmozart · 2012-05-22T16:36:40Z

@beberlei It makes it hard, but not impossible. You can still push your vars to an array and test for the presence of setVars before calling it. Not very beautiful, but not unsolvable either.

<?php

public function buildView(FormView $view, FormInterface $form)
{
    $vars = array();
    // do stuff

    // BC
    if (method_exists($view, 'setVars')) {
        $view->setVars($vars);
    } else {
        foreach ($vars as $var => $value) {
            $view->set($var, $value);
        }
    }
}

@Seldaek We can't change the name "vars" anymore unless we break BC even harder. That's out of discussion for me.

I don't see either how it sucks for the template language. For PHP templates, those rare occasions where you have to call get don't hurt if you know have to call getVar. Twig templates aren't affected by this change if you follow the best practices and access variables like this:

{{ form.vars.varname }}

cordoval · 2012-05-22T17:32:57Z

+1 at least this is good to know this is possible and since few people drive things like this in their formtypes i think it is kind of advantageous, just hope it is well documented :D

jonathaningram · 2012-05-22T22:41:36Z

+1 can't you just make the old methods throw a deprecated error and inform the developer what method to use instead - that will allow for fast BC fixing on the developers end.

cordoval · 2012-05-22T22:42:38Z

and why not a link to this alternative solution

stof · 2012-05-22T23:12:46Z

@jonathaningram throwing a deprecated error would be a BC break as the kernel registers the error handling to E_ALL | E_STRICT and an error handlers turning them into exceptions when you are in debug mode. So triggering an error would break your dev environment

< 8000 !-- -->

jonathaningram · 2012-05-22T23:21:21Z

@stof that was my intention - to break dev so that we as developers can pinpoint the areas to rename (which is essentially what this PR is about, no?). I'm just favouring a big ugly error for the developer rather than silently renaming methods (if that was to be the outcome of the PR)

stof · 2012-05-22T23:32:47Z

if you break the dev environment, it is a hard BC break. Currently, Symfony annotates deprecated methods in thir phpdoc (most IDE will warn you when using them) but does not break things in them (it will break in 2.3 when removing the method deprecated in 2.1)

jonathaningram · 2012-05-22T23:34:40Z

@stof no problems, I understand. Makes sense :)

lstrojny · 2012-05-22T23:54:40Z

What about introducing the new API and deprecating the old one as usual with a plan to remove it in 2.3?

webmozart · 2012-05-23T06:52:31Z

It is pretty difficult to make this change while maintaining BC, since the same method (get()) has a different meaning in the new API. The best we can do is adding a global switch that disables the BC API and enables the new API, but I don't even know if this is so easy.

stof · 2012-05-23T07:28:03Z

this would be pretty hard for built-in types and the rendering helpers: you would have to maintain 2 sets of classes and enable one of them in the DIC depending of the switch, and the same would be true for all shared bundles providing forms

lstrojny · 2012-05-23T07:47:41Z

Sounds like a maintenance nightmare Inders. So let's do it the hard way. Anyway we should have a clear migration guide

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).

webmozart mentioned this issue May 23, 2012

[Form] Cleaned up the Form API #4387

Merged

fabpot closed this as completed May 25, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Form] Fix FormView API #4371

[Form] Fix FormView API #4371

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] Fix FormView API #4371

[Form] Fix FormView API #4371

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!