Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] deprecate most characters in service ids#18028
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedMar 6, 2016
| Q | A |
|---|---|
| Branch | master |
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | yes |
| Tests pass? | yes |
| Fixed tickets | #17801 |
| License | MIT |
| Doc PR | TODO |
| publicfunctionsetAlias($alias,$id) | ||
| { | ||
| if (preg_match('/[^a-z0-9\._]/i',$alias)) { | ||
| trigger_error(sprintf('Using characters other than A-Z, 0-9, ., and _ in service ids ("%s" given) is deprecated since Symfony 3.1 and will throw an exception in 4.0.',$alias),E_USER_DEPRECATED); |
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.
Should be silenced
524a810 tob33c4edComparexabbuh commentedMar 7, 2016
Comments addressed, tests fixed. Status: Needs Review |
| publicfunctionset($id,$service) | ||
| { | ||
| if (preg_match('/[^a-z0-9\._%]/i',$id)) { | ||
| @trigger_error(sprintf('Using characters other than A-Z, 0-9, ., and _ in service ids ("%s" given) is deprecated since Symfony 3.1 and will throw an exception in 4.0.',$id),E_USER_DEPRECATED); |
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.
Instead of:
... and will throw an exception in 4.0.We use this other wording in most deprecation notices:
... and will be removed in 4.0.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.
Yeah, but that sounds really weird in this context (for example, in the Yaml component we used "will throw a ParseException" too).
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.
OK. It makes sense. Thanks.
javiereguiluz commentedMar 10, 2016
👍 Status: Reviewed |
nicolas-grekas commentedMar 10, 2016
should we allow non western users to use utf8 to name their services? I'm sure some do and I don't see anything wrong. wdyt? |
xabbuh commentedMar 10, 2016
Can we make sure that non of them will cause trouble in the future? |
GuilhemN commentedMar 11, 2016
What do you think about allowing |
javiereguiluz commentedMar 12, 2016
@xabbuh given the comments made by some reviewers (support non-English UTF8 chars, support slashes and backslashes, etc.) what do you think about switching to a blacklist filter instead of the current whitelist filter? Let's ban some concrete chars ( |
xabbuh commentedMar 14, 2016
@javiereguiluz We could do that too. The list should then probably contain whitespace characters, control sequences, |
javiereguiluz commentedMar 14, 2016
@xabbuh I can't think of anything else. Your list looks comprehensive. Thanks! |
1e1f353 to097ae80Comparexabbuh commentedMar 14, 2016
changed the PR to use a blacklist instead of a whitelist |
stof commentedMar 14, 2016
the whitelist should be built in a way where the generated method names for the dumping of the container generates a valid name (which means adding more stuff to the allowed whitelist). |
GuilhemN commentedMar 14, 2016
@stof isn't it possible to convert the services' id to another base to have only supported characters (maybe using Edit: at worst, if we really want to keep descriptive name as much as possible, we could solve the actual limitation of the dumper by having a counter which would be incremented each time a new unsupported service id is camelized. |
GuilhemN commentedMar 14, 2016
Can you take a look at#18167 please? |
097ae80 to8bbb655Comparefabpot commentedApr 1, 2016
#18167 has been merged now |
8bbb655 tod06bb91Comparexabbuh commentedApr 6, 2016
rebased here ping @symfony/deciders |
GuilhemN commentedApr 6, 2016
What about deprecating an empty string as service id at the same time ? (see#18265 (comment)) |
| $definition->setPublic($public); | ||
| $definition->addTag('console.command'); | ||
| $container->setDefinition('my-command',$definition); | ||
| $container->setDefinition('app.command',$definition); |
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.
I don't get why we need to reject-? Could we allow it?
stof commentedApr 8, 2016
Do we actually still need this ? |
fabpot commentedApr 28, 2016
Indeed, let's close this then. |