8000 [HttpKernel] Normalized container classname in kernel by xabbuh · Pull Request #27029 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 3 commits into from

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented Apr 24, 2018
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

Copy link
Member
@nicolas-grekas nicolas-grekas left a 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)

@fabpot
Copy link
Member
fabpot commented Apr 25, 2018

I agree with @nicolas-grekas

@xabbuh
Copy link
Member Author
xabbuh commented Apr 25, 2018

Done, but shouldn't we allow digits too?

@nicolas-grekas
Copy link
Member

Actually, did it ever work?
If not, we might throw instead?
(you're right for digits)

@@ -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';
Copy link
Member

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

Copy link
Member

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.

@fabpot fabpot modified the milestones: 2.7, 2.8 May 28, 2018
@fabpot
Copy link
Member
fabpot commented May 28, 2018

What are the next steps for this one? /cc @xabbuh

@xabbuh
Copy link
Member Author
xabbuh commented May 28, 2018

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.

Copy link
Member
@fabpot fabpot left a 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).

@lyrixx
Copy link
Member
lyrixx commented May 28, 2018

This reminds me #13412

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Jul 7, 2018

Re-reading, I'm in favor of throwing an exception, since it never worked.
Right now this throws a fatal error isn't it? Let's just improve the experience without introducing any "new feature" ("make it work").

@fabpot
Copy link
Member
fabpot commented Oct 10, 2018

If we were to merge this one, I think it should be on master.

@nicolas-grekas
Copy link
Member

I'm closing since getName has been deprecated on master meanwhile. Thanks for helping move the discussion forward!

@xabbuh xabbuh deleted the pr-25915 branch October 21, 2018 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
0