8000 [Form] Fix FormView API · Issue #4371 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Fix FormView API #4371

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
webmozart opened this issue May 22, 2012 · 16 comments
Closed

[Form] Fix FormView API #4371

webmozart opened this issue May 22, 2012 · 16 comments
Labels
Enhancement Form

Comments

@webmozart
Copy link
Contributor

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?
@beberlei
Copy link
Contributor

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

@Seldaek
8000 Copy link
Member
Seldaek commented May 22, 2012

(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
Copy link
Member
stof commented May 22, 2012

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
Copy link

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

@webmozart
Copy link
Contributor Author

@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
Copy link
Contributor

+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
Copy link
Contributor

+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
Copy link
Contributor

and why not a link to this alternative solution

@stof
Copy link
Member
stof commented May 22, 2012

@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

@jonathaningram
Copy link
Contributor

@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
Copy link
Member
stof commented May 22, 2012

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
Copy link
Contributor

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

@lstrojny
Copy link
Contributor

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

@webmozart
Copy link
Contributor Author

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
Copy link
Member
stof commented May 23, 2012

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
Copy link
Contributor

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

@webmozart webmozart mentioned this issue May 23, 2012
Merged
fabpot added a commit that referenced this issue May 25, 2012
@fabpot
merged branch bschussek/issue3879 (PR #4387)
023dbf8 
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).
@fabpot fabpot closed this as completed May 25, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement Form
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@beberlei @fabpot @lstrojny @webmozart @Seldaek @vendethiel @cordoval @stof @jonathaningram
0