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

add "setFormTheme" and "setFilterTheme" calls + change "initialize" priority #7127

Merged
merged 3 commits into from
Apr 30, 2021
Merged

add "setFormTheme" and "setFilterTheme" calls + change "initialize" priority #7127

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fcb0ce2
add "setFormTheme" and "setFilterTheme" calls + change "initialize" p…
dmaicher Apr 27, 2021
5fda8dc
only add method call if there is none already
dmaicher Apr 29, 2021
04c2d26
also fix other calls
dmaicher Apr 29, 2021
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
3 changes: 2 additions & 1 deletion src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,8 @@ public function applyDefaults(ContainerBuilder $container, $serviceId, array $at
$definition->addMethodCall('setSecurityInformation', ['%sonata.admin.configuration.security.information%']);
}

$definition->addMethodCall('initialize');
$definition->addMethodCall('setFormTheme', [$overwriteAdminConfiguration['templates']['form'] ?? []]);
Copy link
Member

Choose a reason for hiding this comment

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

should we check if the user has already call setFormTheme in the service definition and merge the arguments?

Copy link
Member

Choose a reason for hiding this comment

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

I would say that if you want to set extra form theme, you should do it with a lower priority instead.

For example sonata-project/SonataDoctrineORMAdminBundle#1427

Copy link
Member

Choose a reason for hiding this comment

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

I mean, if the user has a definition like:

Sonata\AdminBundle\Tests\App\Admin\FooAdmin:
        arguments: [~, Sonata\AdminBundle\Tests\App\Model\Foo, Sonata\AdminBundle\Controller\CRUDController]
        tags:
            - {name: sonata.admin, manager_type: test, label: Foo}
        calls:
            - [ setFormTheme, [['foo.html.twig' ]]]

I guess this setFormTheme call from the compiler pass will override the parameters previously set in this definition.

Copy link
Member

Choose a reason for hiding this comment

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

With this config, what is called first ?
setFormTheme, [['foo.html.twig' ]] or $definition->addMethodCall('setFormTheme', [$overwriteAdminConfiguration['templates']['form'] ?? []]); ?

I would expect the setFormTheme, [['foo.html.twig' ]] to be called last and override the global configuration. This would allow to remove a form theme and use another one instead for example. Isn't the case ?

But maybe the best would be to expose an addFormTheme method instead.

Copy link
Member

Choose a reason for hiding this comment

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

I would say setFormTheme, [['foo.html.twig' ]] is called first as it is in the definition of the service, and the compiler pass retrieves this definition, but I don't know.

We merge the arguments in the ORM:

https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/7493cfffcbe9996316e2dd0cdd9ac5092cf62a18/src/DependencyInjection/Compiler/AddTemplatesCompilerPass.php#L44

Maybe we can do that.

Copy link
Member

Choose a reason for hiding this comment

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

Which one ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was 1 failure:

1) Sonata\AdminBundle\Tests\DependencyInjection\Compiler\AddDependencyCallsCompilerPassTest::testProcessResultingConfig
None of the method calls matched the expected method "setRouteBuilder" with arguments Array &0 (
    0 => 'sonata.admin.route.path_info'
) with any invocation order index
Failed asserting that Symfony\Component\DependencyInjection\Definition Object &000000004bf94b930000000046f6a723 (
    'class' => 'Sonata\AdminBundle\Tests\DependencyInjection\Compiler\MockAdmin'
    'file' => null
    'factory' => null
    'shared' => false
    'deprecation' => Array &0 ()
    'properties' => Array &1 ()
    'calls' => Array &2 (
        0 => Array &3 (
            0 => 'setManagerType'
            1 => Array &4 (
                0 => 'orm'
            )
        )
        1 => Array &5 (
            0 => 'setPagerType'
            1 => Array &6 (
                0 => 'simple'
            )
        )
        2 => Array &7 (
            0 => 'setLabel'
            1 => Array &8 (
                0 => 'Foo'
            )
        )
        3 => Array &9 (
            0 => 'setListModes'
            1 => Array &10 (
                0 => Array &11 (
                    'list' => Array &12 (
                        'class' => 'fa fa-list fa-fw'
                    )
                    'mosaic' => Array &13 (
                        'class' => 'fa fa-th-large fa-fw'
                    )
                )
            )
        )
        4 => Array &14 (
            0 => 'setTemplateRegistry'
            1 => Array &15 (
                0 => Symfony\Component\DependencyInjection\Reference Object &000000004bf94ffb0000000046f6a723 (
                    'id' => 'sonata_news_admin.template_registry'
                    'invalidBehavior' => 1
                )
            )
        )
        5 => Array &16 (
            0 => 'setSecurityInformation'
            1 => Array &17 (
                0 => '%sonata.admin.configuration.security.information%'
            )
        )
        6 => Array &18 (
            0 => 'setFormTheme'
            1 => Array &19 (
                0 => Array &20 ()
            )
        )
        7 => Array &21 (
            0 => 'setFilterTheme'
            1 => Array &22 (
                0 => Array &23 ()
            )
        )
    )
    'instanceof' => Array &24 ()
    'autoconfigured' => false
    'configurator' => null
    'tags' => Array &25 (
        'sonata.admin' => Array &26 (
            0 => Array &27 (
                'group' => 'sonata_group_two'
                'label' => '5 Entry'
                'manager_type' => 'orm'
            )
        )
    )
    'public' => true
    'synthetic' => false
    'abstract' => false
    'lazy' => false
    'decoratedService' => null
    'autowired' => false
    'changes' => Array &28 (
        'public' => true
        'class' => true
        'shared' => true
    )
    'bindings' => Array &29 ()
    'errors' => Array &30 ()
    'arguments' => Array &31 (
        0 => 'sonata_news_admin'
        1 => 'Sonata\AdminBundle\Tests\DependencyInjection\Compiler\News'
        2 => 'Sonata\AdminBundle\Controller\CRUDController'
    )
    'innerServiceId' => null
    'decorationOnInvalid' => null
) has a method call to "setRouteBuilder" with the given arguments..

/var/www/SonataAdminBundle/vendor/matthiasnoback/symfony-dependency-injection-test/PhpUnit/DefinitionHasMethodCallConstraint.php:59
/var/www/SonataAdminBundle/vendor/matthiasnoback/symfony-dependency-injection-test/PhpUnit/AbstractContainerBuilderTestCase.php:188
/var/www/SonataAdminBundle/tests/DependencyInjection/Compiler/AddDependencyCallsCompilerPassTest.php:195

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So we are not setting default service calls then anymore, right? If there is no overwrite and no call yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe like this then? tests pass

diff --git a/src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php b/src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php
index b7af06402..c4b4508e9 100644
--- a/src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php
+++ b/src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php
@@ -338,7 +338,7 @@ class AddDependencyCallsCompilerPass implements CompilerPassInterface
 
             $method = $this->generateSetterMethodName($attr);
 
-            if (isset($overwriteAdminConfiguration[$attr]) || !$definition->hasMethodCall($method)) {
+            if (!$definition->hasMethodCall($method)) {
                 $args = [new Reference($overwriteAdminConfiguration[$attr] ?? $addServiceId)];
                 if ('translator' === $attr) {
                     $args[] = false;

so this would never overwrite any existing call

Copy link
Member

@VincentLanglet VincentLanglet Apr 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed, since we're doing

$overwriteAdminConfiguration[$attr] ?? $addServiceId

As soon as there is no call, we have something to set.

$definition->addMethodCall('setFilterTheme', [$overwriteAdminConfiguration['templates']['filter'] ?? []]);

return $definition;
}
Expand Down
34 changes: 34 additions & 0 deletions src/DependencyInjection/Compiler/AdminAddInitializeCallCompilerPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<?php

declare(strict_types=1);

/*
* This file is part of the Sonata Project package.
*
* (c) Thomas Rabaix <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Sonata\AdminBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

/**
* this compiler pass is registered with low priority to make sure it runs after all the other passes
* as we want the "initialize()" calls to come after all the other calls.
*
* @internal
*/
final class AdminAddInitializeCallCompilerPass implements CompilerPassInterface
dmaicher marked this conversation as resolved.
Show resolved Hide resolved
{
public function process(ContainerBuilder $container): void
{
$admins = $container->findTaggedServiceIds('sonata.admin');
foreach (array_keys($admins) as $id) {
$container->getDefinition($id)->addMethodCall('initialize');
}
}
}
2 changes: 2 additions & 0 deletions src/SonataAdminBundle.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Sonata\AdminBundle\DependencyInjection\Compiler\AddAuditReadersCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AddDependencyCallsCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AddFilterTypeCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AdminAddInitializeCallCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AdminMakerCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AdminSearchCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AliasDeprecatedPublicServicesCompilerPass;
Expand Down Expand Up @@ -63,6 +64,7 @@ public function build(ContainerBuilder $container)
$container->addCompilerPass(new AdminMakerCompilerPass());
$container->addCompilerPass(new AddAuditReadersCompilerPass());
$container->addCompilerPass(new AliasDeprecatedPublicServicesCompilerPass(), PassConfig::TYPE_AFTER_REMOVING);
$container->addCompilerPass(new AdminAddInitializeCallCompilerPass(), PassConfig::TYPE_BEFORE_REMOVING, -100);

$this->registerFormMapping();
}
Expand Down
26 changes: 26 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 @@ -200,6 +200,30 @@ public function testProcessResultingConfig(): void
'setRouteBuilder',
['sonata.admin.route.path_info_slashes']
);

$this->assertContainerBuilderHasServiceDefinitionWithMethodCall(
'sonata_article_admin',
'setFormTheme',
[[]]
);

$this->assertContainerBuilderHasServiceDefinitionWithMethodCall(
'sonata_article_admin',
'setFilterTheme',
[[]]
);

$this->assertContainerBuilderHasServiceDefinitionWithMethodCall(
'sonata_post_admin',
'setFormTheme',
[['some_form_template.twig']]
);

$this->assertContainerBuilderHasServiceDefinitionWithMethodCall(
'sonata_post_admin',
'setFilterTheme',
[['some_filter_template.twig']]
);
}

public function testProcessSortAdmins(): void
Expand Down Expand Up @@ -591,6 +615,8 @@ protected function getConfig()
'sonata_post_admin' => [
'templates' => [
'view' => ['user_block' => 'foobar.twig.html'],
'form' => ['some_form_template.twig'],
'filter' => ['some_filter_template.twig'],
],
],
'sonata_news_admin' => [
Expand Down
33 changes: 33 additions & 0 deletions tests/DependencyInjection/Compiler/AdminAddInitializeCallCompilerPassTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
<?php

declare(strict_types=1);

/*
* This file is part of the Sonata Project package.
*
* (c) Thomas Rabaix <[email protected]>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Sonata\AdminBundle\Tests\DependencyInjection\Compiler;

use PHPUnit\Framework\TestCase;
use Sonata\AdminBundle\DependencyInjection\Compiler\AdminAddInitializeCallCompilerPass;
use Sonata\AdminBundle\Tests\App\Admin\FooAdmin;
use Symfony\Component\DependencyInjection\ContainerBuilder;

final class AdminAddInitializeCallCompilerPassTest extends TestCase
{
public function testProcess(): void
{
$builder = new ContainerBuilder();
$builder->register('foo', FooAdmin::class)
->addTag('sonata.admin');

(new AdminAddInitializeCallCompilerPass())->process($builder);

$this->assertSame([['initialize', []]], $builder->getDefinition('foo')->getMethodCalls());
}
}
4 changes: 3 additions & 1 deletion tests/SonataAdminBundleTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
use Sonata\AdminBundle\DependencyInjection\Compiler\AddAuditReadersCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AddDependencyCallsCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AddFilterTypeCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AdminAddInitializeCallCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AdminMakerCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AdminSearchCompilerPass;
use Sonata\AdminBundle\DependencyInjection\Compiler\AliasDeprecatedPublicServicesCompilerPass;
Expand All @@ -37,7 +38,7 @@ public function testBuild(): void
{
$containerBuilder = $this->createMock(ContainerBuilder::class);

$containerBuilder->expects($this->exactly(11))
$containerBuilder->expects($this->exactly(12))
->method('addCompilerPass')
->withConsecutive(
[new AddDependencyCallsCompilerPass()],
Expand All @@ -51,6 +52,7 @@ public function testBuild(): void
[new AdminMakerCompilerPass()],
[new AddAuditReadersCompilerPass()],
[new AliasDeprecatedPublicServicesCompilerPass()],
[new AdminAddInitializeCallCompilerPass()],
);

$bundle = new SonataAdminBundle();
Expand Down