Prepare making the logger mandatory

This will have two benefits: - getting rid of if statements; - not having to switch arguments when making the second argument mandatory, which allows us to provide a painless upgrade path through the deprecation system.
sonata-project · Dec 4, 2016 · 2bab6fd · 2bab6fd
1 parent c1ccb1b
commit 2bab6fd
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 17 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
@@ -39,10 +39,18 @@ class Pool
      */
     public function __construct(LoggerInterface $logger = null, $defaultFormatter = null)
     {
+        // NEXT_MAJOR: make both parameters required
         $this->logger = $logger;
-
-        // NEXT_MAJOR: This should become a required first parameter
         $this->defaultFormatter = $defaultFormatter;
+        foreach (array('logger', 'defaultFormatter') as $argument) {
+            if (is_null($$argument)) {
+                @trigger_error(sprintf(<<<'EOT'
+Not providing the "%s" argument to %s is deprecated since 3.x.
+This argument will become mandatory in 4.0.
+EOT
+                , $argument, __METHOD__), E_USER_DEPRECATED);
+            }
+        }
     }
 
     /**
@@ -98,12 +106,14 @@ public function transform($code, $text)
                 $text = $env->render($text);
             }
         } catch (Twig_Error_Syntax $e) {
+            // NEXT_MAJOR: remove if statement
             if ($this->logger) {
                 $this->logger->critical(sprintf('[FormatterBundle::transform] %s - Error while parsing twig template : %s', $code, $e->getMessage()), array(
                     'text' => $text,
                 ));
             }
         } catch (Twig_Sandbox_SecurityError $e) {
+            // NEXT_MAJOR: remove if statement
             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,

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,9 @@ public function testWithValidFormatter()
 
         $this->assertSame($expected, $event->getData());
     }
+
+    private function getPool()
+    {
+        return new Pool($this->getMock('Psr\Log\LoggerInterface'), 'whatever');
+    }
 }
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,9 @@ public function testBcDefaultFormatter()
 
         $this->assertSame('foo', $pool->getDefaultFormatter());
     }
+
+    private function getPool()
+    {
+        return new Pool($this->getMock('Psr\Log\LoggerInterface'), 'whatever');
+    }
 }
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,29 @@ sonata_formatter:
     default_formatter: my_formatter
 ```
 
+## Deprecated unspecified Sonata\FormatterBundle\Formatter\Pool arguments
+
+Instanciating the `Sonata\FormatterBundle\Formatter\Pool` class should be done
+with both a `$logger` and `$defaultFormatter` argument.
+If you don't wish to log anything, use a `Psr\Log\NullLogger` instance.
+
+Before:
+
+```
+use Sonata\FormatterBundle\Formatter\Pool;
+
+new Pool();
+```
+
+After:
+
+```
+use Psr\Log\NullLogger;
+use Sonata\FormatterBundle\Formatter\Pool;
+
+new Pool(new NullLogger, 'default');
+```
+
 UPGRADE FROM 3.0 to 3.1
 =======================