Skip to content

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/Symfony/Bridge/Twig/Resources/views/Form/bootstrap_3_layout.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,18 @@
</div>
{%- endblock radio_row %}

{# Support #}

{% block empty_row -%}
{%- set translation_domain = translation_domain ?: 'sf_form' -%}
{%- 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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough then :)

</p>
</div>
{%- endblock empty_row %}

{# Errors #}

{% block form_errors -%}
Expand Down
28 changes: 22 additions & 6 deletions src/Symfony/Bridge/Twig/Resources/views/Form/form_div_layout.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@

{%- block form_widget -%}
{% if compound %}
{{- block('form_widget_compound') -}}
{%- if empty_view %}
{{- block('empty_row') -}}
{{ form_rest(form) }}
{%- else -%}
{{- block('form_widget_compound') -}}
{%- endif -%}
{% else %}
{{- block('form_widget_simple') -}}
{% endif %}
Expand Down Expand Up @@ -35,11 +40,15 @@
{%- endblock textarea_widget -%}

{%- block choice_widget -%}
{% if expanded %}
{{- block('choice_widget_expanded') -}}
{% else %}
{{- block('choice_widget_collapsed') -}}
{% endif %}
{%- if empty_view is defined and empty_view -%}
{{- block('empty_row') -}}
{%- else -%}
{%- if expanded -%}
{{- block('choice_widget_expanded') -}}
{%- else -%}
{{- block('choice_widget_collapsed') -}}
{%- endif -%}
{%- endif -%}
{%- endblock choice_widget -%}

{%- block choice_widget_expanded -%}
Expand Down Expand Up @@ -313,6 +322,13 @@
{% endfor %}
{%- endblock form_rows -%}

{%- block empty_row -%}
{%- set translation_domain = translation_domain ?: 'sf_form' -%}
<div {{ block('widget_container_attributes') }}>
<em>{{- translation_domain is same as(false) ? empty_view : empty_view|trans({}, translation_domain) -}}</em>
</div>
{%- endblock empty_row -%}

{%- block widget_attributes -%}
id="{{ id }}" name="{{ full_name }}"
{%- if disabled %} disabled="disabled"{% endif -%}
Expand Down
11 changes: 11 additions & 0 deletions src/Symfony/Bridge/Twig/Resources/views/Form/form_table_layout.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,14 @@
{{- form_rest(form) -}}
</table>
{%- endblock form_widget_compound -%}

{%- block empty_row -%}
{%- set translation_domain = translation_domain ?: 'sf_form' -%}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you set this translation domain?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now. Shouldn't we rather call the catalog forms.*.xlf to be consistent with the other catalogs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a suggestion that we might provide default translations for "No choices..", "No Fields" and even the actual "None" as default placeholder like it's done for validation message, there is already the dependency.

I can remove it of courses :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I was just not sure it will not conflict with other catalogs with the same name in users code.

<table {{ block('widget_container_attributes') }}>
<tr>
<td colspan="2">
<em>{{ translation_domain is same as(false) ? empty_view : empty_view|trans({}, translation_domain) }}</em>
</td>
</tr>
</table>
{%- endblock empty_row -%}
13 changes: 13 additions & 0 deletions src/Symfony/Bridge/Twig/Resources/views/Form/foundation_5_layout.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,19 @@
{% endif %}
{%- endblock choice_widget_expanded %}

{% block choice_widget_empty -%}
{%- if empty_choices is empty %}
{%- set empty_choices = 'No choice available' %}
{%- set choice_translation_domain = 'sf_form' %}
{%- endif -%}
{%- set attr = attr|merge({class: (attr.class|default('') ~ ' form-control')|trim}) %}
<div {{ block('widget_container_attributes') }}>
<p>
{{- choice_translation_domain is same as(false) ? empty_choices : empty_choices|trans({}, choice_translation_domain) -}}
</p>
</div>
{%- endblock %}

{% block checkbox_widget -%}
{% set parent_label_class = parent_label_class|default('') %}
{% if errors|length > 0 -%}
Expand Down
10 changes: 7 additions & 3 deletions src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/choice_widget.html.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
<?php if ($expanded): ?>
<?php echo $view['form']->block($form, 'choice_widget_expanded') ?>
<?php if (isset($empty_view) && $empty_view): ?>
<?php echo $view['form']->block($form, 'empty_row')?>
<?php else: ?>
<?php echo $view['form']->block($form, 'choice_widget_collapsed') ?>
<?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 endif ?>
9 changes: 9 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/empty_row.html.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php $translation_domain = $translation_domain ?: 'sf_form'; ?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

<div <?php echo $view['form']->block($form, 'widget_container_attributes') ?>>
<em>
<?php echo $view->escape(false !== $translation_domain
? $view['translator']->trans($empty_view, array(), $translation_domain)
: $empty_view
); ?>
</em>
</div>
7 changes: 6 additions & 1 deletion src/Symfony/Bundle/FrameworkBundle/Resources/views/Form/form_widget.html.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
<?php if ($compound): ?>
<?php echo $view['form']->block($form, 'form_widget_compound')?>
<?php if (isset($empty_view) && $empty_view): ?>
<?php echo $view['form']->block($form, 'empty_row')?>
<?php echo $view['form']->rest($form) ?>
<?php else: ?>
<?php echo $view['form']->block($form, 'form_widget_compound')?>
<?php endif ?>
<?php else: ?>
<?php echo $view['form']->block($form, 'form_widget_simple')?>
<?php endif ?>
13 changes: 13 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/Resources/views/FormTable/empty_row.html.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php $translation_domain = $translation_domain ?: 'sf_form'; ?>
<table <?php echo $view['form']->block($form, 'widget_container_attributes') ?>>
<tr>
<td colspan="2">
<em>
<?php echo $view->escape(false !== $translation_domain
? $view['translator']->trans($empty_view, array(), $translation_domain)
: $empty_view
); ?>
</em>
</td>
</tr>
</table>
13 changes: 13 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/ChoiceType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,18 @@ public function finishView(FormView $view, FormInterface $form, array $options)
$childView->vars['full_name'] = $childName;
}
}

// We have to override parent {@link FormType} condition of the empty_view option here,
// even if there is no choices the field view may not be empty because:
//
// - a select input is not "compound" but should be considered as any empty HTML tag,
//
// - when expanded it could hold a placeholder which has been counted as a children,
// we should ignore it in any case since its role is to be a "null" alternative to
// choices, not a choice itself. Think of "Choose something" with no choices.
if (array() === $view->vars['choices'] && array() === $view->vars['preferred_choices']) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this check needed? Can't we decide in the view whether to output the text?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@webmozart Because the view var empty_view is set here but is not compatible with the only choice type.
I just thought we should resolve the state of this view var from within this method to avoid holding too much logic in the view.

I don't know how BC could be broken with such changes in form themes. the fact that I modify several test with the current implementation is not really reassuring. What do you think ?

$view->vars['empty_view'] = $options['empty_view'];
}
}

/**
Expand Down Expand Up @@ -323,6 +335,7 @@ public function configureOptions(OptionsResolver $resolver)
'preferred_choices' => array(),
'group_by' => null,
'empty_data' => $emptyData,
'empty_view' => 'No choice available',
'placeholder' => $placeholderDefault,
'error_bubbling' => false,
'compound' => $compound,
Expand Down
12 changes: 12 additions & 0 deletions src/Symfony/Component/Form/Extension/Core/Type/FormType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,10 @@ public function finishView(FormView $view, FormInterface $form, array $options)
}

$view->vars['multipart'] = $multipart;

if ($form->getConfig()->getCompound()) {
$view->vars['empty_view'] = (bool) $view->count() ? false : $options['empty_view'];
}
}

/**
Expand Down Expand Up @@ -142,6 +146,12 @@ public function configureOptions(OptionsResolver $resolver)
};
};

// For any compound form which has no children, show some text
// instead of an empty HTML tag by default
$emptyView = function (Options $options) {
return $options['compound'] ? 'No fields' : false;
};

// For any form that is not represented by a single HTML control,
// errors should bubble up by default
$errorBubbling = function (Options $options) {
Expand All @@ -157,6 +167,7 @@ public function configureOptions(OptionsResolver $resolver)
$resolver->setDefaults(array(
'data_class' => $dataClass,
'empty_data' => $emptyData,
'empty_view' => $emptyView,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about naming this no_children_message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good :)

'trim' => true,
'required' => true,
'property_path' => null,
Expand All @@ -175,6 +186,7 @@ public function configureOptions(OptionsResolver $resolver)
));

$resolver->setAllowedTypes('label_attr', 'array');
$resolver->setAllowedTypes('empty_view', array('null', 'string', 'bool'));
}

/**
Expand Down
11 changes: 11 additions & 0 deletions src/Symfony/Component/Form/Resources/translations/sf_form.en.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?xml version="1.0"?>
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
<file source-language="en" datatype="plaintext" original="file.ext">
<body>
<trans-unit id="1">
<source>No choice available</source>
<target>No choice available</target>
</trans-unit>
</body>
</file>
</xliff>
11 changes: 11 additions & 0 deletions src/Symfony/Component/Form/Resources/translations/sf_form.fr.xlf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?xml version="1.0"?>
<xliff version="1.2" xmlns="urn:oasis:names:tc:xliff:document:1.2">
<file source-language="en" datatype="plaintext" original="file.ext">
<body>
<trans-unit id="1">
<source>No choice available</source>
<target>Aucun choix disponible</target>
</trans-unit>
</body>
</file>
</xliff>
42 changes: 42 additions & 0 deletions src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,27 @@ public function testSelectWithSizeBiggerThanOneCanBeRequired()
);
}

public function testSingleChoiceWithoutChoices()
{
$form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', null, array(
'choices' => array(),
'multiple' => false,
'expanded' => false,
'required' => true,
'attr' => array('class' => 'my&class'),
));

$this->assertMatchesXpath($this->renderWidget($form->createView()),
'/div
[@class="my&class form-control"]
[not(@required)]
[
./p/em/.="[trans]No choice available[/trans]"
]
'
);
}

public function testSingleChoiceWithoutTranslation()
{
$form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', '&a', array(
Expand Down Expand Up @@ -813,6 +834,27 @@ public function testSingleChoiceExpandedWithLabelsSetFalseByCallable()
);
}

public function testSingleChoiceExpandedWithoutChoices()
{
$form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', null, array(
'choices' => array(),
'multiple' => false,
'expanded' => true,
'required' => true,
'attr' => array('class' => 'my&class'),
));

$this->assertMatchesXpath($this->renderWidget($form->createView()),
'/div
[@class="my&class form-control"]
[not(@required)]
[
./p/em/.="[trans]No choice available[/trans]"
]
'
);
}

public function testSingleChoiceExpandedWithoutTranslation()
{
$form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', '&a', array(
Expand Down
9 changes: 7 additions & 2 deletions src/Symfony/Component/Form/Tests/AbstractDivLayoutTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,15 @@ public function testEmptyCollection()
'entry_type' => 'Symfony\Component\Form\Extension\Core\Type\TextType',
));

$this->assertWidgetMatchesXpath($form->createView(), array(),
$this->assertWidgetMatchesXpath($form->createView(), array('id' => 'my&id', 'attr' => array('class' => 'my&class')),
'/div
[./input[@type="hidden"][@id="names__token"]]
[@id="my&id"]
[@class="my&class"]
[
/em[.="[trans]No fields[/trans]"]
]
[count(./div)=0]
/following-sibling::input[@type="hidden"][@id="names__token"]
'
);
}
Expand Down
34 changes: 34 additions & 0 deletions src/Symfony/Component/Form/Tests/AbstractLayoutTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,23 @@ public function testSelectWithSizeBiggerThanOneCanBeRequired()
);
}

public function testSingleChoiceWithoutChoices()
{
$form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', null, array(
'choices' => array(),
'multiple' => false,
'expanded' => false,
'required' => true,
));

$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[not(@required)]
/em[.="[trans]No choice available[/trans]"]
'
);
}

public function testSingleChoiceWithoutTranslation()
{
$form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', '&a', array(
Expand Down Expand Up @@ -955,6 +972,23 @@ public function testSingleChoiceExpanded()
);
}

public function testSingleChoiceExpandedWithoutChoices()
{
$form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', null, array(
'choices' => array(),
'multiple' => false,
'expanded' => true,
'required' => true,
));

$this->assertWidgetMatchesXpath($form->createView(), array(),
'/div
[not(@required)]
/em[.="[trans]No choice available[/trans]"]
'
);
}

public function testSingleChoiceExpandedWithoutTranslation()
{
$form = $this->factory->createNamed('name', 'Symfony\Component\Form\Extension\Core\Type\ChoiceType', '&a', array(
Expand Down
Loading