8000 [HttpKernel] Container class name should start with letter or underscore by jzawadzki · Pull Request #20750 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 4 commits into from
Closed

[HttpKernel] Container class name should start with letter or underscore #20750

wants to merge 4 commits into from

Conversation

jzawadzki
Copy link
Contributor
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 -

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.

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

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.

Copy link
Contributor Author

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?

Copy link
Member
@GromNaN GromNaN Dec 5, 2016

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_'))

Copy link
Member
@yceruto yceruto Dec 6, 2016

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yceruto - I would like to avoid having default appappDevDebugProjectContainer
@fabpot - what if we would only add Symfony prefix if $this->name is different then app? then we would remove regex and let default to be appDevDebugProjectContainer

return '_'.$name;
}

return $name;
Copy link
Member
@yceruto yceruto Dec 6, 2016

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 ?

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.

👍 (always adding a prefix may break some code - this patch is just making a broken situation work again - much less impact).

Copy link
Member
@javiereguiluz javiereguiluz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Status: reviewed

@fabpot
Copy link
Member
fabpot commented Feb 22, 2017

Note that the regex will be executed twice (at least) per request. I would like to avoid that.

@javiereguiluz
Copy link
Member

Would it be acceptable to leave getContainerClass() untouched and modify the getName() as follows?

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;
}

@stof
Copy link
Member
stof commented Feb 22, 2017

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());
Copy link
Member

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.

@javiereguiluz
Copy link
Member

@stof would 3f88f35 solve the issues that you pointed? Thanks!

@fabpot
Copy link
Member
fabpot commented Mar 5, 2017

Closing in favor of #21883

@fabpot fabpot closed this Mar 5, 2017
fabpot added a commit that referenced this pull request Mar 6, 2017
…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
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