-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpKernel] Container class name should start with letter or underscore #20750
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
@@ -479,7 +479,12 @@ protected function initializeBundles() | |||
*/ | |||
protected function getContainerClass() | |||
{ | |||
return $this->name.ucfirst($this->environment).($this->debug ? 'Debug' : '').'ProjectContainer'; | |||
$name = $this->name.ucfirst($this->environment).($this->debug ? 'Debug' : '').'ProjectContainer'; | |||
if (!preg_match('/^[a-zA-Z_\x7f-\xff]/', $name)) { // class name must start with a letter or underscore |
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, what about always adding a static prefix (like Symfony
)? That would avoid executing this regex.
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.
wouldn't it be somehow considered as BC break? the default name (e.g. appDevDebugContainer) is used by e.g. symfony plugin for phpstorm?
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.
To avoid the regex, it can be replaced by the following:
if(false === stripos($name[0], 'abcdefghijklmnopqrstuvwxyz_'))
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.
@jzawadzki what about static prefix app
then?
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.
return '_'.$name; | ||
} | ||
|
||
return $name; |
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.
to be consistent with the method name getContainerClass()
, what do you think about to rename this one to $class
?
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.
👍 (always adding a prefix may break some code - this patch is just making a broken situation work again - much less impact).
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.
👍
Status: reviewed
Note that the regex will be executed twice (at least) per request. I would like to avoid that. |
Would it be acceptable to leave public function getName()
{
if (null === $this->name) {
$dirName = preg_replace('/[^a-zA-Z0-9_]+/', '', basename($this->rootDir));
$this->name = false !== strpos($dirName[0], '0123456789') ? '_'.$dirName : $dirName;
}
return $this->name;
} |
this Pr is a partial fix only. The dumped classes for the cached router also use the kernel name as prefix, and so suffer from the same issue, which is not fixed here. |
$kernel->__construct('dev', false); | ||
|
||
$kernel->boot(); | ||
$this->assertInstanceOf('Symfony\Component\DependencyInjection\ContainerInterface', $kernel->getContainer()); |
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.
would be much better to write the test in a way which does not trigger a fatal error on failure, as this forbids running other tests and to get the proper PHPUnit reporting.
Closing in favor of #21883 |
…arting with a number (fabpot) This PR was merged into the 2.7 branch. Discussion ---------- [HttpKernel] fix Kernel name when stored in a directory starting with a number | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20489 | License | MIT | Doc PR | - replaces #20750 Commits ------- f244eb8 [HttpKernel] fixed Kernel name when stored in a directory starting with a number
Hi all,
this PR tries to fix (rather rare) issue when generated Container Class name is starting with number (which is the case when kernel name [base name of root dir] starts with one).
Not sure if my solution is correct - it's the easiest I could think off: it just adds underscore in front of the generated class name.