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.

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

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

Closed
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',
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 •
edited
Loading

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 •
edited
Loading

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
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')
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
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