Deprecate not registering filters

These changes force the user to register filters as services so in next major version a service locator could be used instead of the complete container.
sonata-project · Mar 28, 2021 · 7532bd0 · 7532bd0
1 parent 129cdf4
commit 7532bd0
Show file tree

Hide file tree

Showing 6 changed files with 206 additions and 64 deletions.
diff --git a/UPGRADE-3.x.md b/UPGRADE-3.x.md
@@ -4,6 +4,10 @@ UPGRADE 3.x
 UPGRADE FROM 3.xx to 3.xx
 =========================
 
+### `Sonata\AdminBundle\Filter\FilterFactory`
+
+Deprecated passing a value which is not registered as a service as argument 2 for `create()` method.
+
 ### Deprecated MenuBuilderInterface
 
 Deprecated `AbstractAdmin::getSideMenu()`

diff --git a/src/DependencyInjection/Compiler/AddFilterTypeCompilerPass.php b/src/DependencyInjection/Compiler/AddFilterTypeCompilerPass.php
@@ -13,9 +13,13 @@
 
 namespace Sonata\AdminBundle\DependencyInjection\Compiler;
 
+use Sonata\AdminBundle\Filter\FilterInterface;
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 
+// NEXT_MAJOR: Uncomment next line.
+// use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+
 /**
  * @final since sonata-project/admin-bundle 3.52
  *
@@ -25,6 +29,10 @@ class AddFilterTypeCompilerPass implements CompilerPassInterface
 {
     public function process(ContainerBuilder $container)
     {
+        if (!$container->has('sonata.admin.builder.filter.factory')) {
+            return;
+        }
+
         $definition = $container->getDefinition('sonata.admin.builder.filter.factory');
         $types = [];
 
@@ -34,6 +42,32 @@ public function process(ContainerBuilder $container)
             $serviceDefinition->setShared(false);
             $serviceDefinition->setPublic(true); // Temporary fix until we can support service locators
 
+            $class = $serviceDefinition->getClass();
+            $reflectionClass = $container->getReflectionClass($class);
+
+            if (null === $reflectionClass) {
+                // NEXT_MAJOR: Remove "trigger_error" and uncomment the exception below.
+                @trigger_error(sprintf(
+                    'Not declaring a filter with an existing class name is deprecated since'
+                    .' sonata-project/admin-bundle 3.x and will not work in 4.0.'
+                    .' You MUST register a service with an existing class name for service "%s".',
+                    $id,
+                ), \E_USER_DEPRECATED);
+
+                //throw new InvalidArgumentException(sprintf('Class "%s" used for service "%s" cannot be found.', $class, $id));
+            }
+
+            if (null !== $reflectionClass && !$reflectionClass->isSubclassOf(FilterInterface::class)) {
+                // NEXT_MAJOR: Remove "trigger_error" and uncomment the exception below.
+                @trigger_error(sprintf(
+                    'Registering service "%s" without implementing interface "%s" is deprecated since'
+                    .' sonata-project/admin-bundle 3.x and will be mandatory in 4.0.',
+                    $id,
+                    FilterInterface::class,
+                ), \E_USER_DEPRECATED);
+                // throw new InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, FilterInterface::class));
+            }
+
             $types[$serviceDefinition->getClass()] = $id;
 
             // NEXT_MAJOR: Remove this loop, only FQCN will be supported

diff --git a/src/Filter/FilterFactory.php b/src/Filter/FilterFactory.php
@@ -55,12 +55,19 @@ public function create($name, $type, array $options = [])
             if ($filter && !class_exists($type)) {
                 @trigger_error(sprintf(
                     'Referencing a filter by name (%s) is deprecated since version 3.57 and will be removed in 4.0.'
-                    .' Use the fully-qualified type class name instead (%s)',
+                    .' Use the fully-qualified type class name instead (%s).',
                     $type,
                     \get_class($filter)
                 ), \E_USER_DEPRECATED);
             }
         } elseif (class_exists($type)) {
+            // NEXT_MAJOR: Remove this "elseif" block
+            @trigger_error(sprintf(
+                'Not declaring a filter as service is deprecated since sonata-project/admin-bundle 3.x'
+                .' and will not work in 4.0.'
+                .' You MUST register a service with class name (%s) instead.',
+                $type,
+            ), \E_USER_DEPRECATED);
             $filter = new $type();
         } else {
             throw new \RuntimeException(sprintf('No attached service to type named `%s`', $type));

diff --git a/tests/DependencyInjection/Compiler/AddFilterTypeCompilerPassTest.php b/tests/DependencyInjection/Compiler/AddFilterTypeCompilerPassTest.php
@@ -13,83 +13,112 @@
 
 namespace Sonata\AdminBundle\Tests\DependencyInjection\Compiler;
 
-use PHPUnit\Framework\TestCase;
+use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractCompilerPassTestCase;
 use Sonata\AdminBundle\DependencyInjection\Compiler\AddFilterTypeCompilerPass;
+use Sonata\AdminBundle\Filter\FilterFactoryInterface;
+use Sonata\AdminBundle\Tests\Fixtures\Filter\BarFilter;
+use Sonata\AdminBundle\Tests\Fixtures\Filter\FooFilter;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Definition;
 
-class AddFilterTypeCompilerPassTest extends TestCase
-{
-    private $filterFactory;
-
-    private $fooFilter;
-
-    private $barFilter;
+// NEXT_MAJOR: Uncomment next line.
+//use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
 
-    private $bazFilter;
+final class AddFilterTypeCompilerPassTest extends AbstractCompilerPassTestCase
+{
+    use ExpectDeprecationTrait;
 
     protected function setUp(): void
     {
-        $this->filterFactory = $this->createMock(Definition::class);
-        $this->fooFilter = $this->createMock(Definition::class);
-        $this->barFilter = $this->createMock(Definition::class);
-        $this->bazFilter = $this->createMock(Definition::class);
+        parent::setUp();
+
+        $filterFactoryDefinition = new Definition(FilterFactoryInterface::class, [
+            null,
+            [],
+        ]);
+
+        $this->container
+            ->setDefinition('sonata.admin.builder.filter.factory', $filterFactoryDefinition);
     }
 
     public function testProcess(): void
     {
-        $containerBuilderMock = $this->createMock(ContainerBuilder::class);
-
-        $containerBuilderMock
-            ->method('getDefinition')
-            ->with($this->anything())
-            ->willReturnMap([
-                ['sonata.admin.builder.filter.factory', $this->filterFactory],
-                ['acme.demo.foo_filter', $this->fooFilter],
-                ['acme.demo.bar_filter', $this->barFilter],
-                ['acme.demo.baz_filter', $this->bazFilter],
+        $fooFilter = new Definition(FooFilter::class);
+        $fooFilter
+            ->addTag('sonata.admin.filter.type', [
+                'alias' => 'foo_filter_alias',
             ]);
 
-        $containerBuilderMock->expects($this->once())
-            ->method('findTaggedServiceIds')
-            ->with($this->equalTo('sonata.admin.filter.type'))
-            ->willReturn([
-                'acme.demo.foo_filter' => [
-                    'tag1' => [
-                        'alias' => 'foo_filter_alias',
-                    ],
-                ],
-                'acme.demo.bar_filter' => [
-                    'tag1' => [
-                        'alias' => 'bar_filter_alias',
-                    ],
-                ],
-                'acme.demo.baz_filter' => [
-                    'tag1' => [
-                    ],
-                ],
-            ]);
+        $this->container
+            ->setDefinition('acme.demo.foo_filter', $fooFilter);
 
-        $this->fooFilter->method('getClass')
-            ->willReturn('Acme\Filter\FooFilter');
+        $barFilter = new Definition(BarFilter::class);
+        $barFilter
+            ->addTag('sonata.admin.filter.type');
 
-        $this->barFilter->method('getClass')
-            ->willReturn('Acme\Filter\BarFilter');
+        $this->container
+            ->setDefinition('acme.demo.bar_filter', $barFilter);
 
-        $this->bazFilter->method('getClass')
-            ->willReturn('Acme\Filter\BazFilter');
+        $this->compile();
 
-        $this->filterFactory->expects($this->once())
-            ->method('replaceArgument')
-            ->with(1, $this->equalTo([
+        $this->assertContainerBuilderHasServiceDefinitionWithArgument(
+            'sonata.admin.builder.filter.factory',
+            1,
+            [
                 'foo_filter_alias' => 'acme.demo.foo_filter',
-                'Acme\Filter\FooFilter' => 'acme.demo.foo_filter',
-                'bar_filter_alias' => 'acme.demo.bar_filter',
-                'Acme\Filter\BarFilter' => 'acme.demo.bar_filter',
-                'Acme\Filter\BazFilter' => 'acme.demo.baz_filter',
-            ]));
-
-        $extensionsPass = new AddFilterTypeCompilerPass();
-        $extensionsPass->process($containerBuilderMock);
+                FooFilter::class => 'acme.demo.foo_filter',
+                BarFilter::class => 'acme.demo.bar_filter',
+            ]
+        );
+    }
+
+    /**
+     * NEXT_MAJOR: Remove legacy group.
+     *
+     * @group legacy
+     */
+    public function testServicesMustHaveAClassName(): void
+    {
+        $filter = new Definition('not_existing_class');
+        $filter
+            ->addTag('sonata.admin.filter.type');
+
+        $this->container
+            ->setDefinition('acme.demo.foo_filter', $filter);
+
+        // NEXT_MAJOR: Remove deprecation and uncomment exception.
+        $this->expectDeprecation('Not declaring a filter with an existing class name is deprecated since sonata-project/admin-bundle 3.x and will not work in 4.0. You MUST register a service with an existing class name for service "acme.demo.foo_filter".');
+//        $this->expectException(InvalidArgumentException::class);
+//        $this->expectExceptionMessage('Class "not_existing_class" used for service "acme.demo.foo_filter" cannot be found.');
+
+        $this->compile();
+    }
+
+    /**
+     * NEXT_MAJOR: Remove legacy group.
+     *
+     * @group legacy
+     */
+    public function testServicesMustImplementFilterInterface(): void
+    {
+        $filter = new Definition(\stdClass::class);
+        $filter
+            ->addTag('sonata.admin.filter.type');
+
+        $this->container
+            ->setDefinition('acme.demo.foo_filter', $filter);
+
+        // NEXT_MAJOR: Remove deprecation and uncomment exception.
+        $this->expectDeprecation('Registering service "acme.demo.foo_filter" without implementing interface "Sonata\AdminBundle\Filter\FilterInterface" is deprecated since sonata-project/admin-bundle 3.x and will be mandatory in 4.0.');
+//        $this->expectException(InvalidArgumentException::class);
+//        $this->expectExceptionMessage('Service "acme.demo.foo_filter" MUST implement interface "Sonata\AdminBundle\Filter\FilterInterface".');
+
+        $this->compile();
+    }
+
+    protected function registerCompilerPass(ContainerBuilder $container): void
+    {
+        $container->addCompilerPass(new AddFilterTypeCompilerPass());
     }
 }
diff --git a/tests/Filter/FilterFactoryTest.php b/tests/Filter/FilterFactoryTest.php
@@ -17,16 +17,20 @@
 use Sonata\AdminBundle\Filter\FilterFactory;
 use Sonata\AdminBundle\Filter\FilterInterface;
 use Sonata\AdminBundle\Form\Type\Filter\DefaultType;
+use Sonata\AdminBundle\Tests\Fixtures\Filter\FooFilter;
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 use Symfony\Component\DependencyInjection\Container;
 
 class FilterFactoryTest extends TestCase
 {
+    use ExpectDeprecationTrait;
+
     public function testEmptyType(): void
     {
         $this->expectException(\RuntimeException::class);
         $this->expectExceptionMessage('The type must be defined');
 
-        $filter = new FilterFactory(new Container(), []);
+        $filter = new FilterFactory(new Container());
         $filter->create('test', null);
     }
 
@@ -35,7 +39,7 @@ public function testUnknownType(): void
         $this->expectException(\RuntimeException::class);
         $this->expectExceptionMessage('No attached service to type named `mytype`');
 
-        $filter = new FilterFactory(new Container(), []);
+        $filter = new FilterFactory(new Container());
         $filter->create('test', 'mytype');
     }
 
@@ -44,13 +48,18 @@ public function testUnknownClassType(): void
         $this->expectException(\RuntimeException::class);
         $this->expectExceptionMessage('No attached service to type named `Sonata\AdminBundle\Form\Type\Filter\FooType`');
 
-        $filter = new FilterFactory(new Container(), []);
+        $filter = new FilterFactory(new Container());
         $filter->create('test', 'Sonata\AdminBundle\Form\Type\Filter\FooType');
     }
 
     public function testClassType(): void
     {
-        $filter = new FilterFactory(new Container(), []);
+        $container = new Container();
+        $container
+            ->set(DefaultType::class, new DefaultType());
+        $filter = new FilterFactory($container, [
+            DefaultType::class => DefaultType::class,
+        ]);
 
         $this->expectException(\RuntimeException::class);
         $this->expectExceptionMessage(
@@ -60,6 +69,20 @@ public function testClassType(): void
         $filter->create('test', DefaultType::class);
     }
 
+    /**
+     * NEXT_MAJOR: Remove this test.
+     *
+     * @group legacy
+     */
+    public function testItTriggersDeprecationWithoutTheService(): void
+    {
+        $filter = new FilterFactory(new Container());
+
+        $this->expectDeprecation('Not declaring a filter as service is deprecated since sonata-project/admin-bundle 3.x and will not work in 4.0. You MUST register a service with class name (Sonata\AdminBundle\Tests\Fixtures\Filter\FooFilter) instead.');
+
+        $filter->create('test', FooFilter::class);
+    }
+
     /**
      * NEXT_MAJOR: Remove this test.
      *

diff --git a/tests/Fixtures/Filter/BarFilter.php b/tests/Fixtures/Filter/BarFilter.php
@@ -0,0 +1,45 @@
+<?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\Fixtures\Filter;
+
+use Sonata\AdminBundle\Datagrid\ProxyQueryInterface;
+use Sonata\AdminBundle\Filter\Filter;
+use Sonata\AdminBundle\Form\Type\Filter\DefaultType;
+
+final class BarFilter extends Filter
+{
+    /**
+     * NEXT_MAJOR: Remove this method.
+     */
+    public function filter(ProxyQueryInterface $query, $alias, $field, $value): void
+    {
+    }
+
+    // NEXT_MAJOR: Add type declarations
+    public function apply($query, $value): void
+    {
+    }
+
+    public function getDefaultOptions(): array
+    {
+        return ['bar' => 'bar'];
+    }
+
+    public function getRenderSettings(): array
+    {
+        return [DefaultType::class, [
+            'label' => 'label',
+        ]];
+    }
+}