[Form] Merged FieldType into FormType #3923

webmozart · 2012-04-13T14:15:30Z

Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3878
Todo: update the documentation on theming

This PR is a preparatory PR for #3879. See also #3878.

juliendidier · 2012-04-13T14:25:19Z

What's the benefit ?

henrikbjorn · 2012-04-13T14:26:40Z

why input_widget ? and not just widget

Burgov · 2012-04-13T14:27:49Z

@juliendidier dynamic inheritance is now obsolete which fixes some other issues

stof · 2012-04-13T14:36:42Z

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig

 {% endspaceless %}
 {% endblock form_widget %}

+{% block input_widget %}


this will conflict if someone creates a type named input. you should not use a block named *_widget as this matches the convention for blocks of the theme

Ok, thanks for the remark!

stloyd · 2012-04-13T14:37:26Z

What about not breaking API so badly and leaving FieldType which will be simple like (with marking as deprecated):

<?php

class FieldType extends AbstractType
{
    public function getParent(array $options)
    {
        return 'form';
    }

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

webmozart · 2012-04-13T14:43:41Z

@stloyd That's a very good idea.

stof · 2012-04-13T14:45:31Z

src/Symfony/Component/Form/Form.php

+        // NULL is the default meaning:
+        // bubble up if the form has children (complex forms)
+        // don't bubble up if the form has no children (primitive fields)
+        $this->errorBubbling = null === $errorBubbling ? null : (Boolean) $errorBubbling;


@bschussek what was the reason to bubble the error for complex forms ? People complained about this regularly because they don't understand why the error appears at the root

If we disable error bubbling on complex forms, errors will end up somewhere inside the form, but not associated to any specific field. Would that be better?

mvrhov · 2012-04-13T17:41:21Z

IMHO what @stloyd proposed sounds like a good idea, but removing FieldType class, if #3903 will come into life might ensure that more forms will broke and people will check them thoroughly.

attilabukor · 2012-04-13T18:46:08Z

@bschussek looks great, but I'm concerned about how quickly will the third-party bundles adapt to this BC break. I hope really quick, because if they don't the whole stuff will be useless :S of course it's not your problem to solve.

stof · 2012-04-13T18:50:32Z

@r1pp3rj4ck there is already another BC break requiring to update custom types for Symfony master. So third party bundles already have to do some work.

attilabukor · 2012-04-13T18:59:37Z

@stof which one? I've looked into @bschussek 's RFC about these [foo].bar stuff, but it's not yet implemented. Are you refering to this or another one I've missed?

stof · 2012-04-13T19:04:06Z

@r1pp3rj4ck the change regarding default options

attilabukor · 2012-04-13T19:06:10Z

@stof oh, I forgot that one. Weird thing is that I've already changed my default options today and still forgetting these stuff :D

webmozart · 2012-04-14T08:58:29Z

I restored and deprecated FieldType now. I'd appreciate further reviews.

stloyd · 2012-04-14T09:02:32Z

Maybe we should try to avoid this BC in templates ? What do you think about similar move like with FieldType ? (hold old, but inside just render new)

webmozart · 2012-04-14T09:07:22Z

@stloyd You mean for those cases where people explicitely render the block "field_*"? We can do that.

stloyd · 2012-04-14T09:09:45Z

@bschussek Yes I mean this case =) Sorry for not being explicit, I need some coffee I think =)

jalliot · 2012-04-14T09:27:28Z

src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php

@@ -182,7 +182,7 @@ public function getDefaultOptions()
     */
    public function getParent(array $options)
    {
-        return isset($options['expanded']) && $options['expanded'] ? 'form' : 'field';
+        return 'field';


Don't you mean form instead? Idem below in other types.

Btw, you can remove the method entirely as it can be inherited from AbstractType

We need to keep this code for BC.

@bschussek Keeping the FieldType should be enough for BC, no? We don't need to extend it for built-in types.

If we don't extend it, custom themes are broken.

CSRF fields are now only added when the view is built. For this reason we already know if the form is the root form and avoid to create unnecessary CSRF fields for nested fields.

webmozart · 2012-04-17T14:45:35Z

I added the field_* blocks again for BC. Could someone please review again? Otherwise this can be merged.

Burgov · 2012-04-17T15:11:16Z

@bschussek I'm not sure what has changed to cause this, but if I try out your branch on our forms, if I leave the value of an input empty, eventually the reverseTransform method receives a null value, rather than a '' (empty string) value, as on the current symfony master.

DateTimeToLocalizedStringTransformer, for example, will throw an Exception if the value is not a string

if (!is_string($value)) {
   throw new UnexpectedTypeException($value, 'string');
}

Other than that, all forms render just the same as they do on symfony master

Burgov · 2012-04-17T15:18:21Z

src/Symfony/Component/Form/Extension/Core/Type/FormType.php

+                }
+            };
+
+            return '';


In regard to my previous comment - this line is never reached. I suppose the if on line 179 has to be placed before the return on line 178

Better yet, it should read:

$emptyData = function (Options $options) { $class = $options['data_class']; if (null !== $class) { return function (FormInterface $form) use ($class) { if ($form->isEmpty() && !$form->isRequired()) { return null; } return new $class(); }; } return function (FormInterface $form) { if ($form->hasChildren()) { return array(); } else { return ''; } }; };

Well spotted, thanks! Seems like a test case is missing for this case.

…ing anymore

webmozart · 2012-04-17T15:30:29Z

@Burgov Fixed.

@juliendidier

Commits ------- 6e4ed9e [Form] Fixed regression: bind(null) was not converted to an empty string anymore fcb2227 [Form] Deprecated FieldType, which has been merged into FormType bfa7ef2 [Form] Removed obsolete exceptions 2a49449 [Form] Simplified CSRF mechanism and removed "csrf" type Discussion ---------- [Form] Merged FieldType into FormType Bug fix: no Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: #3878 Todo: update the documentation on theming ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3878) This PR is a preparatory PR for #3879. See also #3878. --------------------------------------------------------------------------- by juliendidier at 2012-04-13T14:25:19Z What's the benefit ? --------------------------------------------------------------------------- by henrikbjorn at 2012-04-13T14:26:40Z why `input_widget` ? and not just `widget` --------------------------------------------------------------------------- by Burgov at 2012-04-13T14:27:49Z @juliendidier dynamic inheritance is now obsolete which fixes some other issues --------------------------------------------------------------------------- by stloyd at 2012-04-13T14:37:26Z What about __not__ breaking API so *badly* and leaving `FieldType` which will be simple like (with marking as deprecated): ```php <?php class FieldType extends AbstractType { public function getParent(array $options) { return 'form'; } public function getName() { return 'field'; } } --------------------------------------------------------------------------- by bschussek at 2012-04-13T14:43:41Z @stloyd That's a very good idea. --------------------------------------------------------------------------- by mvrhov at 2012-04-13T17:41:21Z IMHO what @stloyd proposed sounds like a good idea, but removing FieldType class, if #3903 will come into life might ensure that more forms will broke and people will check them thoroughly. --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T18:46:08Z @bschussek looks great, but I'm concerned about how quickly will the third-party bundles adapt to this BC break. I hope really quick, because if they don't the whole stuff will be useless :S of course it's not your problem to solve. --------------------------------------------------------------------------- by stof at 2012-04-13T18:50:32Z @r1pp3rj4ck there is already another BC break requiring to update custom types for Symfony master. So third party bundles already have to do some work. --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T18:59:37Z @stof which one? I've looked into @bschussek 's RFC about these [foo].bar stuff, but it's not yet implemented. Are you refering to this or another one I've missed? --------------------------------------------------------------------------- by stof at 2012-04-13T19:04:06Z @r1pp3rj4ck the change regarding default options --------------------------------------------------------------------------- by r1pp3rj4ck at 2012-04-13T19:06:10Z @stof oh, I forgot that one. Weird thing is that I've already changed my default options today and still forgetting these stuff :D --------------------------------------------------------------------------- by bschussek at 2012-04-14T08:58:29Z I restored and deprecated FieldType now. I'd appreciate further reviews. --------------------------------------------------------------------------- by stloyd at 2012-04-14T09:02:32Z Maybe we should try to avoid this BC in templates ? What do you think about similar move like with `FieldType` ? (hold old, but inside just render new) --------------------------------------------------------------------------- by bschussek at 2012-04-14T09:07:22Z @stloyd You mean for those cases where people explicitely render the block "field_*"? We can do that. --------------------------------------------------------------------------- by stloyd at 2012-04-14T09:09:45Z @bschussek Yes I mean this case =) Sorry for not being explicit, I need some coffee I think =) --------------------------------------------------------------------------- by bschussek at 2012-04-17T14:45:35Z I added the field_* blocks again for BC. Could someone please review again? Otherwise this can be merged. --------------------------------------------------------------------------- by Burgov at 2012-04-17T15:11:16Z @bschussek I'm not sure what has changed to cause this, but if I try out your branch on our forms, if I leave the value of an input empty, eventually the reverseTransform method receives a null value, rather than a '' (empty string) value, as on the current symfony master. DateTimeToLocalizedStringTransformer, for example, will throw an Exception if the value is not a string ```php if (!is_string($value)) { throw new UnexpectedTypeException($value, 'string'); } ``` Other than that, all forms render just the same as they do on symfony master --------------------------------------------------------------------------- by bschussek at 2012-04-17T15:30:29Z @Burgov Fixed.

vicb · 2012-04-18T17:06:06Z

src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig

-        {{ block('field_rows') }}
-        {{ form_rest(form) }}
-    </div>
+    {% if form.children|length > 0 %}


@bschussek will it be ok with empty collections ?

@vicb yes, as |length uses count() and collection are countable

the question is if this might render an input (count() returns 0)

ah, you mean for the collection type with an empty collection ?

This reverts commit 85bb439, reversing changes made to 962f975. Conflicts: CHANGELOG-2.1.md UPGRADE-2.1.md src/Symfony/Component/Form/Extension/Core/Type/FormType.php src/Symfony/Component/Form/Tests/FormFactoryTest.php s3923

stof reviewed Apr 13, 2012
View reviewed changes

jalliot reviewed Apr 14, 2012
View reviewed changes

webmozart added 3 commits April 17, 2012 16:44

[Form] Simplified CSRF mechanism and removed "csrf" type

2a49449

CSRF fields are now only added when the view is built. For this reason we already know if the form is the root form and avoid to create unnecessary CSRF fields for nested fields.

[Form] Removed obsolete exceptions

bfa7ef2

[Form] Deprecated FieldType, which has been merged into FormType

fcb2227

Burgov reviewed Apr 17, 2012
View reviewed changes

[Form] Fixed regression: bind(null) was not converted to an empty str…

6e4ed9e

…ing anymore

fabpot merged commit 6e4ed9e into symfony:master Apr 18, 2012

vicb reviewed Apr 18, 2012
View reviewed changes

Uh oh!

[Form] Merged FieldType into FormType #3923

[Form] Merged FieldType into FormType #3923

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!