sonata-project / SonataAdminBundle

The missing Symfony Admin Generator
https://docs.sonata-project.org/projects/SonataAdminBundle
MIT License
2.11k stars 1.26k forks source link

Choice values not translated for list and show fields when 'choices_as_values' => true is used (as in Symfony 3 is standard) #4208

Closed aurelijusrozenas closed 4 years ago

aurelijusrozenas commented 7 years ago

Environment

Sonata packages

$ composer show sonata-project/*
sonata-project/admin-bundle              3.9.0 The missing Symfony Admin Generator
sonata-project/block-bundle              3.2.0 Symfony SonataBlockBundle
sonata-project/cache                     1.0.7 Cache library
sonata-project/core-bundle               3.1.1 Symfony SonataCoreBundle
sonata-project/doctrine-orm-admin-bundle 3.1.1 Symfony Sonata / Integrate Doctrine ORM into the SonataAdminBundle
sonata-project/exporter                  1.7.0 Lightweight Exporter library

Symfony packages

$ composer show symfony/*
symfony/assetic-bundle     v2.8.0  Integrates Assetic into Symfony2
symfony/monolog-bundle     2.11.1  Symfony MonologBundle
symfony/phpunit-bridge     v2.8.4  Symfony PHPUnit Bridge
symfony/polyfill-apcu      v1.1.1  Symfony polyfill backporting apcu_* functions to lower PHP versions
symfony/polyfill-intl-icu  v1.1.1  Symfony polyfill for intl's ICU-related data and classes
symfony/polyfill-mbstring  v1.1.1  Symfony polyfill for the Mbstring extension
symfony/polyfill-php54     v1.1.1  Symfony polyfill backporting some PHP 5.4+ features to lower PHP versions
symfony/polyfill-php55     v1.1.1  Symfony polyfill backporting some PHP 5.5+ features to lower PHP versions
symfony/polyfill-php56     v1.1.1  Symfony polyfill backporting some PHP 5.6+ features to lower PHP versions
symfony/polyfill-php70     v1.1.1  Symfony polyfill backporting some PHP 7.0+ features to lower PHP versions
symfony/polyfill-util      v1.1.1  Symfony utilities for portability of PHP codes
symfony/security-acl       v2.8.0  Symfony Security Component - ACL (Access Control List)
symfony/swiftmailer-bundle v2.3.11 Symfony SwiftmailerBundle
symfony/symfony            v2.8.4  The Symfony PHP framework

PHP version

$ php -v
HP 5.6.28-1+deb.sury.org~xenial+1 (cli) 

Subject

I am updating admins to future proof choice arrays with 'choices_as_values' => true. Form fields and Datagrid filter fields - works as expected. Show and list fields - ignores 'choices_as_values' => true,.

Steps to reproduce

protected function configureShowFields(ShowMapper $showMapper)
{
    $showMapper
        ->add(
            'type',
            'choice',
            [
                'choices' => self::getTypeChoices(),
                'choices_as_values' => true,
            ]
        )
    ;
}

protected function configureListFields(ListMapper $listMapper)
{
    $listMapper
        ->add(
            'type',
            'choice',
            [
                'choices' => self::getTypeChoices(),
                'choices_as_values' => true,
            ]
        )
    ;
}

protected static function getTypeChoices()
{
    $choices = [
        'Hey, the option!' => self::OPTION_NO_1,
    ];

    return $choices;
}
// this one works as expected
protected function configureFormFields(FormMapper $formMapper)
{
    $formMapper
        ->add(
            'type',
            'choice',
            [
                'choices' => self::getTypeChoices(),
                'choices_as_values' => true,
            ]
        )
    ;
}

Expected results

I would expect value to be translated in show view same as it is translated in form view.

Actual results

String is translated for form and filter fields, but not for show and list. Problem lies here and here. It does not check for choices_as_values and always uses old key => label choice array structure.

fracsi commented 7 years ago

Why do you want to use a FormType for list and show?

The list and show field are only for display, thus the type for the selected field should be choice, so Sonata can use a list_choice and show_choice templates, these templates have no relation to Symfony2 Choice form type.

aurelijusrozenas commented 7 years ago

Yeah, sorry, not using that anymore, old code. Updated.

fracsi commented 7 years ago

@aurelijusrozenas My previous comment is still valid. Why should show and list choice fields need to flip the choices?

aurelijusrozenas commented 7 years ago

So you can use single choice array without thinking about it. Otherwise you should pass one array to form and datagrid and flipped one for list and show. 'choices_as_values' => true is default from symfony 3.0. Did I miss anything?

fracsi commented 7 years ago

@aurelijusrozenas At the moment, that is right.

greg0ire commented 7 years ago

Just to make sure I got it right : list and show should be aware of what convention is in use for the ChoiceType because people probably want to provide the same source array with the same convention for list, show, and form, is that right?

fracsi commented 7 years ago

I think choices_as_values option exists in ChoiceType because there was a need to be able to duplicate values with different labels.

E.g.:

<select>
    <option value="1">Label</option>
    <option value="1">Another label</option>
<select>

But when showing the value in the list or show views there should only be one resolution for one value. How can you determine which label to show for the previous example, if the database (or any storage) holds only the value of the selected option (1)?

greg0ire commented 7 years ago

How can you determine which label to show for the previous example, if the database (or any storage) holds only the value of the selected option (1)?

Any label should do, right? So just take the first one, problem solved. That's probably how they do it in symfony when editing an existing object.

fracsi commented 7 years ago

@greg0ire I guess so. I think it could be implemented, but with respecting BC. I think a developer could choose to use different arrays for displaying and for forms.

In my opinion: If the choices_as_values option gets implemented in ListMapper and ShowMapper, then the default value for it should be false, marking in the docs that true would mean a similar behaviour as ChoiceType. I'm not sure that there should be a deprecation warning, as in the PR #4209 .

greg0ire commented 7 years ago

I'm not sure that there should be a deprecation warning, as in the PR #4209 .

On the contrary, I think there should be a warning, since the sensible thing to do is to be consistent with symfony, which implies with should move towards choices as values, and deprecate choices as keys

fracsi commented 7 years ago

@greg0ire And what if i would like to display in list and show views same labels for different values? E.g value 1 and 2 means the same thing for the user, but in the application there should be a difference.

greg0ire commented 7 years ago

I can't imagine a case where that would make sense. Can you show me a concrete example? Because if the user can't tell the difference, all they will see in your choice list is the same label twice. And they won't be able to know which to choose, which is weird to me.

fracsi commented 7 years ago

I use it for payment statuses.

1 - Created 2 - Processed 3 - Payment in progress 4 - Payment captured

But for the user: 1 - Created 2 - In progress 3 - In progress 4 - Completed

2 and 3 are for only internal and logging. The user should only see "In progress", and in the logs he can check the exact description.

greg0ire commented 7 years ago

Ok so in your form, you will have different translations for different labels, and in your show, you will have the same translation, but the labels will still be different (otherwise it means you have two enumerations for the same thing in your model, which means your model is not the same internally and for the client).

fracsi commented 7 years ago

@greg0ire I guess it could be a solution.

lukepass commented 5 years ago

Hello @greg0ire and sorry for reopening this issue but I think I solved the problem by changing the list template.

{#

This file is part of the Sonata package.

(c) Thomas Rabaix <thomas.rabaix@sonata-project.org>

For the full copyright and license information, please view the LICENSE
file that was distributed with this source code.

#}

{% extends get_admin_template('base_list_field', admin.code) %}

{% set is_editable =
    field_description.options.editable is defined and
    field_description.options.editable and
    admin.hasAccess('edit', object)
%}
{% set x_editable_type = field_description.type|sonata_xeditable_type %}

{% if is_editable and x_editable_type %}
    {% block field_span_attributes %}
        {% spaceless %}
            {{ parent() }}
            data-source="{{ field_description|sonata_xeditable_choices|json_encode }}"
        {% endspaceless %}
    {% endblock %}
{% endif %}

{% block field %}
{% spaceless %}
    {% if field_description.options.choices is defined %}
        {% if field_description.options.multiple is defined and field_description.options.multiple==true and value is iterable %}

            {% set result = '' %}
            {% set delimiter = field_description.options.delimiter|default(', ') %}

            {% for val in value %}
                {% if result is not empty %}
                    {% set result = result ~ delimiter %}
                {% endif %}

                {% if field_description.options.choices[val] is defined %}
                    {% if field_description.options.catalogue is not defined %}
                        {% set result = result ~ field_description.options.choices[val] %}
                    {% else %}
                        {% set result = result ~ field_description.options.choices[val]|trans({}, field_description.options.catalogue) %}
                    {% endif %}
                {% else %}
                    {% set result = result ~ val %}
                {% endif %}
            {% endfor %}

            {% set value = result %}

        {% elseif field_description.options.choices_as_values|default(false) %}
            {% for k, v in field_description.options.choices %}
                {% if v == value %}
                    {% if field_description.options.catalogue is not defined %}
                        {% set value = k %}
                    {% else %}
                        {% set value = k|trans({}, field_description.options.catalogue) %}
                    {% endif %}
                {% endif %}
            {% endfor %}
        {% elseif value in field_description.options.choices|keys %}
            {% if field_description.options.catalogue is not defined %}
                {% set value = field_description.options.choices[value] %}
            {% else %}
                {% set value = field_description.options.choices[value]|trans({}, field_description.options.catalogue) %}
            {% endif %}
        {% endif %}
    {% endif %}

    {{ value }}
{% endspaceless %}
{% endblock %}

Now it's possible to provide a choices_as_values => true for list and show admin classes and then you can use array with flipped keys/values as in the new versions of Symfony.

Do you think I could make a PR for this?

stale[bot] commented 4 years ago

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.