Skip to content

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

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
nnmer wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8bce6a2
patch to proper overwrite 'label_translator_strategy' from yml config…
nnmer Sep 1, 2017
3609edc
test that if we overwrite label_translation_strategy the service is a…
nnmer Sep 10, 2017
0393402
fixing StyleCi for the test
nnmer Sep 10, 2017
0a7f655
OMG, StyleCI wants a periods at the end of the sentence.
nnmer Sep 10, 2017
8ac9ea3
okay, follow recommendation of codacy/pr , change variable name
nnmer Sep 10, 2017
e5a8a25
AddDependencyCallsCompilerPassTest relocate public methods above prot…
nnmer Sep 10, 2017
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
2 changes: 1 addition & 1 deletion DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php
View file
Open in desktop
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',

@jlamur jlamur Sep 10, 2017

Copy link
Copy Markdown
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?

@nnmer nnmer Sep 10, 2017

Copy link
Copy Markdown
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

@jlamur jlamur Sep 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.

@nnmer nnmer Sep 10, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@jlamur jlamur Sep 10, 2017

Copy link
Copy Markdown
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
View file
Open in desktop
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')

@greg0ire greg0ire Sep 1, 2017

Copy link
Copy Markdown
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?

@nnmer nnmer Sep 10, 2017

Copy link
Copy Markdown
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 .

@greg0ire greg0ire Sep 10, 2017

Copy link
Copy Markdown
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
44 changes: 44 additions & 0 deletions Tests/DependencyInjection/Compiler/AddDependencyCallsCompilerPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,50 @@ public function testProcessAbstractAdminServiceInServiceDefinition()
$this->assertSame('extra_argument_2', $definition->getArgument(3));
}


/**
* By default label translation policy is 'sonata.admin.label.strategy.native'.
*/
public function testLabelTranslationStrategyDefaultConfig()
{
$container = $this->getContainer();

$this->extension->load(array(), $container);

$compilerPass = new AddDependencyCallsCompilerPass();
$compilerPass->process($container);

$container->compile();

$this->assertContains('sonata.admin.label.strategy.native', $container->getServiceIds());
}

/**
* Overwritten default label translation policy service should be available in a services pool.
*
* @depends testLabelTranslationStrategyDefaultConfig
*/
public function testLabelTranslationStrategyOverwrittenConfig()
{
$config = array(
'sonata_admin' => array(
'admin_services' => array(
'label_translator_strategy' => 'sonata.admin.label.strategy.underscore'
)
)
);
$container = $this->getContainer();

$this->extension->load($config, $container);

$compilerPass = new AddDependencyCallsCompilerPass();
$compilerPass->process($container);

$container->compile();

$this->assertContains('sonata.admin.label.strategy.underscore', $container->getServiceIds());
}

/**
* @return array
*/
Expand Down