Improved the error message when using "@" in a decorated service #17687
Conversation
javiereguiluz
commented
Feb 4, 2016
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #17666 |
| License | MIT |
| Doc PR | - |
|
👍 |
|
👍 (I made this mistake recently). |
| @@ -288,6 +288,10 @@ private function parseDefinition($id, $service, $file) | |||
| } | |||
|
|
|||
| if (isset($service['decorates'])) { | |||
| if ('@' === $service['decorates']{0}) { | |||
There was a problem hiding this comment.
Could $service['decorates'] be empty here?
There was a problem hiding this comment.
and by the way, I don't think we use braces anywhere else to access chars in string (everywhere else we use square brackets instead IIRC)
|
I've implemented the changes asked by reviewers. This is now ready for the final review. Thanks! |
| @@ -288,6 +288,10 @@ private function parseDefinition($id, $service, $file) | |||
| } | |||
|
|
|||
| if (isset($service['decorates'])) { | |||
| if ('' !== $service['decorates'] && '@' === $service['decorates'][0]) { | |||
| throw new InvalidArgumentException(sprintf('The value of the "decorates" option for the "%s" service must be the id of the service without the "@" prefix (replace "%s" by "%s").', $id, $service['decorates'], substr($service['decorates'], 1))); | |||
|
I left a minor comment, but beside that 👍 |
|
AFAIK you can technically have a service id like |
|
If I'm right, we need a different solution. |
|
@Tobion IMO, the right solution is to restrict the list of allowed chars in service ids to chars working everywhere (anything not working for the PhpDumper means it cannot work in a project wanting performance, as it would prevent dumping the container in the cache). This could start as a deprecation for now though |
|
Yes that is probably the easiest solution. Another one would be the remove this restriction in the PhpDumper by encoding special chars (hex for example). And IMO the better solution to this is issue here is to improve the real exception message in the ContainerBuilder: mention which service is decorating the non-existing one and maybe add suggestions for existing ones (like it is done for console commands). This would also work if we remove the restriction on service ids. |
|
I'm 👍 to merge this one as is (even if technically, a service name can start with |
|
Yes, I'll take care of that in the new issue #17801. |
|
But wouldn't it indeed be better to raise this exception only in the |
|
@xabbuh I think it's better to throw the exception as early as possible; it is a YAML issue, so it should probably be dealt with there. |
|
@fabpot The only thing is that technically, you can have a service id starting with But anyway, I think it makes sense to restrict the list of allowed characters and deprecate everything else. So 👍 for doing this check for the YAML part only. |
|
Thank you @javiereguiluz. |
…ervice (javiereguiluz) This PR was squashed before being merged into the 2.7 branch (closes #17687). Discussion ---------- Improved the error message when using "@" in a decorated service | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17666 | License | MIT | Doc PR | - Commits ------- e7690ba Improved the error message when using "@" in a decorated service