[Form] Fixed errors introduced in the FieldType+FormType merge #4046

webmozart · 2012-04-20T17:07:55Z

Bug fix: yes
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: #3994, #4000, #2294, #4118
Todo: -

Tobion · 2012-04-22T15:07:58Z

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

+{% block form_widget_primitive %}
+{% spaceless %}
+    {% set type = type|default('text') %}
+    <input type="{{ type }}" {{ block('widget_attributes') }} {% if value is not empty %}value="{{ value }}" {% endif %}/>


You can inline the type with default here so you don't need to set it in the previous line.

type can be passed into the block.

You mean it could be used in a block called from within form_widget_primitive? Because at the moment its not used anywhere inside.
I simply mean <input type="{{ type|default('text') }}"

Ah yes, you're right.

Hm, I'd rather leave it like that, in case someone tries to access the type in "widget_attributes".

Tobion · 2012-04-22T15:39:20Z

primitive is a pretty abstract option name. It depends on the person what he considers primitive. Maybe more explicit naming or better documentation what it means.

webmozart · 2012-04-22T15:47:29Z

Better suggestions?

The distinction here is between primitive and complex forms, where primitive forms are such forms that can be represented by a single HTML tag. This obviously needs to be documented.

Tobion · 2012-04-22T15:49:45Z

Maybe single_widget or something like that.

vicb · 2012-04-23T13:01:54Z

src/Symfony/Component/Form/FormView.php

@bschussek what the purpose of modifying this file (seems unused) ?

You are right, this was a mistake.

vicb · 2012-04-23T13:09:43Z

@Tobion @bschussek would elementary be better than primitive ?

vicb · 2012-04-23T13:17:04Z

and compound \ composite better than complex ?

webmozart · 2012-04-23T14:08:33Z

@vicb I fail to see how elementary/compound is easier to understand than primitive/complex. Maybe single_widget, but what's the opposite of this case? multi_widget?

vicb · 2012-04-23T14:15:09Z

Actually I am fine with anything... as long as it is documented.

webmozart · 2012-04-23T14:22:31Z

Still I think that this unveals a more profound naming problem. How do we (also in the documentation) name forms with children (formerly "forms") and forms without children (formerly "fields")?

Should we refer to them as

forms and fields?
complex and primitive forms?
...

We must first answer this question before we can find an intuitive option name. If the documentation always switches between different terminologies, neither will it be understandable nor will this option be easy to remember.

vicb · 2012-04-23T15:10:32Z

Still I think that this unveals a more profound naming problem. How do we (also in the documentation) name forms with children (formerly "forms") and forms without children (formerly "fields")?

To make it clear, I would rather say forms that can have children and forms that can not have children (i.e. Empty collections have no children but they can have and this is reason why you have to introduce those options, right ? - that could be a good example for the doc).

It will probably be better to refer to "complex" / "primitive" forms in the doc (and use the "form" / "field" terms to explain them).

Note: I think @Tobion concern is that "primitive" / "complex" could be pejorative terms (this is why I have proposed "elementary" / "compound").

Tobion · 2012-04-23T16:00:54Z

primitive/complex is subjective (and could be pejorative too)
elementary/compound is more explicit so probably better than primitive/complex
I dislike this option in general. Does it make sense to change this option from a user perspective? I guess it's always the same as long as the widget structure stays the same. So it should be resolved at a higher level dynamically from the widget structure and not exposed to any configuration.
In documentation I would use the terms forms and fields. Because all people with HTML knowledge will understand that fields cannot have sub-fields whereas forms can. But since this distinction is not findable in code, it should be mentioned that all these are implemented as a form hierarchy.

mvrhov · 2012-04-23T16:02:00Z

how about simple and complex?

webmozart · 2012-04-23T16:06:33Z

@Tobion It does not make sense to change this option from the user perspective, still the overloading type has to propagate to FormType whether it is a form or a field, so that the default behaviour is correct.

A second option how to implement this is to add a method isField to FormTypeInterface that can be overloaded and receives the options. I don't really like to introduce new methods here unless absolutely required.

What about renaming the option "primitive" to "is_field"? The blocks in the template would then be named "form_widget_field" and "form_widget_form".

tristanbes · 2012-04-25T14:01:06Z

Oh, I should've seen this before, i thought I was doing something wrong. (empty collections gets an input field bug)

Please big :UP: on this. When will it be merged ? @bschussek

Tobion · 2012-04-25T15:30:28Z

+1 for "is_field" and "form_widget_field" but I would rather use "form_widget_compound" instead of "form_widget_form" which is quite strange.

webmozart · 2012-04-26T16:34:04Z

@Tobion "simple" and "compound" then?

Tobion · 2012-04-26T16:49:58Z

no "field" and "compound"

webmozart · 2012-04-26T17:17:02Z

I don't like "field" for a simple reason: Consider the "date" type. We are typically speaking of the "date" field there. But technically, the "date" field is a compound field. So?

Tobion · 2012-04-26T21:17:37Z

I don't understand the open question. You proposed "is_field" and "form_widget_field" yourself. So calling the template block "form_widget_field" is a comprehensible consequence of "is_field". I wouldn't call the date type with multiple inputs a field.

tristanbes · 2012-04-26T21:52:39Z

We should take a decision cause right here i got all my forms that are broken because of the empty collection rendering as input field :-).

I guess we are many in that situation.

… and that needs to be adapted very often

… Label attributes are now passed in "label_attr"

…the 'single_control' option

webmozart · 2012-04-27T08:28:16Z

I renamed "primitive" to "single_control" now to match with the HTML specification which names all input elements (input, select etc.) "controls". The opposite is now "compound".

Meanwhile, I added a fix for #4118.

@fabpot This is ready for merge now.

Tobion · 2012-04-27T10:22:49Z

Hm, I know naming things is hard and sometimes not really important. But since users need to know which block to override, it is essential to make it clear. I think there is still one issue.
The block is named form_widget_single_control in order, as you said, to abstract away if it's an input, select etc. But in fact it can only render input and nothing else. So this is misleading.
So you could also simply name it form_widget_input.
Apart from that I agree with everything.

Tobion · 2012-04-27T10:34:04Z

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

@@ -174,6 +178,7 @@ public function getDefaultOptions()
            'empty_data'        => $emptyData,
            'empty_value'       => $emptyValue,
            'error_bubbling'    => false,
+            'single_control'         => $singleControl,


indention wrong

@Tobion

Commits ------- 246c885 [Form] Fixed: Default value of 'error_bubbling' is now determined by the 'single_control' option d3bb4d0 [Form] Renamed option 'primitive' to 'single_control' 167e64f [Form] Fixed: Field attributes are not rendered in the label anymore. Label attributes are now passed in "label_attr" 68018a1 [Form] Dropped useless test that is guaranteed by OptionsParser tests and that needs to be adapted very often 649752c [Form] Fixed: CSRF token was not displayed on empty complex forms c623fcf [Form] Fixed: CSRF protection did not run if token was missing eb75ab1 [Form] Fixed results of the FieldType+FormType merge. Discussion ---------- [Form] Fixed errors introduced in the FieldType+FormType merge Bug fix: yes Feature addition: no Backwards compatibility break: yes Symfony2 tests pass: yes Fixes the following tickets: #3994, #4000, #2294, #4118 Todo: - ![Travis Build Status](https://secure.travis-ci.org/bschussek/symfony.png?branch=issue3994) --------------------------------------------------------------------------- by Tobion at 2012-04-22T15:39:20Z `primitive` is a pretty abstract option name. It depends on the person what he considers primitive. Maybe more explicit naming or better documentation what it means. --------------------------------------------------------------------------- by bschussek at 2012-04-22T15:47:29Z Better suggestions? The distinction here is between primitive and complex forms, where primitive forms are such forms that can be represented by a single HTML tag. This obviously needs to be documented. --------------------------------------------------------------------------- by Tobion at 2012-04-22T15:49:45Z Maybe `single_widget` or something like that. --------------------------------------------------------------------------- by vicb at 2012-04-23T13:09:43Z @Tobion @bschussek would `elementary` be better than `primitive` ? --------------------------------------------------------------------------- by vicb at 2012-04-23T13:17:04Z and `compound \ composite` better than `complex` ? --------------------------------------------------------------------------- by bschussek at 2012-04-23T14:08:33Z @vicb I fail to see how elementary/compound is easier to understand than primitive/complex. Maybe single_widget, but what's the opposite of this case? multi_widget? --------------------------------------------------------------------------- by vicb at 2012-04-23T14:15:09Z Actually I am fine with anything... as long as it is documented. --------------------------------------------------------------------------- by bschussek at 2012-04-23T14:22:31Z Still I think that this unveals a more profound naming problem. How do we (also in the documentation) name forms with children (formerly "forms") and forms without children (formerly "fields")? Should we refer to them as * forms and fields? * complex and primitive forms? * ... We must first answer this question before we can find an intuitive option name. If the documentation always switches between different terminologies, neither will it be understandable nor will this option be easy to remember. --------------------------------------------------------------------------- by vicb at 2012-04-23T15:10:32Z > Still I think that this unveals a more profound naming problem. How do we (also in the documentation) name forms with children (formerly "forms") and forms without children (formerly "fields")? To make it clear, I would rather say forms that **can have** children and forms that **can not have** children (i.e. Empty collections have no children but they can have and this is reason why you have to introduce those options, right ? - that could be a good example for the doc). It will probably be better to refer to "complex" / "primitive" forms in the doc (and use the "form" / "field" terms to explain them). Note: I think @Tobion concern is that "primitive" / "complex" could be pejorative terms (this is why I have proposed "elementary" / "compound"). --------------------------------------------------------------------------- by Tobion at 2012-04-23T16:00:54Z 1. primitive/complex is subjective (and could be pejorative too) 2. elementary/compound is more explicit so probably better than primitive/complex 3. I dislike this option in general. Does it make sense to change this option from a user perspective? I guess it's always the same as long as the widget structure stays the same. So it should be resolved at a higher level dynamically from the widget structure and not exposed to any configuration. 4. In documentation I would use the terms forms and fields. Because all people with HTML knowledge will understand that fields cannot have sub-fields whereas forms can. But since this distinction is not findable in code, it should be mentioned that all these are implemented as a form hierarchy. --------------------------------------------------------------------------- by mvrhov at 2012-04-23T16:02:00Z how about simple and complex? --------------------------------------------------------------------------- by bschussek at 2012-04-23T16:06:33Z @Tobion It does not make sense to change this option from the user perspective, still the overloading type has to propagate to FormType whether it is a form or a field, so that the default behaviour is correct. A second option how to implement this is to add a method `isField` to FormTypeInterface that can be overloaded and receives the options. I don't really like to introduce new methods here unless absolutely required. What about renaming the option "primitive" to "is_field"? The blocks in the template would then be named "form_widget_field" and "form_widget_form". --------------------------------------------------------------------------- by tristanbes at 2012-04-25T14:01:06Z Oh, I should've seen this before, i thought I was doing something wrong. (empty collections gets an input field bug) Please big :UP: on this. When will it be merged ? @bschussek --------------------------------------------------------------------------- by Tobion at 2012-04-25T15:30:28Z +1 for "is_field" and "form_widget_field" but I would rather use "form_widget_compound" instead of "form_widget_form" which is quite strange. --------------------------------------------------------------------------- by bschussek at 2012-04-26T16:34:04Z @Tobion "simple" and "compound" then? --------------------------------------------------------------------------- by Tobion at 2012-04-26T16:49:58Z no "field" and "compound" --------------------------------------------------------------------------- by bschussek at 2012-04-26T17:17:02Z I don't like "field" for a simple reason: Consider the "date" type. We are typically speaking of the "date" field there. But technically, the "date" field is a compound field. So? --------------------------------------------------------------------------- by Tobion at 2012-04-26T21:17:37Z I don't understand the open question. You proposed "is_field" and "form_widget_field" yourself. So calling the template block "form_widget_field" is a comprehensible consequence of "is_field". I wouldn't call the date type with multiple inputs a field. --------------------------------------------------------------------------- by tristanbes at 2012-04-26T21:52:39Z We should take a decision cause right here i got all my forms that are broken because of the empty collection rendering as input field :-). I guess we are many in that situation. --------------------------------------------------------------------------- by bschussek at 2012-04-27T08:28:16Z I renamed "primitive" to "single_control" now to match with the HTML specification which names all input elements (input, select etc.) "controls". The opposite is now "compound". Meanwhile, I added a fix for #4118. @fabpot This is ready for merge now. --------------------------------------------------------------------------- by Tobion at 2012-04-27T10:22:49Z Hm, I know naming things is hard and sometimes not really important. But since users need to know which block to override, it is essential to make it clear. I think there is still one issue. The block is named `form_widget_single_control` in order, as you said, to abstract away if it's an input, select etc. But in fact it can only render `input` and nothing else. So this is misleading. So you could also simply name it `form_widget_input`. Apart from that I agree with everything.

Tobion · 2012-05-02T14:52:47Z

@bschussek do you disagree with my arguement about form_widget_single_control?

webmozart · 2012-05-02T16:13:49Z

@Tobion I disagree in that the form_widget_single_control block doesn't necessarily have to render as input (conceptually). You could also define that every form with single_control=true is rendered as checkbox by default if you like this.

Tobion · 2012-05-02T16:38:39Z

My point is that the block has a generic name but is not generically implemented. So by default it can only be used for inputs. Conceptually I would think, according to its name, it looks like this:

{% block form_widget_single_control %}  
{% spaceless %} 
    {% set type = type|default('text') %}
    {% if 'select' == type %}   
        {{ block('choice_widget_collapsed') }}
    {% else %}  
        <input type="{{ type }}" {{ block('widget_attributes') }} {% if value is not empty %}value="{{ value }}" {% endif %}/>
    {% endif %}
{% endspaceless %}
{% endblock form_widget_single_control %}

That's why I find the current naming misleading.

Tobion reviewed Apr 22, 2012
View reviewed changes

vicb reviewed Apr 23, 2012
View reviewed changes

webmozart mentioned this pull request Apr 24, 2012

[3.0] [Form] Ensure that the input of basic fields is a string #4102

Closed

webmozart mentioned this pull request Apr 26, 2012

[Form] errors in form children aren't rendered with form_rest #4118

Closed

webmozart added 7 commits April 27, 2012 09:47

[Form] Fixed results of the FieldType+FormType merge.

eb75ab1

[Form] Fixed: CSRF protection did not run if token was missing

c623fcf

[Form] Fixed: CSRF token was not displayed on empty complex forms

649752c

[Form] Dropped useless test that is guaranteed by OptionsParser tests…

68018a1

… and that needs to be adapted very often

[Form] Fixed: Field attributes are not rendered in the label anymore.…

167e64f

… Label attributes are now passed in "label_attr"

[Form] Renamed option 'primitive' to 'single_control'

d3bb4d0

[Form] Fixed: Default value of 'error_bubbling' is now determined by …

246c885

…the 'single_control' option

Tobion reviewed Apr 27, 2012
View reviewed changes

fabpot merged commit 246c885 into symfony:master Apr 27, 2012

Uh oh!

[Form] Fixed errors introduced in the FieldType+FormType merge #4046

[Form] Fixed errors introduced in the FieldType+FormType merge #4046

Uh oh!

Conversation

webmozart commented Apr 20, 2012

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!

Choose a reason for hiding this comment

Uh oh!

Tobion commented Apr 22, 2012

Uh oh!

webmozart commented Apr 22, 2012

Uh oh!

Tobion commented Apr 22, 2012

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!

vicb commented Apr 23, 2012

Uh oh!

vicb commented Apr 23, 2012

Uh oh!

webmozart commented Apr 23, 2012

Uh oh!

vicb commented Apr 23, 2012

Uh oh!

webmozart commented Apr 23, 2012

Uh oh!

vicb commented Apr 23, 2012

Uh oh!

Tobion commented Apr 23, 2012

Uh oh!

mvrhov commented Apr 23, 2012

Uh oh!

webmozart commented Apr 23, 2012

Uh oh!

tristanbes commented Apr 25, 2012

Uh oh!

Tobion commented Apr 25, 2012

Uh oh!

webmozart commented Apr 26, 2012

Uh oh!

Tobion commented Apr 26, 2012

Uh oh!

webmozart commented Apr 26, 2012

Uh oh!

Tobion commented Apr 26, 2012

Uh oh!

tristanbes commented Apr 26, 2012

Uh oh!

webmozart commented Apr 27, 2012

Uh oh!

Tobion commented Apr 27, 2012

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tobion commented May 2, 2012

Uh oh!

webmozart commented May 2, 2012

Uh oh!

Tobion commented May 2, 2012

Uh oh!

Uh oh!