8000 [Form] Replace [append|prepend][Norm|Client]Transformer methods with add[Norm|Client]Transformer · Issue #3855 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Replace [append|prepend][Norm|Client]Transformer methods with add[Norm|Client]Transformer #3855

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 Apr 10, 2012 · 7 comments
Closed

[Form] Replace [append|prepend][Norm|Client]Transformer methods with add[Norm|Client]Transformer #3855

webmozart opened this issue Apr 10, 2012 · 7 comments
Labels
Enhancement Form
Milestone
2.1

Comments

@webmozart
Copy link
Contributor

The following methods should be renamed:

  • FormBuilder::prependNormTransformer → addNormTransformer
  • FormBuilder::appendClientTransformer → addClientTransformer

The methods appendNormTransformer and prependClientTransformer should be removed. These assume that the normalized format can be changed. In reality, the normalized format is chosen by the creator of a type and should not be modified in order to keep the type working correctly.

This change simplifies the Form API and reduces WTF's arising when appendNormTransformer or prependClientTransformer are used.
@kriswallsmith
Copy link
Contributor

We use prependClientTransformer() at @opensky. We're looking at whether append would work in these cases.

@webmozart
Copy link
Contributor Author

Thanks @kriswallsmith. In these cases, prependNormTransformer() might work as well.

@rande
Copy link
Contributor
rande commented Apr 11, 2012

This change might break if a field is altered by an FieldExtension. But maybe the FieldExtension instance can reset the current transformers...

@webmozart
Copy link
Contributor Author

@rande Can you give a more precise example? As far as I see, FieldExtensions have the same extension possibilities as subtypes.

@kriswallsmith Any news on your case?

@rande
Copy link
Contributor
rande commented Apr 11, 2012

@bschussek I don't have precise example. I only know that a FieldTypeExtension can alter the FormBuilder to add new types or remove defined ones.

So I can imagine a situation where a FieldTypeExtension add a new type and a transformation must be done before the original transformer (the one defined in the FieldType).

@webmozart
Copy link
Contributor Author
8000

@rande I doubt this could (should) happen. Any kind of field type should be self-contained: It has a set of allowed input types and of allowed output types. If I change the transformation between input and output type, I break the type. Adding new conversions from my custom to the original input or from the original output to my custom output should be the correct way to go.

@rande
Copy link
Contributor
rande commented Apr 12, 2012

@bschussek I know, it is a very rare edge case ;) but the issue is still possible and removing methods will limit possibilities (or avoid this situation, which can be good too ...)

@stof stof mentioned this issue Apr 21, 2012
Closed
@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
2.1
Development

No branches or pull requests
4 participants
@rande @kriswallsmith @fabpot @webmozart
0