search

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

The field labels in list is broken #4615

Closed peter-gribanov closed 7 years ago

peter-gribanov commented 7 years ago

Environment

Sonata packages

$ composer show --latest 'sonata-project/*'
sonata-project/admin-bundle              3.22.0          3.22.0
sonata-project/block-bundle              3.3.2           3.3.2
sonata-project/cache                     1.0.7           1.0.7
sonata-project/core-bundle               3.4.0           3.4.0
sonata-project/datagrid-bundle           2.x-dev 80692d9 2.x-dev 80692d9
sonata-project/doctrine-extensions       1.0.2           1.0.2
sonata-project/doctrine-orm-admin-bundle 3.1.6           3.1.6
sonata-project/easy-extends-bundle       2.2.0           2.2.0
sonata-project/exporter                  1.7.1           1.7.1
sonata-project/google-authenticator      1.1.0           1.1.0
sonata-project/user-bundle               3.2.4           3.2.4

Symfony packages

$ composer show --latest 'symfony/*'
symfony/assetic-bundle     v2.8.2  v2.8.2 Integrates Assetic into Symfony2
symfony/monolog-bundle     v2.12.1 v3.1.0 Symfony MonologBundle
symfony/polyfill-apcu      v1.5.0  v1.5.0 Symfony polyfill backporting apcu...
symfony/polyfill-intl-icu  v1.5.0  v1.5.0 Symfony polyfill for intl's ICU-r...
symfony/polyfill-mbstring  v1.5.0  v1.5.0 Symfony polyfill for the Mbstring...
symfony/polyfill-php54     v1.5.0  v1.5.0 Symfony polyfill backporting some...
symfony/polyfill-php55     v1.5.0  v1.5.0 Symfony polyfill backporting some...
symfony/polyfill-php56     v1.5.0  v1.5.0 Symfony polyfill backporting some...
symfony/polyfill-php70     v1.5.0  v1.5.0 Symfony polyfill backporting some...
symfony/polyfill-php72     v1.5.0  v1.5.0 Symfony polyfill backporting some...
symfony/polyfill-util      v1.5.0  v1.5.0 Symfony utilities for portability...
symfony/security-acl       v3.0.0  v3.0.0 Symfony Security Component - ACL ...
symfony/swiftmailer-bundle v2.6.3  v3.0.3 Symfony SwiftmailerBundle
symfony/symfony            v2.8.26 v3.3.6 The Symfony PHP framework

PHP version

$ php -v
PHP 5.6.24 (cli) (built: Jul 20 2016 21:19:29)
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies
    with Zend OPcache v7.0.6-dev, Copyright (c) 1999-2016, by Zend Technologies

Subject

After updating to the latest version i found that the field labels in list are broken.

Steps to reproduce

Example admin class

class BroadcastAdmin extends BaseAdmin
{

    // ...

    protected function configureListFields(ListMapper $listMapper)
    {
        $listMapper
            ->addIdentifier('id')
            ->addIdentifier('title', null, ['label' => 'Заголовок'])
            ->add('type', null, ['label' => 'Тип'])
            ->add('enabled', null, ['label' => 'Включена', 'editable' => true])
        ;
    }
}

Expected results

In sonata-project/admin-bundle 3.20.0

image

Actual results

What i saw after the update:

In sonata-project/admin-bundle 3.21.0

image

In sonata-project/admin-bundle 3.22.0

image

axzx commented 7 years ago

@peter-gribanov
You have a problem with encoding your php files. I've pasted your texts to my configureListFields and everything is fine.

config:

          ->add('user', null, array(
              'route' => array('name' => 'show'),
              'label' => 'Включена',
          ))
peter-gribanov commented 7 years ago

@axzx the problem not in a encoding.

  1. I use UTF8.
  2. Result is broken only if i change a version of SonataAdminBundle.

This code worked for 2 years until your update came out.

peter-gribanov commented 7 years ago

image

If i change labels

class BroadcastAdmin extends BaseAdmin
{

    // ...

    protected function configureListFields(ListMapper $listMapper)
    {
        $listMapper
            ->addIdentifier('id')
            ->add('type', null, ['label' => 'Что-то'])
            ->add('enabled', null, ['label' => 'Еще что-то', 'editable' => true])
        ;
    }
}

then i see this

image

If i move labels to translation file

class BroadcastAdmin extends BaseAdmin
{

    // ...

    protected function configureListFields(ListMapper $listMapper)
    {
        $listMapper
            ->addIdentifier('id')
            ->add('type', null, ['label' => 'something'])
            ->add('enabled', null, ['label' => 'something_else', 'editable' => true])
        ;
    }
}
# messages.ru.yml
something: 'Что-то'
something_else: 'Еще что-то'
# config.yml
framework:
    translator: { fallbacks: [ 'ru' ] }

I see this:

image

This problem not in my code.

axzx commented 7 years ago

I have tested your configuration and it works fine for me.

bez nazwy

axzx commented 7 years ago

In messages.ru.yml should be:

# messages.ru.yml
Something: 'Что-то'
Something Else: 'Еще что-то'
peter-gribanov commented 7 years ago

No. In messages.ru.yml should be:

# messages.ru.yml
app:
    broadcast:
        fields:
            something: 'Что-то'
            something_else: 'Еще что-то'
jlamur commented 7 years ago

@peter-gribanov Could you please check if this PR https://github.com/sonata-project/SonataAdminBundle/pull/4617 fixes your issue?

peter-gribanov commented 7 years ago

@jlamur Unfortunately this did not fix the problem. Label breaks somewhere before.

image

OskarStark commented 7 years ago

@peter-gribanov do you use a different naming strategy than the default one?

can you check if it works fine with the default naming strategy?

peter-gribanov commented 7 years ago

The problem in LabelTranslatorStrategy:

var_dump($fieldDescription->getLabel());
var_dump($this->admin->getLabelTranslatorStrategy()->getLabel($fieldDescription->getLabel(), 'list', 'label'));

image

@OskarStark I did not change the naming strategy.

peter-gribanov commented 7 years ago

@OskarStark

$this->admin->getLabelTranslatorStrategy() instanceof \Sonata\AdminBundle\Translator\NativeLabelTranslatorStrategy // true
peter-gribanov commented 7 years ago

@OskarStark It looks like it's still something on my side

public function getLabel($label, $context = '', $type = '')
{
    ob_clean();
    var_dump($label);
    $label = str_replace(array('_', '.'), ' ', $label);
    $label = preg_replace('/(?<=\\w)([A-Z])/', '_$1', $label);
    var_dump($label);
    var_dump(strtolower($label));
    var_dump(mb_strtolower($label));

    return trim(ucwords(str_replace('_', ' ', $label)));
}

image

OskarStark commented 7 years ago

Sounds interesting 🤔

peter-gribanov commented 7 years ago

I think that the problem with the locale.

var_dump(setlocale(LC_ALL, 0));
mb_detect_order('UTF8,CP1251,ASCII');
var_dump(mb_detect_encoding($label));
var_dump(mb_detect_encoding(strtolower($label)));
string(78) "LC_COLLATE=C;LC_CTYPE=Russian_Russia.1251;LC_MONETARY=C;LC_NUMERIC=C;LC_TIME=C"
string(5) "UTF-8"
string(12) "Windows-1251"

But it's strange, because in sonata-project/admin-bundle 3.20.0 everything is fine.

@OskarStark did you change the default naming strategy?

OskarStark commented 7 years ago

No we didn't change anything related to the naming strategy.

We name everything here: https://github.com/sonata-project/SonataAdminBundle/releases

peter-gribanov commented 7 years ago

I see this change https://github.com/sonata-project/SonataAdminBundle/commit/1b1d4b47d95daec7b4ebe8675cf6befce1d496ac If you specify a label, it is always transformed.

peter-gribanov commented 7 years ago

It broke BC. And now i'm forced to redefine the default transformer to NoopLabelTranslatorStrategy to be sure that everything works as before.

OskarStark commented 7 years ago

Could you add a failing test here with your configuration, so that we can fix it?

axzx commented 7 years ago

@peter-gribanov https://github.com/sonata-project/SonataAdminBundle/pull/4621 could you check it?

peter-gribanov commented 7 years ago

@axzx Yes. But your change does not make sense in this way.

peter-gribanov commented 7 years ago

I do not really understand that you tried to achieve changes like this #4602. Perhaps this is the right change. Just need to warn users that you are breaking BC.

peter-gribanov commented 7 years ago

By the way, what am I doing wrong?

I change configuration:

sonata_admin:
    admin_services:
        label_translator_strategy: 'sonata.admin.label.strategy.native'

and cache error

Invalid type for path "sonata_admin.admin_services.label_translator_strategy". Expected array, but got string

If i change it to array

sonata_admin:
    admin_services:
        label_translator_strategy: [ 'sonata.admin.label.strategy.native' ]

I see this:

Unrecognized option "0" under "sonata_admin.admin_services.label_translator_strategy"
axzx commented 7 years ago

Why do you set it? 'sonata.admin.label.strategy.native' is a default value

peter-gribanov commented 7 years ago

Excuse me. Of course, sonata.admin.label.strategy.noop

sonata_admin:
    admin_services:
        label_translator_strategy: [ 'sonata.admin.label.strategy.noop' ]
axzx commented 7 years ago 
sonata_admin:
    admin_services:
        <service id>:
            label_translator_strategy: 'sonata.admin.label.strategy.noop'
peter-gribanov commented 7 years ago

@axzx that is, is it the same that i will specify the service tag attribute?

admin.broadcast:
    class: BroadcastAdmin
    arguments: [ ~, Broadcast, ~ ]
    tags:
        - { name: sonata.admin, manager_type: orm, label: 'admin.menu.broadcast.broadcast', label_translator_strategy: 'sonata.admin.label.strategy.noop'}

Apparently the documentation lagged behind reality. https://sonata-project.org/bundles/admin/3-x/doc/reference/configuration.html

axzx commented 7 years ago

yes, the same solution

jxamorro commented 7 years ago

the problem is in the last two updated versions.

The field 'label' in list not should have translator strategy, because the user must put what he wants, for example

with this implementation the spanish accents no works.

The code is in the file listmapper.php on line 120:

if ($fieldDescription->getLabel() === null) {
    $fieldDescription->setOption(
            'label',
                $this->admin->getLabelTranslatorStrategy()->getLabel($fieldDescription->getName(), 'list', 'label')
        );
} elseif ($fieldDescription->getLabel() !== false) {
        $fieldDescription->setOption(
                'label',
                $this->admin->getLabelTranslatorStrategy()->getLabel($fieldDescription->getLabel(), 'list', 'label')
        );
}

i propose change the code before for the next solution:

if (($label = $fieldDescription->getLabel()) !== false && !$label) {
    $fieldDescription->setOption(
        'label', 
        $this->admin->getLabelTranslatorStrategy()->getLabel($fieldDescription->getName(), 'list', 'label')
    );
}

regards

axzx commented 7 years ago

@peter-gribanov @jxamorro

https://github.com/sonata-project/SonataAdminBundle/pull/4621/files could you check it now?

peter-gribanov commented 7 years ago

@axzx yes. that's better

OskarStark commented 7 years ago

I asked for a release, stay tuned 👍