Skip to content

Forms Refactoring #399

Closed
wants to merge 281 commits into from
@webmozart
Symfony member

Hi all,

Finally I consider the Form API of the refactored branch to be mostly stable. Mostly means that the public API - the parts that are used by 80% of the users - aren't going to change anymore, but private API (interface, class and method names) might change in the next days of cleaning up

Improvements over the existing API:

  • proper use of DI throughout the whole component
  • massively better decoupling = more extension points
  • simplified user experience
  • much better rendering layer
  • no more hacks

What's missing (due to time pressure in the last weeks):

  • inline documentation
  • unit tests for new functionality. Old functionality is still validated by the existing tests.
  • a list of small features
  • small API simplifications
  • configuration hooks

I don't want to write a book now about all the things that changed. If you are interested in the technical details, I invite you to read the (still poorly documented) code.

You can also look at Benjamin Eberlei's PizzaBundle, which demonstrates some more complex use cases with the new API:
https://github.com/beberlei/AcmePizzaBundle

Ryan is currently rewriting the Form documentation to match the new API. The first results will be pushed this weekend.

To start, I've prepared a Gist that demonstrates the basic functionality of the new API:
https://gist.github.com/883293

Before we can merge this PR, I want to fix a few tests that are currently skipped because the API changed, but the tests weren't rewritten yet.

Nevertheless, please use the new API and give me feedback. Various people are already using it and satisfied so far.

Bernhard

webmozart added some commits Feb 24, 2011
@webmozart webmozart Merge branch 'bugfix' into experimental
Conflicts:
	src/Symfony/Component/Form/EntityChoiceField.php
	src/Symfony/Component/Form/Field.php
	src/Symfony/Component/Form/HybridField.php
	tests/Symfony/Tests/Component/Form/FieldTest.php
	tests/Symfony/Tests/Component/Form/FormTest.php
e3e8c29
@webmozart webmozart [Form] Fixed failing DateFieldTest and TimeFieldTest acc5c76
@webmozart webmozart [Form] Fixed EntityChoiceFieldTest 0f8a413
@webmozart webmozart [Form] Fixed DateTimeFieldTest 87b4178
@webmozart webmozart [Form] Refactored RepeatedField to FormFactory 848ec01
@webmozart webmozart [Form] Refactored FileField to FormFactory and fixed file upload mech…
…anism
f2c1976
@webmozart webmozart [Form] Registered FormFactory in the DIC e334c47
@webmozart webmozart Merge branch 'master' into experimental eca2b87
@webmozart webmozart [Form] Fixed various bugs c6e9fd9
@webmozart webmozart [Form] Added ArrayAccess interface to DefaultRenderer adbf2cb
@webmozart webmozart [Form] Improved rendering
Fields are not available in the templates anymore. Instead, all required information can be
accessed through view variables.

Example usage of helpers and variables in a form theme:

// use the label helper
{{ this.label('my label') }}

// use the label variable
{{ this.vars.label }}
{{ label }}

Example usage of helpers and variables in a normal template:

// use the label helper
{{ field.label('my label') }}

// use the label variable
{{ field.vars.label }}
02d2121
@webmozart webmozart [Form] Removed notion of "hidden" fields
Instead, hidden fields now override the "row" template to not include a label or errors.

The "rest" (former "hidden") helper has been adapted to output any fields that were not
rendered manually. It should usually be called at the end of a form.
c1edf11
@webmozart webmozart [Form] Implemented generic data filter hooks
You can now modify set or bound data by adding a filter for either of the following events:

* Filters::filterBoundDataFromClient
* Filters::filterBoundData
* Filters::filterSetData
528ef55
@webmozart webmozart [Form] Added EventListener implementation and moved CollectionField t…
…o factory
9eff64d
@webmozart webmozart [Form] Refactored contents of FormFactory into individual FieldConfig…
… classes
8a6246b
@webmozart webmozart Merge remote branch 'symfony/master' into experimental 68013f4
@avalanche123

must be a typo, $currentYear is not defined

webmozart and others added some commits Mar 2, 2011
@webmozart webmozart [Form] Refactored FieldFactory and moved new implementation into the …
…DIC. FormTest fails now.
5705f74
@webmozart webmozart [Form] Fixed CSRF protection ea49621
@webmozart webmozart [Form] Fixed RepeatedField, improved structure of the Twig templates e53c688
@webmozart webmozart [Form] The TwigTheme now accepts several templates 0f8cd43
@webmozart webmozart [Form] Added ArrayAccess to DefaultRenderer bfbc112
@webmozart webmozart [Form] Added TODOs and a tweak 4346097
@webmozart webmozart Merge branch 'event-manager' into experimental
Conflicts:
	src/Symfony/Component/Form/BirthdayField.php
	src/Symfony/Component/Form/CheckboxField.php
	src/Symfony/Component/Form/ChoiceField.php
	src/Symfony/Component/Form/ChoiceList/TimeZoneChoiceList.php
	src/Symfony/Component/Form/CollectionField.php
	src/Symfony/Component/Form/DateField.php
	src/Symfony/Component/Form/DateTimeField.php
	src/Symfony/Component/Form/EntityChoiceField.php
	src/Symfony/Component/Form/Events.php
	src/Symfony/Component/Form/FieldFactory/FieldFactory.php
	src/Symfony/Component/Form/FieldFactory/FieldFactoryInterface.php
	src/Symfony/Component/Form/FileField.php
	src/Symfony/Component/Form/Filters.php
	src/Symfony/Component/Form/FormContext.php
	src/Symfony/Component/Form/FormContextInterface.php
	src/Symfony/Component/Form/FormFactoryInterface.php
	src/Symfony/Component/Form/HybridField.php
	src/Symfony/Component/Form/IntegerField.php
	src/Symfony/Component/Form/LanguageField.php
	src/Symfony/Component/Form/LocaleField.php
	src/Symfony/Component/Form/MoneyField.php
	src/Symfony/Component/Form/NumberField.php
	src/Symfony/Component/Form/PasswordField.php
	src/Symfony/Component/Form/PercentField.php
	src/Symfony/Component/Form/RepeatedField.php
	src/Symfony/Component/Form/TextField.php
	src/Symfony/Component/Form/TimeField.php
	src/Symfony/Component/Form/ToggleField.php
	src/Symfony/Component/Form/UrlField.php
	src/Symfony/Component/HttpFoundation/File/UploadedFile.php
	tests/Symfony/Tests/Component/Form/FileFieldTest.php
	tests/Symfony/Tests/Component/Form/FormContextTest.php
	tests/Symfony/Tests/Component/Form/HiddenFieldTest.php
0bf5663
@webmozart webmozart Merge remote branch 'symfony/master' into experimental
Conflicts:
	src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php
	src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php
f64f550
@webmozart webmozart [Form] Fixed a couple of failing tests fb8efab
@webmozart webmozart [Form] Changed form to use the new EventDispatcher implementation e85aab2
@webmozart webmozart [Form] Moved trimming logic to listener caa49aa
@webmozart webmozart [Form] Renamed 'key' to 'name'. Removed setKey() totally. cb283d3
@webmozart webmozart [Form] Removed dependency from renderer plugins on fields. The field …
…instance is now passed to setUp() instead.
eba602e
@webmozart webmozart Merge branch 'event-manager' into experimental
Conflicts:
	src/Symfony/Component/Security/Http/Firewall/AbstractAuthenticationListener.php
	src/Symfony/Component/Security/Http/Firewall/AbstractPreAuthenticatedListener.php
1c9a007
@webmozart webmozart [Form] Fixed DateTimeToArrayTransformer when transforming empty value…
…s and the option 'fields' is set
f0d841e
@webmozart webmozart [Form] Removed FieldInterface::getDisplayedData(). Use getTransformed…
…Data() instead
50ce0d5
@webmozart webmozart [Form] Moved logic of Field::isMultipart() to rendering layer ffa5bd2
@webmozart webmozart [Form] Removed deprecated method preprocessData() f2f7889
@webmozart webmozart [Form] Renamed bind() to bindRequest(). It is now semantically the sa…
…me as in symfony1 again
5bfd02b
@webmozart webmozart [Form] Renamed submit() to bind() 7a63b84
@webmozart webmozart [Form] Moved form logic to event listeners 8e41cc6
@webmozart webmozart [Form] Removed unused method Form::deepArrayUnion() d00f1fa
@beberlei beberlei First attempt of PhpTheme. 85b61fe
@beberlei beberlei Merge branch 'experimental' of git://github.com/bschussek/symfony int…
…o forms
3bc825b
@webmozart webmozart [Form] Extracted data mapping logic from Form into ObjectMapperListener 0799662
@webmozart webmozart [Form] Moved validation logic to ValidationListener a51321c
@beberlei beberlei [Form] Implemented first PhpTheme attempt 76b0041
@johnwards johnwards [FrameworkBundle] Fixed filename of TraceableEventDispatcher d5adaa2
@johnwards johnwards [FrameworkBundle] Added missing argument for listenerToString() call 380afc5
@johnwards johnwards [FrameworkBundle] Fixed typo in ProfileListener cd3e184
@johnwards johnwards [FrameworkBundle] Fixed undefined $response variable in ProfilerListener c56a803
@johnwards johnwards [FrameworKBundle] Fixed TraceableEventDispatcher to use parent::getLi…
…steners() because parent::$listeners is now private. Passed event name to listenerToString()
694c7fd
@johnwards johnwards [FrameworkBundle] Fixed order of the classes to compile for EventDisp…
…atcher
7c0fe6a
@webmozart webmozart Merge branch 'event-manager' into experimental 52f382a
@webmozart webmozart Merge remote branch 'symfony/master' into experimental
Conflicts:
	src/Symfony/Bundle/FrameworkBundle/Debug/TraceableEventDispatcher.php
	src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
	src/Symfony/Component/HttpFoundation/File/UploadedFile.php
3f70f89
@webmozart webmozart [Form] Introduced FieldBuilder for field/form creation and made Field…
…/Form mostly immutable
9caaf0f
@webmozart webmozart [Form] Moved error distribution logic to validation listener 4cb76c0
@Koc
Symfony member

Thanks, fixed

webmozart and others added some commits Mar 18, 2011
@webmozart webmozart [Form] Moved FieldFactoryTest to FormFactoryTest and fixed it 700c96e
@webmozart webmozart [Form] Moved properties propertyPath, modifyByReference, validationGr…
…oups and virtual to generic attributes because they are specific to the used data validator/mapper implementations
97d0183
@webmozart webmozart [Form] Extracted data validation logic into DataValidatorInterface f86ecec
@webmozart webmozart [Form] Renamed ValueTransformers to DataTransformers to fit with Data…
…Mapper and DataValidator
0b929ee
@webmozart webmozart [Form] Renamed field configs to types. Everywhere else, we are referr…
…ing to the name of the type as 'type' now (was 'identifier')
2a1e394
@webmozart webmozart [Form] Split FieldBuilder into FieldBuilder and FormBuilder to make c…
…reation of classes deterministic
1c85daa
@webmozart webmozart [Form] Renamed different kinds of data transformers within a field fo…
…r better clarity
6bc79a1
@webmozart webmozart [Form] Fixed undefined variable in DefaultRenderer b66b832
@beberlei beberlei Merge branch 'experimental' of git://github.com/bschussek/symfony int…
…o forms
d44e226
@beberlei beberlei Rename this to renderer to be able to use it in Php Templating bb8c2a9
@webmozart webmozart Merge remote branch 'symfony/master' into experimental 89215d1
@webmozart webmozart [FrameworkBundle] Fixed DI configuration for field types 8742caf
@beberlei beberlei Fix some missing use statements and small bugs. 3e2b8e5
@webmozart webmozart [Form] Moved CSRF protection into separate field 3e17b26
@webmozart webmozart [Form] Removed unused Form::isCsrfTokenValid() d2210a2
@beberlei beberlei Finished first version of PhpTheme, added huge functional test. 44e85ae
@beberlei beberlei Merge bschussek/experimental into branch forms. 32e1a7c
@beberlei beberlei Moved PhpTheme to FrameworkBundle and renamed to PhpEngineTheme af60ddf
@webmozart webmozart [Form] Moved namespace FieldGuesser to Type\Guesser 7f92841
@webmozart webmozart [Form] Moved namespace DataValidator to Validator 0259d4d
@webmozart webmozart [Form] Renamed events to match terminology in Field 43a24fa
@beberlei beberlei [Form] Add PhpTheme that relies on no Template Engine, generalize tes…
…ts to use for PhpTheme and PhpEngineTheme. Fixed some bugs
39c2d3f
@beberlei beberlei [Form] Add DefaultFormFactory for convenience usage outside of a DIC …
…context.
c05b3c4
@webmozart webmozart [Form] Fixed failing choice field tests 65353bd
@webmozart webmozart [Form] Renamed field types. They are now always the name of the type …
…with a 'Type' suffix
3586268
@webmozart webmozart Merge remote branch 'beberlei/forms' into beberlei-merge
Conflicts:
	src/Symfony/Component/Form/Type/Loader/DefaultTypeLoader.php
c4b7a77
@webmozart webmozart [Form] Removed magic from method signature of add() 6793bcd
@webmozart webmozart [Form] Added fluid interface for inline adding of sub-builders to a b…
…uilder
fb2db58
@webmozart webmozart [Form] Optimized code in Form to remove one event listener e9cb197
@webmozart webmozart [Form] Added comment a1c3d21
@beberlei beberlei [Form] Add StripTagsFilter EventListener 864a3ec
@beberlei beberlei Merge branch 'experimental' of git://github.com/bschussek/symfony int…
…o forms
d622136
@beberlei beberlei [Form] Add tests for untested classes. b43318e
@webmozart webmozart Merge remote branch 'beberlei/forms' into beberlei-merge 2c6c89d
@webmozart webmozart [Form] Renamed StripTagsFilter to StripTagsListener until we have bet…
…ter conventions
295d017
@webmozart webmozart [Form] Renamed ObjectMapper to PropertyPathMapper b5656f1
@webmozart webmozart [Form] Renamed field option 'disabled' to 'read_only'. How to render …
…read-only fields is now the responsibility of the renderer
cb599f4
@webmozart webmozart [Form] Extracted validation logic of form. Fields can now contain mul…
…tiple validators
fc7281b
@webmozart webmozart [Form] Merged Field and Form. Merged FieldBuilder and FormBuilder. Af…
…ter the refactoring, the distinction between the two concepts is small enough to merge them
2dbb417
@webmozart webmozart [Form] Renamed FormBuilder::getInstance() to getForm() 119866e
@webmozart webmozart [Form] Improved naming of data conversion methods in Form and made th…
…em private
6c9ff0a
@webmozart webmozart [Form] Fixed signature of Form::addError() acaa9c9
@webmozart webmozart [Form] Moved form type tests into according namespace 4989e96
@webmozart webmozart Merge remote branch 'symfony/master' into experimental 61804bb
@webmozart webmozart [Form] Decoupled FormBuilder from ThemeInterface 321d40b
@ericclemmons

@bschussek Easy tiger, I was pouring through the commits and was using the file src/Symfony/Component/Form/Type/CollectionType.php as an example. Just trying to help clarify why some found the Type vs Field naming confusing.

@webmozart
Symfony member

@ericclemmons: I didn't mean to offend you :) Keep suggestions coming with the above in mind!

@ericclemmons

@bschussek: No sweat :) I've been tracking the Forms changes for some time now (eager to replace most of my Zend\Form code), so was hoping this PR could stabilize asap for inclusion... (especially considering Ryan has already knocked out a lot of the new docs!)

@henrikbjorn

Have played with the experimental branch the last couple of days it is almost identical i use as the old one which is a plus for me as i dont need to put stuff into to the dic to use it.

Old
$form = new MyForm($this->get('form.factory'));

New hotness
$form = $this->get('form.factory')->create(new MyFormType);

so +1 for that.

The last problem i have is with rendering and how the errors are handled. To test this funtionality i created a FormType with 2 levels of nesting and that uses a repeated type. And the errors are not at the field and jumps around which is weird. Then using a password as the repeated field prototype it will also show double errors because the data cant be transformer so it will have "The value must not be blank" and "This value is invalid"

So for this to be useable the rendering part needs a bit of work, and for me i dont really like the $factory->createRenderer() call either as a form per definition needs to be rendered to be useful.

Last issue i have found, is that textarea type is rendered as a input text field.

@henrikbjorn

This breaks all the rendering because all other methods still calls render and not renderPart.

Symfony member

Thanks, fixed.

@webmozart
Symfony member

@henrikbjorn: Yes, error mapping needs to be tweaked. It's mostly relevant for the repeated field right now, but might also be relevant for custom fields. I'll work on that ASAP.

@weaverryan
Symfony member

I'm wondering if we could make the creation of forms more succinct. I don't like needing to set both the data_class and setData (there's redundancy there and the data_class is long). I realize that forms are just fields, but would something like the following be possible? Especially the last option, it's very straightforward.

https://gist.github.com/890407

@henrikbjorn

@weaverryan the difference is the data you are acting upon, and is hard to have default data etc if you dont provide the object or array, also if the DataClass have any constructor parameters it can not be auto instantiated.

@henrikbjorn

This Pull Request should remain open until there is a decision on this topic: https://groups.google.com/forum/?pli=1#!topic/symfony-devs/n3HznxzQW8A

As it requires changes to the form component.

@weaverryan
Symfony member

@henrikbjorn Take another look at the gist (the second or third pieces are what I'd like to see, the first is the current state) - I'm not changing the way anything is constructed - it's purely a re-arranging of where the existing pieces are placed. The $product data object is still created, but it's passed in differently, making the data_class unnecessary.

@henrikbjorn

There is missing a way to turn the guessers off, absolutely annoying to have them

webmozart added some commits Mar 28, 2011
@webmozart webmozart Merge remote branch 'symfony/master' into experimental
Conflicts:
	src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/money_field.html.php
	src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/percent_field.html.php
	src/Symfony/Component/Form/Resources/config/validation.xml
53838ab
@webmozart webmozart [FrameworkBundle][TwigBundle] Adapted bundle references to latest sym…
…fony/master changes
544f763
@henrikbjorn

@bschussek is it a bug that when the form is validated it validates the whole service object and not just the properties that are used in the form ?

@stof
Symfony member
stof commented Mar 29, 2011

@henrikbjorn this is what validation groups are for.

@henrikbjorn

@stof no before the refactoring it only validated the fields that the form contained.

@webmozart
Symfony member

@henrikbjorn That's not true. Validation behaviour didn't change.

@henrikbjorn

@bschussek what is the reason behind validating the whole object if the form dosent have any fields attached ? or at least default validation_group to a group that is attached to the fields in the form.

@henrikbjorn

There is a problem with _token fields, every embedded form seems to have their own _token field, shouldnt there only be one for the whole form ?

@brikou
brikou commented Apr 2, 2011

when using some kind:

->add('my_entity', 'entity', array('class' => 'Acme\MyBundle\Entity\MyEntity'))

it would be great to have the ability to display it with input type radio instead of select option. It would be cleaner when dealing with small collection.

we could also use checkbox if multiple allowed

btw, your job with form is very good indeed

@henrikbjorn

@brikou this is already possible by twaking the expanded and multiple options, because the field inherits from the choice list.

@brikou
brikou commented Apr 2, 2011

@henrikbjorn thx bschussek told me about this so nice feature :)

@webmozart
Symfony member

Since others and me as well are unhappy with the current notion of "type", what do you think about:

  • recipe (CheckboxRecipe, DateRecipe)
  • blueprint (CheckboxBlueprint, DateBlueprint)
  • scheme (CheckboxScheme, DateScheme)
  • config - we had that before (CheckboxConfig, DateConfig)

?

@brikou
brikou commented Apr 6, 2011
  • "Scheme" is too close to "Schema" and database reference, and too general
  • "Blueprint" is too long (to write down)
  • "Config" is too general
  • I like very much the "Recipe" notion because it is close to cooking, when you fill a form you have to follow instruction like in cooking

+1 for "Recipe"

@Seldaek
Symfony member
Seldaek commented Apr 6, 2011

I still think they should be called Field and Form. Because yes, they are not really fields or form, they're just configurations of lala, but that's an implementation detail that users shouldn't have to care about imo.

@brikou
brikou commented Apr 6, 2011
@henrikbjorn

+1 Recipe

@schmittjoh
@brikou
brikou commented Apr 6, 2011

@schmittjoh you're right I would prefer have inside MyBundle

MyBundle/Form/PostForm.php
MyBundle/Form/Field/CategoryField.php

instead of

MyBundle/Recipe/PostRecipe.php
MyBundle/Recipe/Ingedient/CategoryIngredient.php

or maybe it would be fun, i don't know ;)

@Seldaek
Symfony member
Seldaek commented Apr 6, 2011

@schmittjoh @brikou: well yeah, that's what I was trying to say before. Those types are the closest things to fields and forms that we have, so let's call them that, even though it's not a concrete implementation of a field or a form but just a way to configure the internal classes, nobody cares about that, people know about fields and forms, and will talk about that, and will wonder wtf Types are.

@webmozart
Symfony member

@schmittjoh: I agree that names should be as close to spoken language as possible. The problem is that your example is wrong.

When you talk about the "register form", you talk about a Form instance that is built in a specific way. Renaming RegisterType to RegisterForm would imply that you are dealing with a RegisterForm instance (like in sf1 or ZF) which is wrong and leads to confusion.

@webmozart
Symfony member

@Seldaek: I can already see the questions coming: "I have added method getX() to RegisterForm but cannot access it!! Why??"

@Seldaek
Symfony member
Seldaek commented Apr 6, 2011

@bschussek: If one does $form = $factory->create(new RegisterForm()) and then expects $form to be his RegisterForm, well I think they have issues. On the other hand we could say that maybe the factory should inject stuff in RegisterForm (handled by the parent class) and then return the form object, then your $form would actually be a form class + your method.

@fabpot
Symfony member
fabpot commented Apr 6, 2011

@bschussek: I think you should explain once more what these type classes are really; just to be sure everybody know what we are talking about. It took me quite some time to figure out their usage and why they are not Forms or Fields. "Config" looks the only possible suffix to me amongst your proposals.

@schmittjoh
@brikou
brikou commented Apr 6, 2011

if we need to keep an abstract "notion" let's say structure + 1

@inanimatt

This is getting a bit too much like bike-shedding. Type and Config are vague and laden with potentially faulty expectations. Skeleton's kinda creepy-sounding. Recipe is twee and unnecessarily metaphorical. Structure is bland and vague. Form & Field are potentially misleading.

They're all fine. People will get confused whatever you call it, because forms aren't as simple as they want to believe, and languages are ambiguous (especially English, especially especially technical English). That's okay, we have docs for that.

So I say that @fabpot (or @bschussek) should just pick any name (as long as it's not something like Fred or Mamlet) and let's move on. :)

@datiecher

I do agree with @inanimatt. Whatever option we decide to use here will lead to confusion.

Coming from a Portuguese speaking person, I would prefer to use @schmittjoh second suggestion over the others posted here but, to me, I can live with all of the suggestions made here, besides recipe - even though it would be cool. maybe for Silex, eh?

@henrikbjorn

I think this introduced a bug when you have a choice field that are expanded but not multiple will trigger the form to have extra data. This is what print_r printed out for $form->getExtraData();

array (
    [] = 1,
)
@joelwurtz

I agree too, all options will lead to confusion, so we have to use an uncommon prefix so the user we wonder what he is using.

In my point of view "Type" is for Form or Field like XSL (or XSD) is for XML

So we can have FSL (or FSD) for Field (or Form) Structure Language (or Field (or Form) Structure Definition)
Or just Definition

@fabpot
Symfony member
fabpot commented Apr 12, 2011

Let's focus on reviewing the patch and see what needs to be worked on before merging. Type is really just fine.

@fabpot
Symfony member
fabpot commented Apr 12, 2011

Looks like I forgot to add a note here about my work on removing the Renderer sub-framework. Basically, I've removed the Renderer/ directory and re-implemented the current way of dealing with templates:

https://github.com/fabpot/symfony/tree/form

@Richtermeister

Inspired by Brouznouf, would "Definition" be such a bad term? RegisterFormDefinition, InputFieldDefinition.. stuff like that?
$form = $factory->create(new RegisterFormDefinition()) looks pretty intuitive..

@henrikbjorn

@fabpot as mentioned on twitter. Would it be a possibility to move the createTemplateContext() method to FormInterface and have it named getContext() or getTemplateContext(). The change would be better in such a way that factory dosent need to be saved in a variable to be reused.

@fabpot fabpot [Form] moved the template context creation to the Form class
Moving the template context creation makes sense and allows for simpler code for the end user:

Before:

        return array('post' => $post, 'form' => $this->get('form.factory')->createTemplateContext($form));

After:

        return array('post' => $post, 'form' => $form->getContext());
49dc836
@fabpot
Symfony member
fabpot commented Apr 13, 2011

@henrikbjorn: This totally makes sense and simplifies things a lot for the end user: fabpot@49dc836

@henrikbjorn

Awesome!

@fabpot
Symfony member
fabpot commented Apr 14, 2011

Closing this PR as the code now lives at symfony/form and will be merged probably next week.

@fabpot fabpot closed this Apr 14, 2011
@stloyd

Hmm, this should be replaced with something like:

$builder->add((string)$value, $options['multiple'] ? 'checkbox' : 'radio', array('value' => $choice));

As giving an array of choices:

$builder->add('package_id', 'choice', array(
        'choices' => array(
            '1' => 'Basic',
            '2' => 'Pro'
        ),
//      'expanded' => true
))

Will generate invalid labels (it use keys instead of values).

@weaverryan

I realize this code is a little out-of-date, but my comment is on the spirit of this code.

Is setting the max_length option here necessary? Since the parent type is field, isn't the max_length already set in buildView() of FieldType?

Symfony member

You're absolutely right. Fixed in c58b05a

@docteurklein

I think this can be terribly bad for flexibility. Couldn't we externalize instanciation in order to keep control ?
Or at least provide class name as an argument ?

Symfony member

There are a lot of extension points in the form framework - which is why this isn't one. If you really want to change the class (which shouldn't be necessary in 99% of cases) you can override this method.

@vicb

Isn't there something wrong with this test (it should always returns true)

Symfony member

The whole renderer sub-namespace has never been part of symfony/symfony.

vicb replied May 6, 2011

Ooops should be Form\FormView !

@jpb0104

I feel like the logic here is a little ambiguous. I see a situation where a user might have a choice list with choices set as an empty array and required set to false? This would trigger this exception.
Suggestion:

if (!isset($options['choices']) && !isset($options['choice_list']) && $options['required'] === true) {
    throw new FormException('Either the option "choices" or "choice_list" is required');
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.