Move optional dependency out of the constructor

This will allows us to avoid having to switch arguments when making the default formatter mandatory, thus getting a good upgrade path through the deprecation system. If no logger is provided, a NullLogger is used.
sonata-project · Dec 10, 2016 · b5e1677 · b5e1677
1 parent 913cd1d
commit b5e1677
Show file tree

Hide file tree

Showing 8 changed files with 163 additions and 32 deletions.
diff --git a/DependencyInjection/SonataFormatterExtension.php b/DependencyInjection/SonataFormatterExtension.php
@@ -72,7 +72,6 @@ public function load(array $configs, ContainerBuilder $container)
         }
 
         $pool = $container->getDefinition('sonata.formatter.pool');
-        // NEXT_MAJOR: This should become the first (zero-indexed) argument
         $pool->addArgument($config['default_formatter']);
 
         foreach ($config['formatters'] as $code => $configuration) {

diff --git a/Formatter/Pool.php b/Formatter/Pool.php
@@ -11,12 +11,14 @@
 
 namespace Sonata\FormatterBundle\Formatter;
 
+use Psr\Log\LoggerAwareInterface;
 use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 use Twig_Environment;
 use Twig_Error_Syntax;
 use Twig_Sandbox_SecurityError;
 
-class Pool
+class Pool implements LoggerAwareInterface
 {
     /**
      * @var array
@@ -34,15 +36,43 @@ class Pool
     protected $logger;
 
     /**
-     * @param LoggerInterface|null $logger
-     * @param string|null          $defaultFormatter
+     * @param string|null $defaultFormatter
      */
-    public function __construct(LoggerInterface $logger = null, $defaultFormatter = null)
+    public function __construct($defaultFormatter = null)
     {
-        $this->logger = $logger;
+        // NEXT_MAJOR: remove this block
+        if (func_num_args() == 2) {
+            list($logger, $defaultFormatter) = func_get_args();
+            $this->logger = $logger;
+            $this->defaultFormatter = $defaultFormatter;
 
-        // NEXT_MAJOR: This should become a required first parameter
+            return;
+        }
+        if (func_num_args() == 1 && $defaultFormatter instanceof LoggerInterface) {
+            $this->logger = $defaultFormatter;
+            @trigger_error(sprintf(
+                'Providing a logger to %s through the constructor is deprecated since 3.x'.
+                ' and we no longer be possible in 4.0'.
+                ' This argument should be provided through the setLogger() method.',
+                __METHOD__
+            ), E_USER_DEPRECATED);
+
+            return;
+        }
         $this->defaultFormatter = $defaultFormatter;
+        // NEXT_MAJOR: make defaultFormatter required
+        if (is_null($defaultFormatter)) {
+            @trigger_error(sprintf(
+                'Not providing the defaultFormatter argument to %s is deprecated since 3.x.'.
+                ' This argument will become mandatory in 4.0.',
+                __METHOD__
+            ), E_USER_DEPRECATED);
+        }
+    }
+
+    final public function setLogger(LoggerInterface $logger)
+    {
+        $this->logger = $logger;
     }
 
     /**
@@ -98,17 +128,13 @@ public function transform($code, $text)
                 $text = $env->render($text);
             }
         } catch (Twig_Error_Syntax $e) {
-            if ($this->logger) {
-                $this->logger->critical(sprintf('[FormatterBundle::transform] %s - Error while parsing twig template : %s', $code, $e->getMessage()), array(
-                    'text' => $text,
-                ));
-            }
+            $this->getLogger()->critical(sprintf('[FormatterBundle::transform] %s - Error while parsing twig template : %s', $code, $e->getMessage()), array(
+                'text' => $text,
+            ));
         } catch (Twig_Sandbox_SecurityError $e) {
-            if ($this->logger) {
-                $this->logger->critical(sprintf('[FormatterBundle::transform] %s - the user try an non white-listed keyword : %s', $code, $e->getMessage()), array(
-                    'text' => $text,
-                ));
-            }
+            $this->getLogger()->critical(sprintf('[FormatterBundle::transform] %s - the user try an non white-listed keyword : %s', $code, $e->getMessage()), array(
+                'text' => $text,
+            ));
         }
 
         return $text;
@@ -136,4 +162,17 @@ public function getDefaultFormatter()
 
         return $this->defaultFormatter;
     }
+
+    /**
+     * Falls back to a NullLogger so that we do not have to check for the logger
+     * before calling it.
+     */
+    private function getLogger()
+    {
+        if ($this->logger) {
+            return $this->logger;
+        }
+
+        return new NullLogger();
+    }
 }
diff --git a/Resources/config/formatter.xml b/Resources/config/formatter.xml
@@ -8,7 +8,9 @@
     </parameters>
     <services>
         <service id="sonata.formatter.pool" class="Sonata\FormatterBundle\Formatter\Pool">
-            <argument type="service" id="logger"/>
+            <call method="setLogger">
+                <argument type="service" id="logger"/>
+            </call>
         </service>
         <service id="sonata.formatter.text.markdown" class="%sonata.formatter.text.markdown.class%">
             <tag name="sonata.text.formatter" code="markdown"/>

diff --git a/Tests/Form/EventListener/FormatterListenerTest.php b/Tests/Form/EventListener/FormatterListenerTest.php
@@ -21,7 +21,7 @@ public function testWithInvalidFormatter()
     {
         $this->setExpectedException('RuntimeException');
 
-        $pool = new Pool();
+        $pool = $this->getPool();
 
         $listener = new FormatterListener($pool, '[format]', '[source]', '[target]');
 
@@ -41,7 +41,7 @@ public function testWithValidFormatter()
             return strtoupper($text);
         }));
 
-        $pool = new Pool();
+        $pool = $this->getPool();
         $pool->add('myformat', $formatter);
 
         $listener = new FormatterListener($pool, '[format]', '[source]', '[target]');
@@ -62,4 +62,12 @@ public function testWithValidFormatter()
 
         $this->assertSame($expected, $event->getData());
     }
+
+    private function getPool()
+    {
+        $pool = new Pool('whatever');
+        $pool->setLogger($this->getMock('Psr\Log\LoggerInterface'));
+
+        return $pool;
+    }
 }
diff --git a/Tests/Form/Type/FormatterTypeTest.php b/Tests/Form/Type/FormatterTypeTest.php
@@ -86,7 +86,9 @@ public function testBuildFormSeveralChoices()
 
     public function testBuildFormWithCustomFormatter()
     {
-        $pool = $this->getMockBuilder('Sonata\FormatterBundle\Formatter\Pool')->getMock();
+        $pool = $this->getMockBuilder('Sonata\FormatterBundle\Formatter\Pool')
+            ->disableOriginalConstructor()
+            ->getMock();
         $translator = $this->getMock('Symfony\Component\Translation\TranslatorInterface');
         $configManager = $this->getMock('Ivory\CKEditorBundle\Model\ConfigManagerInterface');
 
@@ -130,7 +132,9 @@ public function testBuildFormWithCustomFormatter()
 
     public function testBuildFormWithDefaultFormatter()
     {
-        $pool = $this->getMockBuilder('Sonata\FormatterBundle\Formatter\Pool')->getMock();
+        $pool = $this->getMockBuilder('Sonata\FormatterBundle\Formatter\Pool')
+            ->disableOriginalConstructor()
+            ->getMock();
         $translator = $this->getMock('Symfony\Component\Translation\TranslatorInterface');
         $configManager = $this->getMock('Ivory\CKEditorBundle\Model\ConfigManagerInterface');
 
@@ -173,7 +177,9 @@ public function testBuildFormWithDefaultFormatter()
 
     public function testBuildFormWithDefaultFormatterAndPluginManager()
     {
-        $pool = $this->getMockBuilder('Sonata\FormatterBundle\Formatter\Pool')->getMock();
+        $pool = $this->getMockBuilder('Sonata\FormatterBundle\Formatter\Pool')
+            ->disableOriginalConstructor()
+            ->getMock();
         $translator = $this->getMock('Symfony\Component\Translation\TranslatorInterface');
         $configManager = $this->getMock('Ivory\CKEditorBundle\Model\ConfigManagerInterface');
         $pluginManager = $this->getMock('Ivory\CKEditorBundle\Model\PluginManagerInterface');

diff --git a/Tests/Formatter/PoolTest.php b/Tests/Formatter/PoolTest.php
@@ -22,7 +22,7 @@ public function testPool()
         $env = $this->getMock('\Twig_Environment');
         $env->expects($this->once())->method('render')->will($this->returnValue('Salut'));
 
-        $pool = new Pool();
+        $pool = $this->getPool();
 
         $this->assertFalse($pool->has('foo'));
 
@@ -37,7 +37,7 @@ public function testNonExistantFormatter()
     {
         $this->setExpectedException('RuntimeException');
 
-        $pool = new Pool();
+        $pool = $this->getPool();
         $pool->get('foo');
     }
 
@@ -47,7 +47,7 @@ public function testSyntaxError()
         $env = $this->getMock('\Twig_Environment');
         $env->expects($this->once())->method('render')->will($this->throwException(new \Twig_Error_Syntax('Error')));
 
-        $pool = new Pool();
+        $pool = $this->getPool();
         $pool->add('foo', $formatter, $env);
 
         $this->assertSame('Salut', $pool->transform('foo', 'Salut'));
@@ -59,7 +59,7 @@ public function testTwig_Sandbox_SecurityError()
         $env = $this->getMock('\Twig_Environment');
         $env->expects($this->once())->method('render')->will($this->throwException(new \Twig_Sandbox_SecurityError('Error')));
 
-        $pool = new Pool();
+        $pool = $this->getPool();
         $pool->add('foo', $formatter, $env);
 
         $this->assertSame('Salut', $pool->transform('foo', 'Salut'));
@@ -73,20 +73,24 @@ public function testUnexpectedException()
         $env = $this->getMock('\Twig_Environment');
         $env->expects($this->once())->method('render')->will($this->throwException(new \RuntimeException('Error')));
 
-        $pool = new Pool();
+        $pool = $this->getPool();
         $pool->add('foo', $formatter, $env);
 
         $pool->transform('foo', 'Salut');
     }
 
     public function testDefaultFormatter()
     {
-        $pool = new Pool(null, 'default');
+        $pool = new Pool($this->getMock('Psr\Log\LoggerInterface'), 'default');
 
         $this->assertSame('default', $pool->getDefaultFormatter());
     }
 
-    // NEXT_MAJOR: This should be removed
+    /**
+     * NEXT_MAJOR: This should be removed.
+     *
+     * @group legacy
+     */
     public function testBcDefaultFormatter()
     {
         $formatter = new RawFormatter();
@@ -98,4 +102,32 @@ public function testBcDefaultFormatter()
 
         $this->assertSame('foo', $pool->getDefaultFormatter());
     }
+
+    /**
+     * NEXT_MAJOR: This should be removed.
+     *
+     * @group legacy
+     */
+    public function testLoggerProvidedThroughConstuctor()
+    {
+        $formatter = new RawFormatter();
+        $pool = new Pool($logger = $this->getMock('Psr\Log\LoggerInterface'));
+        $env = $this->getMock('\Twig_Environment');
+        $env->expects($this->once())->method('render')->will(
+            $this->throwException(new \Twig_Sandbox_SecurityError('Error'))
+        );
+
+        $pool->add('foo', $formatter, $env);
+        $logger->expects($this->once())->method('critical');
+
+        $pool->transform('foo', 'whatever');
+    }
+
+    private function getPool()
+    {
+        $pool = new Pool('whatever');
+        $pool->setLogger($this->getMock('Psr\Log\LoggerInterface'));
+
+        return $pool;
+    }
 }
diff --git a/Tests/Validator/Constraints/FormatterValidatorTest.php b/Tests/Validator/Constraints/FormatterValidatorTest.php
@@ -27,7 +27,9 @@ protected function setUp()
 
     public function testValidator()
     {
-        $pool = $this->getMock('Sonata\FormatterBundle\Formatter\Pool');
+        $pool = $this->getMockBuilder('Sonata\FormatterBundle\Formatter\Pool')
+            ->disableOriginalConstructor()
+            ->getMock();
 
         $validator = new FormatterValidator($pool);
         $this->assertInstanceOf('Symfony\Component\Validator\ConstraintValidator', $validator);
@@ -61,7 +63,9 @@ public function testInvalidCase()
 
     public function testValidCase()
     {
-        $pool = $this->getMock('Sonata\FormatterBundle\Formatter\Pool');
+        $pool = $this->getMockBuilder('Sonata\FormatterBundle\Formatter\Pool')
+            ->disableOriginalConstructor()
+            ->getMock();
         $pool->expects($this->any())
             ->method('has')
             ->will($this->returnValue(true));

diff --git a/UPGRADE-3.x.md b/UPGRADE-3.x.md
@@ -10,6 +10,47 @@ sonata_formatter:
     default_formatter: my_formatter
 ```
 
+## Deprecated specifying a logger in the Sonata\FormatterBundle\Formatter\Pool constructor
+
+Providing a logger to a `Sonata\FormatterBundle\Formatter\Pool` instance is
+optional, and optional dependencies should be provided through a setter.
+
+Before:
+
+```
+use Sonata\FormatterBundle\Formatter\Pool;
+
+new Pool($myLogger, 'myFormatter');
+```
+
+After:
+
+```
+$pool = new Pool('myFormatter');
+$pool->setLogger($myLogger);
+```
+
+## Deprecated unspecified Sonata\FormatterBundle\Formatter\Pool defaultFormatter
+
+Instanciating the `Sonata\FormatterBundle\Formatter\Pool` class should be done
+with both a `$defaultFormatter` argument.
+
+Before:
+
+```
+use Sonata\FormatterBundle\Formatter\Pool;
+
+new Pool();
+```
+
+After:
+
+```
+use Sonata\FormatterBundle\Formatter\Pool;
+
+new Pool('myFormatter');
+```
+
 UPGRADE FROM 3.0 to 3.1
 =======================