-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Normalized container classname in kernel #27029
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
Conversation
Q | A |
---|---|
Branch? | 2.7 |
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tests pass? | yes |
Fixed tickets | #25915, #26884 |
License | MIT |
Doc PR |
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.
(although the env name should only contain a-z_ imho)
I agree with @nicolas-grekas |
Done, but shouldn't we allow digits too? |
Actually, did it ever work? |
@@ -480,7 +480,7 @@ protected function initializeBundles() | |||
*/ | |||
protected function getContainerClass 8000 () | |||
{ | |||
return $this->name.ucfirst($this->environment).($this->debug ? 'Debug' : '').'ProjectContainer'; | |||
return $this->name.ucfirst(preg_replace('/[^a-zA-Z0-9_]/', '', $this->environment)).($this->debug ? 'Debug' : '').'ProjectContainer'; |
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.
the regex about valid chars in class names is broader than that
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.
The official regexp for class names is ^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$
(see http://php.net/manual/en/language.oop5.basic.php) so this part should be [^a-zA-Z0-9_\x7f-\xff]
instead of [^a-zA-Z0-9_]
... but I don't think we should do this change. The probability of causing a clash because two weird env names have been stripped from wrong characters is negligible.
What are the next steps for this one? /cc @xabbuh |
We will need to agree on the regex to be used. It looks like @nicolas-grekas and @stof have different opinions on what would be the best we should use. |
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.
LGTM as is. Env names should really match this simple regex. If one wants to be more creative, that's not a problem either (we will just strip more chars) except if two env names would return the same value (highly improbable).
This reminds me #13412 |
Re-reading, I'm in favor of throwing an exception, since it never worked. |
If we were to merge this one, I think it should be on master. |
I'm closing since getName has been deprecated on master meanwhile. Thanks for helping move the discussion forward! |