Skip to content
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.

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

allow to change 'label_translator_strategy' for all admins at global config of 'sonata_admin.admin_services' key #4637

Closed
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ public function applyDefaults(ContainerBuilder $container, $serviceId, array $at
'menu_factory' => 'knp_menu.factory',
'route_builder' => 'sonata.admin.route.path_info'.
(($manager_type == 'doctrine_phpcr') ? '_slashes' : ''),
'label_translator_strategy' => 'sonata.admin.label.strategy.native',
'label_translator_strategy' => null !== $settings['label_translator_strategy'] ? $settings['label_translator_strategy'] : 'sonata.admin.label.strategy.native',
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this code supposed to achieve the exact same goal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean $definition->addMethodCall($method, $args); ?
currently 'label_translator_strategy' => 'sonata.admin.label.strategy.native' is hardcoded, and there is no way to globaly change the stratagy, only one by one for each admin service

Copy link
Contributor

@jlamur jlamur Sep 10, 2017

Choose a reason for hiding this comment

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

My bad, I missed the fact that $settings is not $overwriteAdminConfiguration.

Anyway, if other properties sonata_admin.admin_services.* work out of the box (I mean without such ternary condition here), I don't get why you have to override this one here.

Copy link
Contributor Author

@nnmer nnmer Sep 10, 2017

Choose a reason for hiding this comment

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

not sure about other properties yet. I was needed to change the label translation strategy for all my admin services and faced this issue

but i guess this settings will have similar issue if will try to set them in a global configuration area

Copy link
Contributor

Choose a reason for hiding this comment

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

That's why I asked. Could you test if they work? If not we will need a fix for all of them with a cleaner solution.

);

$definition->addMethodCall('setManagerType', array($manager_type));
Expand Down
59 changes: 29 additions & 30 deletions DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -245,36 +245,35 @@ public function getConfigTreeBuilder()
->end()
->end()
->arrayNode('admin_services')
->prototype('array')
Copy link
Contributor

Choose a reason for hiding this comment

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

Uuuuuh why did you remove this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

$rootNode = $treeBuilder->root('sonata_admin', 'array');

this already defining the global node type, and mentioned ->prototype('array') breaks underlying nodes, configuration in yml doesn't work and throws that error as in #3190 .

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you are removing the ability to set admin services on an admin per admin basis when doing that, aren't you? Why not create a default_admin_service node instead, and not touch that? This looks like a BC-break.

->children()
->scalarNode('model_manager')->defaultNull()->end()
->scalarNode('form_contractor')->defaultNull()->end()
->scalarNode('show_builder')->defaultNull()->end()
->scalarNode('list_builder')->defaultNull()->end()
->scalarNode('datagrid_builder')->defaultNull()->end()
->scalarNode('translator')->defaultNull()->end()
->scalarNode('configuration_pool')->defaultNull()->end()
->scalarNode('route_generator')->defaultNull()->end()
->scalarNode('validator')->defaultNull()->end()
->scalarNode('security_handler')->defaultNull()->end()
->scalarNode('label')->defaultNull()->end()
->scalarNode('menu_factory')->defaultNull()->end()
->scalarNode('route_builder')->defaultNull()->end()
->scalarNode('label_translator_strategy')->defaultNull()->end()
->scalarNode('pager_type')->defaultNull()->end()
->arrayNode('templates')
->addDefaultsIfNotSet()
->children()
->arrayNode('form')
->prototype('scalar')->end()
->end()
->arrayNode('filter')
->prototype('scalar')->end()
->end()
->arrayNode('view')
->useAttributeAsKey('id')
->prototype('scalar')->end()
->end()
->addDefaultsIfNotSet()
->children()
->scalarNode('model_manager')->defaultNull()->end()
->scalarNode('form_contractor')->defaultNull()->end()
->scalarNode('show_builder')->defaultNull()->end()
->scalarNode('list_builder')->defaultNull()->end()
->scalarNode('datagrid_builder')->defaultNull()->end()
->scalarNode('translator')->defaultNull()->end()
->scalarNode('configuration_pool')->defaultNull()->end()
->scalarNode('route_generator')->defaultNull()->end()
->scalarNode('validator')->defaultNull()->end()
->scalarNode('security_handler')->defaultNull()->end()
->scalarNode('label')->defaultNull()->end()
->scalarNode('menu_factory')->defaultNull()->end()
->scalarNode('route_builder')->defaultNull()->end()
->scalarNode('label_translator_strategy')->defaultNull()->end()
->scalarNode('pager_type')->defaultNull()->end()
->arrayNode('templates')
->addDefaultsIfNotSet()
->children()
->arrayNode('form')
->prototype('scalar')->end()
->end()
->arrayNode('filter')
->prototype('scalar')->end()
->end()
->arrayNode('view')
->useAttributeAsKey('id')
->prototype('scalar')->end()
->end()
->end()
->end()
Expand Down