8000 Add support for deprecated definitions by Taluu · Pull Request #15491 · symfony/symfony · GitHub
[go: up one dir, main page]

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 1 commit
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
Prev Previous commit
Next Next commit
[DI] Allow to change the deprecation message in Definition
  • Loading branch information
Taluu committed Sep 24, 2015
commit 0b3d0a0bd9c7ada0933f97f2bad16ac95546f9a9
Original file line number Diff line number Diff line change
Expand Up @@ -932,7 +932,7 @@ public function createService(Definition $definition, $id, $tryProxy = true)
}

if ($definition->isDeprecated()) {
@trigger_error(sprintf('The service %s relies on a deprecated definition. You should avoid using it.', $id), E_USER_DEPRECATED);
@trigger_error($definition->getDeprecationMessage($id), E_USER_DEPRECATED);
}

if ($tryProxy && $definition->isLazy()) {
Expand Down
36 changes: 33 additions & 3 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 All @@ -38,7 +40,6 @@ class Definition
private $public = true;
private $synthetic = false;
private $abstract = false;
private $deprecated = false;
private $synchronized = false;
private $lazy = false;
private $decoratedService;
Expand Down Expand Up @@ -834,14 +835,29 @@ public function isAbstract()
* Whether this definition is deprecated, that means it should not be called
* anymore.
*
* @param bool $status
* @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)
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;
Expand All @@ -860,6 +876,20 @@ 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
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,7 @@ private function addService($id, $definition)
$return[] = '';
}

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

$return = str_replace("\n * \n", "\n *\n", implode("\n * ", $return));
Expand Down Expand Up @@ -661,7 +661,7 @@ private function addService($id, $definition)
$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('The service %s has been marked as deprecated. You should stop using it.', E_USER_DEPRECATED);\n\n", $id);
$code .= sprintf(" @trigger_error(%s, E_USER_DEPRECATED);\n\n", var_export($definition->getDeprecationMessage($id), true));
}

$code .=
Expand Down
10 changes: 7 additions & 3 deletions src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php
Original file line number Diff line number Diff line change
Expand Up @@ -148,9 +148,6 @@ private function addService($definition, $id, \DOMElement $parent)
if ($definition->isLazy()) {
$service->setAttribute('lazy', 'true');
}
if ($definition->isDeprecated()) {
$service->setAttribute('deprecated', 'true');
}
if (null !== $decorated = $definition->getDecoratedService()) {
list($decorated, $renamedId, $priority) = $decorated;
$service->setAttribute('decorates', $decorated);
Expand Down Expand Up @@ -204,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 @@ -105,7 +105,7 @@ private function addService($id, $definition)
}

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

if ($definition->getFactoryClass(false)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ private function parseDefinition(\DOMElement $service, $file)
$definition = new Definition();
}

foreach (array('class', 'shared', 'public', 'factory-class', 'factory-method', 'factory-service', 'synthetic', 'lazy', 'abstract', 'deprecated') as $key) {
foreach (array('class', 'shared', 'public', 'factory-class', 'factory-method', 'factory-service', 'synthetic', 'lazy', 'abstract') as $key) {
if ($value = $service->getAttribute($key)) {
if (in_array($key, array('factory-class', 'factory-method', 'factory-service'))) {
@trigger_error(sprintf('The "%s" attribute of service "%s" in file "%s" is deprecated since version 2.6 and will be removed in 3.0. Use the "factory" element instead.', $key, (string) $service->getAttribute('id'), $file), E_USER_DEPRECATED);
Expand Down 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,8 +196,8 @@ private function parseDefinition($id, $service, $file)
$definition->setAbstract($service['abstract']);
}

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

if (isset($service['factory'])) {
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 All @@ -94,7 +95,6 @@
<xsd:attribute name="synchronized" type="boolean" />
<xsd:attribute name="lazy" type="boolean" />
<xsd:attribute name="abstract" type="boolean" />
<xsd:attribute name="deprecated" type="boolean" />
<xsd:attribute name="factory-class" type="xsd:string" />
<xsd:attribute name="factory-method" type="xsd:string" />
<xsd:attribute name="factory-service" type="xsd:string" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public function testCreateDeprecatedService()

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

Expand Down
24 changes: 24 additions & 0 deletions src/Symfony/Component/DependencyInjection/Tests/DefinitionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -236,13 +236,37 @@ public function testSetIsAbstract()
/**
* @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'),
);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,11 +152,11 @@ protected function getDecoratorServiceWithNameService()
*
* @return \stdClass A stdClass instance.
*
* @deprecated
* @deprecated The "deprecated_service" service is deprecated. You should stop using it, as it will soon be removed.
*/
protected function getDeprecatedServiceService()
{
@trigger_error('The service deprecated_service has been marked as deprecated. You should stop using it.', E_USER_DEPRECATED);
@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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -153,11 +153,11 @@ protected function getDecoratorServiceWithNameService()
*
* @return \stdClass A stdClass instance.
*
* @deprecated
* @deprecated The "deprecated_service" service is deprecated. You should stop using it, as it will soon be removed.
*/
protected function getDeprecatedServiceService()
{
@trigger_error('The service deprecated_service has been marked as deprecated. You should stop using it.', E_USER_DEPRECATED);
@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();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,9 @@
<service id="decorated" class="stdClass"/>
<service id="decorator_service" class="stdClass" decorates="decorated"/>
<service id="decorator_service_with_name" class="stdClass" decorates="decorated" decoration-inner-name="decorated.pif-pouf"/>
<service id="deprecated_service" class="stdClass" deprecated="true"/>
<service id="deprecated_service" class="stdClass">
<deprecated>The "%service_id%" service is deprecated. You should stop using it, as it will soon be removed.</deprecated>
</service>
<service id="new_factory" class="FactoryClass" public="false">
<property name="foo">bar</property>
</service>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ services:
decoration_inner_name: decorated.pif-pouf
deprecated_service:
class: stdClass
deprecated: true
deprecated: The "%service_id%" service is deprecated. You should stop using it, as it will soon be removed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add test with deprecated: true (without overwrite default deprecation message)?

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 already supports the null value (or ~), but I can also add true

Copy link
Member

Choose a reason for hiding this comment

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

If we give a special meaning to "true", then we should also add one to "false". But we don't want to allow "false". Thus I wouldn't allow "true". The expected value is either a deprecation message template, or null (which is not recommended: the best practice is to set a more contextual message)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough.

new_factory:
class: FactoryClass
public: false
Expand Down
0