[WIP] [3.2] [Form] add 'empty_view' option in FormType #17609

HeahDude · 2016-01-30T10:25:59Z

Q	A
Bug fix?	yes
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#17579, #18804
License	MIT
Doc PR	todo

The empty_view option can be a string, or a boolean. By default it is 'No field' when compound and false otherwise. It uses the translation_domain option for its translation.

CollectionType inherits it but this can be avoid be setting empty_view to false.

The empty_view is defined in FormType::finishView() and overriden in ChoiceType::finishView() to be defined when choices and preferred_choices view vars are empty.

In the templates, the old behaviour is kept explicitly set to false.

So 2 ways of using it:

passing a translatable simple string to custom a message instead of an empty div or an empty select input :

// src/AppBundle/Form/Type/TenantType.php
// ...
$builder->add('cars', \Symfony\Bridge\Doctrine\Form\Type\EntityType::class, array(
    'class' => \AppBundle\Entity\Cars::class,
    'empty_view' => 'No cars available', // will be escaped
    // ...
));

overriding the empty_row to use custom html :

{# 'default/rent_a_car.html.twig #}
{# ... #}
{% form_theme _self %}
{% block 'empty_row' %}
    {# custom html with access to 'full_name', 'attr', and 'empty_view' view vars #}
{% endblock %}
{{ form(form) }}

What do you think ?

xabbuh · 2016-02-03T16:40:04Z

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

@@ -165,21 +165,25 @@
 {%- endblock choice_widget_collapsed %}

 {% block choice_widget_expanded -%}
-    {% if '-inline' in label_attr.class|default('') %}
+    {% if '-inline' in label_attr.class|default('') -%}


looks like the whitespace control changes are unrelated to this feature and should be done in a different PR

@xabbuh I did this to match bootstrap layout. DYTH that it needs a PR ?

Well, I think there's now use in having this different in 2.8/3.0.

@xabbuh I don't understand your comment.

@HeahDude it's not related to this PR so should be done in a separate one. It should also be done on the latest branch this layout is available, to ease future merging.

@jakzal ok reverted. foundation_5_layout was introduced on 2.8 should I make a PR on this branch or on master ? Do you think it's really needed ?

xabbuh · 2016-02-03T16:42:22Z

Status: Needs work

HeahDude · 2016-02-03T18:33:18Z

@xabbuh thanks for the review.

This PR is a POF to see if symfony deciders agree with this way of fixing #17579, I added your suggestion in the todo list of the description.

backbone87 · 2016-02-08T13:17:51Z

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

@@ -219,6 +219,12 @@ public function buildView(FormView $view, FormInterface $form, array $options)
            // POST request.
            $view->vars['full_name'] .= '[]';
        }
+
+        // Provide a default message when choices var is empty
+        if (empty($view->vars['choices']) && $options['expanded']) {


I would remove this if completely. Its up to the template to decided what to display and when.
At least I would remove the check for expanded since one could decide to display a message insteadof an empty select box.

@backbone87 👍 for removing the expanded condition.

But I don't think it's worth init a view var empty_choices and add it to the context if choices are not empty.

xabbuh · 2016-02-17T16:58:33Z

src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/choice_widget.html.php

 <?php if ($expanded): ?>
 <?php echo $view['form']->block($form, 'choice_widget_expanded') ?>
 <?php else: ?>
 <?php echo $view['form']->block($form, 'choice_widget_collapsed') ?>
 <?php endif ?>
+<?php else: ?>
+<?php if ('' === $empty_choices) {


$empty_choices isn't set here (otherwise the block above would have been executed).

right, the condition on the first line is wrong, it should be :
+<?php if (!isset($empty_choices)): ?>

@xabbuh thanks, fixed.

HeahDude · 2016-02-18T10:26:10Z

Work is still in progress, but I've updated the description and refactored a bit in the last commit.

Status: Needs Review

HeahDude · 2016-02-22T16:10:30Z

I've got a suggestion as there is here an addition of a new default translation domain, may be it would be nice to translate the 'None' default placeholder too (see https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php#L283).

Do you agree with this handling ?
If so, may I add this in that PR (in a separated commit) or send another one if this one gets merged ?

backbone87 · 2016-02-22T16:48:39Z

src/Symfony/Component/Form/Resources/translations/sf_form.en.xlf

+        <body>
+            <trans-unit id="1">
+                <source>No choice available</source>
+                <target><em>No choice available.</em></target>


Either the em-tag should be put in the correct xmlns or the whole content of target should be marked CDATA

@backbone87, thanks for the review, I've just copy-pasted validators.en.xliff as I'm not familiarized with this format, could you please precise ?

its nothing specific to xliff. its about how xml namespaces work. when an xml parser parses the given xliff xml file, it interprets the em-element to be part of the xliff namespace (which is declared as the "default" namespace in the given document with xmlns="urn:oasis:names:tc:xliff:document:1.2"). This isnt a problem, unless the xliff consumer doesnt know what todo with an em-element. But xsd validation against the xliff schema will most likely fail (because it probably didnt define an em-element).

One way is to declare the xhtml namespace in the document ´xmlns:xh="http://www.w3.org/1999/xhtml"` and qualify the em-element against it: <xh:em>...</xh:em>. http://www.w3.org/1999/xhtml is used for xhtml 1.x and xml serialisation of html5 (xhtml5).

But since we dont care for the structure of the content in this use case, because the translation consumer expects text/plain produced by xliff, we can just properly escape the content of the target-element. either with <target>No choice available.</target> or (more readable) <target><![CDATA[No choice available.]]></target> (cdata = character data)

@backbone87 Thank you very much for this explanation :)

webmozart · 2016-03-14T08:29:03Z

I think it's a good idea to add this functionality in general. When choices are loaded from a data source and no choice is available, displaying some text instead seems like the proper thing to do.

I'm not happy with the naming though. "empty", in the sense of a choice field, refers to the "empty choice", i.e. the choice that is selected if no choice is selected. Using the same terminology creates ambiguity and potentially confuses our users. Unfortunately I can't think of a better name at the moment.

HeahDude · 2016-03-14T08:37:10Z

@webmozart, I proposed empty_form if this should be a global option for compound form types which would be inherited by ChoiceType (and CollectionType) in #17579 (comment).

We could add a default normalizer at the root definition for more safety to no use it when compound is false, but depending on the implementation it might just be ignored in that case.

webmozart · 2016-03-25T09:37:07Z

Even then empty is a problematic term. "Empty" - so far - refers to the value of a field. A field/form is empty if that value is null or ''.

You are introducing a different concept of "empty", namely that a form is empty if doesn't contain children. Now it could happen that an empty form (your definition: no children) is not empty (existing definition: data is not null). Do you see how this can be ambiguous and confusing?

I think we need to find another name that does not involve the word "empty".

HeahDude · 2016-03-25T12:16:09Z

@webmozart, I've kept working in this, and did not use that idea but another one also with "empty"...

While implementing it globally, it felt more natural to use empty_view because actually it's the compound container block (HTML tag) which is empty and I found it makes a good echo to empty_data while keeping it clear that the value must be a string.

What about no_child_view or empty_compound ?

I just need to fix some tests with table layout and I will push everything.

HeahDude · 2016-03-25T16:07:47Z

Updated title and description accordingly.

backbone87 · 2016-03-25T19:24:33Z

src/Symfony/Component/Form/Resources/translations/sf_form.fr.xlf

+        <body>
+            <trans-unit id="1">
+                <source>No choice available</source>
+                <target>![CDATA[<em>Aucun choix n'est disponible.</em>]]</target>


the cdata tags have syntax errors. correct is: <![CDATA[ and ]]> (not escaped)

@backbone87 thanks, I will update it.

HeahDude · 2016-03-30T15:36:03Z

I'm actually still stuck on that test:

1) Symfony\Bridge\Twig\Tests\Extension\FormExtensionDivLayoutTest::testEmptyCollection
Failed asserting that 

/div
    /em[.="[trans]No fields[/trans]"]
    [@id="my&id"]
    [@class="my&class"]
    [count(./div)=0]
/following-sibling::input[@type="hidden"][@id="names__token"]


matches exactly once. Matches 0 times in 

<div id="my&amp;id" class="my&amp;class">
        <em>[trans]No fields[/trans]</em>
    </div>
<input type="hidden" id="names__token" name="names[_token]">

/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php:84
/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/AbstractLayoutTest.php:106
/Users/Heah/Sites/dev/symfony/src/Symfony/Component/Form/Tests/AbstractDivLayoutTest.php:338

Does anyone know what I need to fix here ? :(

HeahDude · 2016-03-30T17:27:56Z

Rebased.

HeahDude · 2016-03-30T17:30:48Z

ping @webmozart :) There is no rush to merge any of my PR, we should polish anything but I can't go further for now without your review I guess.

Actually I've got three path tests failing on this one, and I can't make them pass...

webmozart · 2016-03-31T07:04:45Z

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

+    {%- set attr = attr|merge({class: (attr.class|default('') ~ ' form-control')|trim}) %}
+    <div {{ block('widget_container_attributes') }}>
+        <p>
+            <em>{{- translation_domain is same as(false) ? empty_view : empty_view|trans({}, translation_domain) -}}</em>


Why ? I think we should refrain from adding styling as much as possible.

em isnt about styling? its about emphasis. and because there is no form, that one might expect, you put the emphasis on the presented message, probably giving reasons as for why there are no children.

I agree, I did not even think about it when I added it naturally.

Because, it may be unexpected to see that string standing for a missing expectation.

The emphasis clearly mark the informational sense of such string IMHO.

IMO we should leave that decision up to the developer. We never made such assumptions in any other part of form themes.

Fair enough then :)

HeahDude · 2016-04-28T17:52:19Z

Status: needs work

nicolas-grekas · 2017-09-26T12:15:34Z

ping @HeahDude, status of this PR considering feat. freeze?

fabpot · 2018-01-24T13:28:54Z

@HeahDude Do you think you can work on finishing this PR or shall we close it?

fabpot · 2018-03-31T05:58:21Z

Closing this PR as there is no more activity. @HeahDude Feel free to reopen whenever you have time to finish it. Thanks.

lukepass · 2019-01-09T11:44:44Z

Hello, sorry for reopening this one but I had a problem today with an empty CollectionType and I had to manually set setRendered in order to avoid the empty template. Is there another way to fix it?

Thanks!

carsonbot added the Status: Needs Review label Jan 30, 2016

xabbuh reviewed Feb 3, 2016
View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Feb 3, 2016

xabbuh added the Form label Feb 3, 2016

webmozart added the Feature label Feb 4, 2016

HeahDude mentioned this pull request Feb 4, 2016

[Form] Add option "widget" to ChoiceType #6602

Closed

backbone87 reviewed Feb 8, 2016
View reviewed changes

carsonbot added Status: Needs Review and removed Status: Needs Work labels Feb 16, 2016

HeahDude force-pushed the feature-choice_type_option-empty_choices branch from d132bbe to 18fd85e Compare February 16, 2016 03:47

HeahDude changed the title ~~[WIP] [3.1] [Form] better handling empty choices in ChoiceType when expanded~~ [WIP] [3.1] [Form] add 'empty_choices' option in ChoiceType Feb 16, 2016

HeahDude mentioned this pull request Feb 16, 2016

Improved Bootstrap form theme for hidden fields #17568

Closed

xabbuh reviewed Feb 17, 2016
View reviewed changes

xabbuh added Status: Needs Work and removed Status: Needs Review labels Feb 17, 2016

carsonbot added Status: Needs Review and removed Status: Needs Work labels Feb 18, 2016

backbone87 reviewed Feb 22, 2016
View reviewed changes

HeahDude mentioned this pull request Mar 1, 2016

[Form] filter entity choicelist after hydration #14072

Closed

backbone87 mentioned this pull request Mar 10, 2016

Improve handling of choice fields with no choices in form templates #17579

Open

HeahDude changed the title ~~[WIP] [3.1] [Form] add 'empty_choices' option in ChoiceType~~ [WIP] [3.1] [Form] add 'empty_view' option in FormType Mar 25, 2016

backbone87 reviewed Mar 25, 2016
View reviewed changes

[From] add option empty_view to FormType

94f2a70

HeahDude force-pushed the feature-choice_type_option-empty_choices branch from f68e1e0 to 94f2a70 Compare March 30, 2016 17:21

webmozart reviewed Mar 31, 2016
View reviewed changes

This was referenced Apr 11, 2016

[Form] Nested form type renders ugly bootstrap horizontal form #18474

Closed

No Bootstrap markup is rendered for label set as false #18504

Closed

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 28, 2016

HeahDude changed the title ~~[WIP] [3.1] [Form] add 'empty_view' option in FormType~~ [WIP] [3.2] [Form] add 'empty_view' option in FormType Apr 28, 2016

HeahDude mentioned this pull request Sep 27, 2016

Symfony 3 - Collection, how to hide widget when ArrayCollection is empty #18804

Open

HeahDude mentioned this pull request Oct 4, 2016

[Form] compound forms without children should be considered rendered implicitly #20108

Closed

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017

fabpot closed this Mar 31, 2018

[WIP] [3.2] [Form] add 'empty_view' option in FormType #17609

[WIP] [3.2] [Form] add 'empty_view' option in FormType #17609

Conversation

HeahDude commented Jan 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Feb 3, 2016

HeahDude commented Feb 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented Feb 18, 2016

HeahDude commented Feb 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webmozart commented Mar 14, 2016

HeahDude commented Mar 14, 2016

webmozart commented Mar 25, 2016

HeahDude commented Mar 25, 2016

HeahDude commented Mar 25, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented Mar 30, 2016

HeahDude commented Mar 30, 2016

HeahDude commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeahDude commented Apr 28, 2016

nicolas-grekas commented Sep 26, 2017

fabpot commented Jan 24, 2018

fabpot commented Mar 31, 2018

lukepass commented Jan 9, 2019

HeahDude commented Jan 30, 2016 •

edited

Loading