From b5e167746bf12317ca367144994589fda8d8c0e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Sun, 4 Dec 2016 20:43:24 +0100 Subject: [PATCH] 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. --- .../SonataFormatterExtension.php | 1 - Formatter/Pool.php | 71 ++++++++++++++----- Resources/config/formatter.xml | 4 +- .../EventListener/FormatterListenerTest.php | 12 +++- Tests/Form/Type/FormatterTypeTest.php | 12 +++- Tests/Formatter/PoolTest.php | 46 ++++++++++-- .../Constraints/FormatterValidatorTest.php | 8 ++- UPGRADE-3.x.md | 41 +++++++++++ 8 files changed, 163 insertions(+), 32 deletions(-) diff --git a/DependencyInjection/SonataFormatterExtension.php b/DependencyInjection/SonataFormatterExtension.php index d72f5837..4f044d0a 100644 --- 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 index 0f6c4b41..89adba98 100644 --- 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 index 064b375b..6e40f29c 100644 --- a/Resources/config/formatter.xml +++ b/Resources/config/formatter.xml @@ -8,7 +8,9 @@ - + + + diff --git a/Tests/Form/EventListener/FormatterListenerTest.php b/Tests/Form/EventListener/FormatterListenerTest.php index fe21a1cc..0b38a0cc 100644 --- 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 index b9a5f453..a5e8dd81 100644 --- 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 index 71c740fc..f6451900 100644 --- 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,7 +73,7 @@ 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'); @@ -81,12 +81,16 @@ public function testUnexpectedException() 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 index e0762493..d8bf536d 100644 --- 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 index dc563d84..7eb5b73b 100644 --- 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 =======================