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 missing deprecations #222

Merged
merged 5 commits into from
Dec 13, 2016
Merged

Add missing deprecations #222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
965706f
Add NEXT_MAJOR marker
greg0ire Dec 3, 2016
913cd1d
Prepare making the default_formatter mandatory
greg0ire Dec 3, 2016
19f6516
Move optional dependency out of the constructor
greg0ire Dec 4, 2016
aea3e98
Avoid long lines
greg0ire Dec 10, 2016
32b654b
Provide the special exception context key
greg0ire Dec 10, 2016
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
2 changes: 1 addition & 1 deletion DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public function getConfigTreeBuilder()

$rootNode
->children()
->scalarNode('default_formatter')->end() // TODO: make it required when the major version is changed
->scalarNode('default_formatter')->end() // NEXT_MAJOR: make this required
->arrayNode('formatters')
->useAttributeAsKey('name')
->prototype('array')
Expand Down
8 changes: 6 additions & 2 deletions DependencyInjection/SonataFormatterExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,13 @@ public function load(array $configs, ContainerBuilder $container)
$loader->load('ckeditor.xml');
}

// TODO: To be removed when major version is changed
// NEXT_MAJOR: remove this if block
if (!isset($config['default_formatter'])) {
@trigger_error(
'Not setting the default_formatter configuration node is deprecated since 3.x,'.
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't split this string in two parts just for the character limit, because searching for this particular string in the whole projects wouldn't work later.

This is a general discussion I don't want to start :trollface:

Copy link
Contributor

@sagikazarmark sagikazarmark Dec 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

So diplomatic.... 😝

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a general discussion I don't want to start :trollface:

Well why are you starting it then? All jokes aside, you both aprroved, so I'll just merge. But stay tuned for a real discussion about this, I'll make a blog post an issue soon.

' and will no longer be supported in 4.0.',
E_USER_DEPRECATED
);
reset($config['formatters']);
$config['default_formatter'] = key($config['formatters']);
}
Expand All @@ -67,7 +72,6 @@ public function load(array $configs, ContainerBuilder $container)
}

$pool = $container->getDefinition('sonata.formatter.pool');
// TODO: This should become the first (zero-indexed) argument when the major version is changed
$pool->addArgument($config['default_formatter']);

foreach ($config['formatters'] as $code => $configuration) {
Expand Down
84 changes: 66 additions & 18 deletions Formatter/Pool.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

this is per definition a BC break IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. Please tell me how it would be one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious about the reasoning as well. 😄

Copy link
Member

Choose a reason for hiding this comment

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

thats why i said per definition, but if we fullfill the interface, everything is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just for the theoretical debate: how is it a BC break per definition? If you don't actually implement the interface your code will be broken.

{
/**
* @var array
Expand All @@ -29,20 +31,60 @@ class Pool
protected $defaultFormatter;

/**
* NEXT_MAJOR: use LoggerAwareTrait.
*
* @var LoggerInterface
*/
protected $logger;
Copy link
Contributor

Choose a reason for hiding this comment

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

If you use the trait from the psr/log package you can drop this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scheduled for next major


/**
* @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;
if (func_num_args() == 2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NEXT_MAJOR: Remove this block

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the comment to line 34, to explain that line 35 must be kept, which would be harder to convey from line 45.

Copy link
Member

Choose a reason for hiding this comment

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

yoda please

list($logger, $defaultFormatter) = func_get_args();
$this->logger = $logger;
$this->defaultFormatter = $defaultFormatter;
} elseif (func_num_args() == 1) {
Copy link
Member

Choose a reason for hiding this comment

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

yoda please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't assign a value to a function call. So this would protect us from nothing, and make things less readable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to say that. Yoda for function call?????

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sagikazarmark : I always said the Jedi council has too much influence :) Look how carried away they can get

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, for me THIS is the point where sanity should be superior to consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, how you like, I really like the reading more

Copy link
Contributor Author

@greg0ire greg0ire Dec 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah but then again you guys put verbs at the end of a sentence 🇩🇪 ;) :trollface:

if ($defaultFormatter instanceof LoggerInterface) {
$this->logger = $defaultFormatter;
} else {
// NEXT_MAJOR: Only keep this block
$this->defaultFormatter = $defaultFormatter;
}
}

// TODO: This should become a required first parameter when the major version is changed
$this->defaultFormatter = $defaultFormatter;
// NEXT_MAJOR: keep the else block only
if ($this->logger) {
@trigger_error(sprintf(
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this error be triggered if there are two arguments too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe: if this->logger is set then trigger error else this->logger = new NullLogger().

'Providing a logger to %s through the constructor is deprecated since 3.x'.
Copy link
Contributor

Choose a reason for hiding this comment

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

This will become "Providing a logger to Pool::__construct through the constructor...". Is this intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is where the string split kicks in as well: if you change the message, you need to rearrange the strings as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right, changing it to __CLASS__

This is where the string split kicks in as well: if you change the message, you need to rearrange the strings as well.

Yeah but if you used nowdoc it's a breeze: you join the lines together again, hit gqq and you're done. If you use concatenation… that's another story.

' and will no longer be possible in 4.0'.
' This argument should be provided through the setLogger() method.',
__CLASS__
), E_USER_DEPRECATED);
} else {
$this->logger = new NullLogger();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a newline here?


// NEXT_MAJOR: make defaultFormatter required
if (is_null($this->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);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sagikazarmark : I'm done with the LoggerAwareInterface implementation (I think). Looks good?

}

/**
* NEXT_MAJOR: use Psr\Log\LoggerAwareTrait.
*
* @param LoggerInterface will be used to report errors
*/
public function setLogger(LoggerInterface $logger)
{
$this->logger = $logger;
}

/**
Expand Down Expand Up @@ -98,17 +140,23 @@ 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->logger->critical(sprintf(
'[FormatterBundle::transform] %s - Error while parsing twig template : %s',
$code,
$e->getMessage()
), array(
'text' => $text,
'exception' => $e,
));
} 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->logger->critical(sprintf(
'[FormatterBundle::transform] %s - the user try an non white-listed keyword : %s',
$code,
$e->getMessage()
), array(
'text' => $text,
'exception' => $e,
));
}

return $text;
Expand All @@ -127,7 +175,7 @@ public function getFormatters()
*/
public function getDefaultFormatter()
{
// TODO: This should be removed when the major version is changed
// NEXT_MAJOR: This should be removed
if (is_null($this->defaultFormatter)) {
reset($this->formatters);

Expand Down
4 changes: 3 additions & 1 deletion Resources/config/formatter.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"/>
Expand Down
12 changes: 10 additions & 2 deletions Tests/Form/EventListener/FormatterListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public function testWithInvalidFormatter()
{
$this->setExpectedException('RuntimeException');

$pool = new Pool();
$pool = $this->getPool();

$listener = new FormatterListener($pool, '[format]', '[source]', '[target]');

Expand All @@ -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]');
Expand All @@ -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;
}
}
12 changes: 9 additions & 3 deletions Tests/Form/Type/FormatterTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');

Expand Down Expand Up @@ -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');

Expand Down Expand Up @@ -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');
Expand Down
47 changes: 40 additions & 7 deletions Tests/Formatter/PoolTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));

Expand All @@ -37,7 +37,7 @@ public function testNonExistantFormatter()
{
$this->setExpectedException('RuntimeException');

$pool = new Pool();
$pool = $this->getPool();
$pool->get('foo');
}

Expand All @@ -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'));
Expand All @@ -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'));
Expand All @@ -73,20 +73,25 @@ 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('default');
$pool->setLogger($this->getMock('Psr\Log\LoggerInterface'));

$this->assertSame('default', $pool->getDefaultFormatter());
}

// TODO: This should be removed when the major version is changed
/**
* NEXT_MAJOR: This should be removed.
*
* @group legacy
*/
public function testBcDefaultFormatter()
{
$formatter = new RawFormatter();
Expand All @@ -98,4 +103,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;
}
}
8 changes: 6 additions & 2 deletions Tests/Validator/Constraints/FormatterValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Expand Down
Loading