Skip to content

Add support for deprecated definitions #15491

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.

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

Merged
merged 6 commits into from
Sep 25, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,9 @@ private function resolveDefinition(ContainerBuilder $container, DefinitionDecora
if ($parentDef->getFactoryService(false)) {
$def->setFactoryService($parentDef->getFactoryService(false));
}
if ($parentDef->isDeprecated()) {
$def->setDeprecated(true, $parentDef->getDeprecationMessage('%service_id%'));
}
$def->setFactory($parentDef->getFactory());
$def->setConfigurator($parentDef->getConfigurator());
$def->setFile($parentDef->getFile());
Expand Down Expand Up @@ -162,6 +165,9 @@ private function resolveDefinition(ContainerBuilder $container, DefinitionDecora
if (isset($changes['lazy'])) {
$def->setLazy($definition->isLazy());
}
if (isset($changes['deprecated'])) {
$def->setDeprecated($definition->isDeprecated(), $definition->getDeprecationMessage('%service_id%'));
}
if (isset($changes['decorated_service'])) {
$decoratedService = $definition->getDecoratedService();
if (null === $decoratedService) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,10 @@ public function createService(Definition $definition, $id, $tryProxy = true)
throw new RuntimeException(sprintf('You have requested a synthetic service ("%s"). The DIC does not know how to construct this service.', $id));
}

if ($definition->isDeprecated()) {
@trigger_error($definition->getDeprecationMessage($id), E_USER_DEPRECATED);
}

if ($tryProxy && $definition->isLazy()) {
$container = $this;

Expand Down
61 changes: 61 additions & 0 deletions src/Symfony/Component/DependencyInjection/Definition.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ class Definition
private $factoryMethod;
private $factoryService;
private $shared = true;
private $deprecated = false;
private $deprecationTemplate = 'The "%service_id%" service is deprecated. You should stop using it, as it will soon be removed.';
private $scope = ContainerInterface::SCOPE_CONTAINER;
private $properties = array();
private $calls = array();
Expand Down Expand Up @@ -829,6 +831,65 @@ public function isAbstract()
return $this->abstract;
}

/**
* Whether this definition is deprecated, that means it should not be called
* anymore.
*
* @param bool $status
* @param string $template Template message to use if the definition is deprecated
*
* @return Definition the current instance
*
* @throws InvalidArgumentException When the message template is invalid.
*
* @api
*/
public function setDeprecated($status = true, $template = null)
{
if (null !== $template) {
if (preg_match('#[\r\n]|\*/#', $template)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Taluu : sorry for digging this up but… isn't there any way this could strip out carriage returns and trim indention instead of refusing carriage returns? This gives very long lines

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I recall correctly, it was @nicolas-grekas's idea to forbid these characters, but I can't find his comment (had just a quick look).

IMO, it shouldn't be this long, or otherwise it's too much information...

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the reply, what do you mean by "too much information"? Is there a limit to what the profiler can display? In my case, of I don't provide that much info, I'm afraid people won't be able to migrate w/o opening issues. Plus, it doesn't take much to be above 120 characters given the amount of indention in this tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I know, but IMO a deprecation message should only state that the thing is deprecated (since when), and what to use as a suggestion (not why or how). This should be done in an upgrade file.

But then it's not really an obligation either...

If I remember correctly, the behavior you're suggesting (striping the carriages returns) was what was done in the first place, but @nicolas-grekas wanted to be stricter

Copy link
Contributor

Choose a reason for hiding this comment

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

We do have an upgrade file in our project, so I guess I will put all the details in it

throw new InvalidArgumentException('Invalid characters found in deprecation template.');
}

if (false === strpos($template, '%service_id%')) {
throw new InvalidArgumentException('The deprecation template must contain the "%service_id%" placeholder.');
}

$this->deprecationTemplate = $template;
}

$this->deprecated = (bool) $status;

return $this;
}

/**
* Whether this definition is deprecated, that means it should not be called
* anymore.
Copy link
Member

Choose a reason for hiding this comment

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

On one line, I suggest using a shorter one: Whether this definition is deprecated or not.

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 copied the line from the isAbstract and setAbstract methods, but if you think that's better, I don't mind changing it.

Copy link
Member

Choose a reason for hiding this comment

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

ok then, no need to change

*
* @return bool
*
* @api
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, things shouldn't be tagged as @api in PRs. And, btw, the class is already API so there is also no need.

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 ported the tags from the abstract behaviour, but I can remove those if it's too soon to put them here

*/
public function isDeprecated()
{
return $this->deprecated;
}

/**
* Message to use if this definition is deprecated.
*
* @param string $id Service id relying on this definition
*
* @return string
*
* @api
*/
public function getDeprecationMessage($id)
Copy link
Contributor

Choose a reason for hiding this comment

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

would rename that to $serviceIdor use %service_id% in the other parts

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 used %service_id% in the message template then, but I left$id` as a parameter here, as this is the definition (Service id), this should be explicit enough, as this is what is used in the other places

{
return str_replace('%service_id%', $id, $this->deprecationTemplate);
}

/**
* Sets a configurator to call after the service is fully initialized.
*
Expand Down
10 changes: 10 additions & 0 deletions src/Symfony/Component/DependencyInjection/DefinitionDecorator.php
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,16 @@ public function setDecoratedService($id, $renamedId = null, $priority = 0)
return parent::setDecoratedService($id, $renamedId, $priority);
}

/**
* {@inheritdoc}
*/
public function setDeprecated($boolean = true, $template = null)
{
$this->changes['deprecated'] = true;

return parent::setDeprecated($boolean, $template);
}

/**
* Gets an argument to pass to the service constructor/factory method.
*
Expand Down
14 changes: 13 additions & 1 deletion src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
Original file line number Diff line number Diff line change
Expand Up @@ -592,7 +592,15 @@ private function addService($id, $definition)
$return[] = sprintf("@throws InactiveScopeException when the '%s' service is requested while the '%s' scope is not active", $id, $scope);
}

$return = implode("\n * ", $return);
if ($definition->isDeprecated()) {
if ($return && 0 === strpos($return[count($return) - 1], '@return')) {
$return[] = '';
}

$return[] = sprintf('@deprecated %s', $definition->getDeprecationMessage($id));
}

$return = str_replace("\n * \n", "\n *\n", implode("\n * ", $return));

$doc = '';
if ($definition->isShared() && ContainerInterface::SCOPE_PROTOTYPE !== $scope) {
Expand Down Expand Up @@ -652,6 +660,10 @@ private function addService($id, $definition)
if ($definition->isSynthetic()) {
$code .= sprintf(" throw new RuntimeException('You have requested a synthetic service (\"%s\"). The DIC does not know how to construct this service.');\n }\n", $id);
} else {
if ($definition->isDeprecated()) {
$code .= sprintf(" @trigger_error(%s, E_USER_DEPRECATED);\n\n", var_export($definition->getDeprecationMessage($id), true));
}

$code .=
$this->addServiceInclude($id, $definition).
$this->addServiceLocalTempVariables($id, $definition).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,13 @@ private function addService($definition, $id, \DOMElement $parent)
$service->appendChild($factory);
}

if ($definition->isDeprecated()) {
$deprecated = $this->document->createElement('deprecated');
$deprecated->appendChild($this->document->createTextNode($definition->getDeprecationMessage('%service_id%')));

$service->appendChild($deprecated);
}

if ($callable = $definition->getConfigurator()) {
$configurator = $this->document->createElement('configurator');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ private function addService($id, $definition)
$code .= sprintf(" synchronized: true\n");
}

if ($definition->isDeprecated()) {
$code .= sprintf(" deprecated: %s\n", $definition->getDeprecationMessage('%service_id%'));
}

if ($definition->getFactoryClass(false)) {
$code .= sprintf(" factory_class: %s\n", $definition->getFactoryClass(false));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,10 @@ private function parseDefinition(\DOMElement $service, $file)
$definition->setFile($files[0]->nodeValue);
}

if ($deprecated = $this->getChildren($service, 'deprecated')) {
$definition->setDeprecated(true, $deprecated[0]->nodeValue);
}

$definition->setArguments($this->getArgumentsAsPhp($service, 'argument'));
$definition->setProperties($this->getArgumentsAsPhp($service, 'property'));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,10 @@ private function parseDefinition($id, $service, $file)
$definition->setAbstract($service['abstract']);
}

if (array_key_exists('deprecated', $service)) {
$definition->setDeprecated(true, $service['deprecated']);
}

if (isset($service['factory'])) {
if (is_string($service['factory'])) {
if (strpos($service['factory'], ':') !== false && strpos($service['factory'], '::') === false) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
<xsd:element name="argument" type="argument" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="configurator" type="callable" minOccurs="0" maxOccurs="1" />
<xsd:element name="factory" type="callable" minOccurs="0" maxOccurs="1" />
<xsd:element name="deprecated" type="xsd:string" minOccurs="0" maxOccurs="1" />
Copy link
Member

Choose a reason for hiding this comment

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

It should be a boolean, not a string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has to receive the deprecation message, not a flag ; this is a xml element

<deprecated>This service is deprecated</deprecated>

Copy link
Member

Choose a reason for hiding this comment

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

I was confused by the difference between the YAML and XML syntax (see my other comment).

<xsd:element name="call" type="call" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="tag" type="tag" minOccurs="0" maxOccurs="unbounded" />
<xsd:element name="property" type="property" minOccurs="0" maxOccurs="unbounded" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,36 @@ public function testSetDecoratedServiceOnServiceHasParent()
$this->assertEquals(array('foo', 'foo_inner', 0), $container->getDefinition('child1')->getDecoratedService());
}

public function testDecoratedServiceCopiesDeprecatedStatusFromParent()
{
$container = new ContainerBuilder();
$container->register('deprecated_parent')
->setDeprecated(true)
;

$container->setDefinition('decorated_deprecated_parent', new DefinitionDecorator('deprecated_parent'));

$this->process($container);

$this->assertTrue($container->getDefinition('decorated_deprecated_parent')->isDeprecated());
}

public function testDecoratedServiceCanOverwriteDeprecatedParentStatus()
{
$container = new ContainerBuilder();
$container->register('deprecated_parent')
->setDeprecated(true)
;

$container->setDefinition('decorated_deprecated_parent', new DefinitionDecorator('deprecated_parent'))
->setDeprecated(false)
;

$this->process($container);

$this->assertFalse($container->getDefinition('decorated_deprecated_parent')->isDeprecated());
}

protected function process(ContainerBuilder $container)
{
$pass = new ResolveDefinitionTemplatesPass();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,28 @@ public function testDefinitions()
}
}

public function testCreateDeprecatedService()
{
$definition = new Definition('stdClass');
$definition->setDeprecated(true);

$that = $this;
$wasTriggered = false;

set_error_handler(function ($errno, $errstr) use ($that, &$wasTriggered) {
$that->assertSame(E_USER_DEPRECATED, $errno);
$that->assertSame('The "deprecated_foo" service is deprecated. You should stop using it, as it will soon be removed.', $errstr);
$wasTriggered = true;
});

$builder = new ContainerBuilder();
$builder->createService($definition, 'deprecated_foo');

restore_error_handler();
Copy link
Member

Choose a reason for hiding this comment

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

you'd need to check $this->assertTrue($wasTriggered); here also


$this->assertTrue($wasTriggered);
}

/**
* @covers Symfony\Component\DependencyInjection\ContainerBuilder::register
*/
Expand Down
36 changes: 36 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,42 @@ public function testSetIsAbstract()
$this->assertTrue($def->isAbstract(), '->isAbstract() returns true if the instance must not be public.');
}

/**
* @covers Symfony\Component\DependencyInjection\Definition::setDeprecated
* @covers Symfony\Component\DependencyInjection\Definition::isDeprecated
* @covers Symfony\Component\DependencyInjection\Definition::hasCustomDeprecationTemplate
* @covers Symfony\Component\DependencyInjection\Definition::getDeprecationMessage
*/
public function testSetIsDeprecated()
{
$def = new Definition('stdClass');
$this->assertFalse($def->isDeprecated(), '->isDeprecated() returns false by default');
$this->assertSame($def, $def->setDeprecated(true), '->setDeprecated() implements a fluent interface');
$this->assertTrue($def->isDeprecated(), '->isDeprecated() returns true if the instance should not be used anymore.');
$this->assertSame('The "deprecated_service" service is deprecated. You should stop using it, as it will soon be removed.', $def->getDeprecationMessage('deprecated_service'), '->getDeprecationMessage() should return a formatted message template');
}

/**
* @dataProvider invalidDeprecationMessageProvider
* @covers Symfony\Component\DependencyInjection\Definition::setDeprecated
* @expectedException Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
*/
public function testSetDeprecatedWithInvalidDeprecationTemplate($message)
{
$def = new Definition('stdClass');
$def->setDeprecated(false, $message);
}

public function invalidDeprecationMessageProvider()
{
return array(
"With \rs" => array("invalid \r message %service_id%"),
"With \ns" => array("invalid \n message %service_id%"),
'With */s' => array('invalid */ message %service_id%'),
'message not containing require %service_id% variable' => array('this is deprecated'),
);
}

/**
* @covers Symfony\Component\DependencyInjection\Definition::setConfigurator
* @covers Symfony\Component\DependencyInjection\Definition::getConfigurator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,10 @@
->register('decorator_service_with_name', 'stdClass')
->setDecoratedService('decorated', 'decorated.pif-pouf')
;
$container
->register('deprecated_service', 'stdClass')
->setDeprecated(true)
;
$container
->register('new_factory', 'FactoryClass')
->setProperty('foo', 'bar')
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ digraph sc {
node_decorated [label="decorated\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_decorator_service [label="decorator_service\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_decorator_service_with_name [label="decorator_service_with_name\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_deprecated_service [label="deprecated_service\nstdClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_new_factory [label="new_factory\nFactoryClass\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_factory_service [label="factory_service\nBar\n", shape=record, fillcolor="#eeeeee", style="filled"];
node_new_factory_service [label="new_factory_service\nFooBarBaz\n", shape=record, fillcolor="#eeeeee", style="filled"];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public function __construct()
'decorated' => 'getDecoratedService',
'decorator_service' => 'getDecoratorServiceService',
'decorator_service_with_name' => 'getDecoratorServiceWithNameService',
'deprecated_service' => 'getDeprecatedServiceService',
'factory_service' => 'getFactoryServiceService',
'foo' => 'getFooService',
'foo.baz' => 'getFoo_BazService',
Expand Down Expand Up @@ -143,6 +144,23 @@ protected function getDecoratorServiceWithNameService()
return $this->services['decorator_service_with_name'] = new \stdClass();
}

/**
* Gets the 'deprecated_service' service.
*
* This service is shared.
* This method always returns the same instance of the service.
*
* @return \stdClass A stdClass instance.
*
* @deprecated The "deprecated_service" service is deprecated. You should stop using it, as it will soon be removed.
*/
protected function getDeprecatedServiceService()
{
@trigger_error('The "deprecated_service" service is deprecated. You should stop using it, as it will soon be removed.', E_USER_DEPRECATED);

return $this->services['deprecated_service'] = new \stdClass();
}

/**
* Gets the 'factory_service' service.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public function __construct()
'configured_service' => 'getConfiguredServiceService',
'decorator_service' => 'getDecoratorServiceService',
'decorator_service_with_name' => 'getDecoratorServiceWithNameService',
'deprecated_service' => 'getDeprecatedServiceService',
'factory_service' => 'getFactoryServiceService',
'foo' => 'getFooService',
'foo.baz' => 'getFoo_BazService',
Expand Down Expand Up @@ -144,6 +145,23 @@ protected function getDecoratorServiceWithNameService()
return $this->services['decorator_service_with_name'] = new \stdClass();
}

/**
* Gets the 'deprecated_service' service.
*
* This service is shared.
* This method always returns the same instance of the service.
*
* @return \stdClass A stdClass instance.
*
* @deprecated The "deprecated_service" service is deprecated. You should stop using it, as it will soon be removed.
*/
protected function getDeprecatedServiceService()
{
@trigger_error('The "deprecated_service" service is deprecated. You should stop using it, as it will soon be removed.', E_USER_DEPRECATED);

return $this->services['deprecated_service'] = new \stdClass();
}

/**
* Gets the 'factory_service' service.
*
Expand Down
Loading