-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Changes from 1 commit
4b6fab0
8f6c21c
2f37cb1
954247d
0b3d0a0
83f4e9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
|
@@ -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; | ||
|
@@ -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)) { | ||
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; | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would rename that to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I used |
||
{ | ||
return str_replace('%service_id%', $id, $this->deprecationTemplate); | ||
} | ||
|
||
/** | ||
* Sets a configurator to call after the service is fully initialized. | ||
* | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be a boolean, not a string. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" /> | ||
|
@@ -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" /> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add test with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It already supports the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough. |
||
new_factory: | ||
class: FactoryClass | ||
public: false | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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