Added normalized env getter in kernel for correct container compile. #25915

ed-fruty · 2018-01-24T14:27:41Z

Q	A
Branch?	4.0
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	symfony/symfony-docs#

I don't think it's a big bug, but it's a case when symfony wasn't work properly.
When you have a specific env name, you can't even install symfony correctly.
Steps to reproduce:

export APP_ENV=dev-server
composer create-project symfony/skeleton symfony_will_crash

And you will get an error [KO] [KO] Script cache:clear returned with error code 255 !! PHP Fatal error: Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Undefined constant 'ContainerCPr692s\srcDev' in /home/project/symfony_will_crash/var/cache/dev-server/srcDev-serverDebugProjectContainer.php:5.

Full installation log will be like this:

Installing symfony/skeleton (v4.0.5)
  - Installing symfony/skeleton (v4.0.5): Loading from cache
Created project in symfony_will_crash
Loading composer repositories with package information
Updating dependencies (including require-dev)
Package operations: 21 installs, 0 updates, 0 removals
  - Installing symfony/flex (v1.0.66): Loading from cache
  - Installing symfony/polyfill-mbstring (v1.6.0): Loading from cache
  - Installing symfony/console (v4.0.3): Loading from cache
  - Installing symfony/routing (v4.0.3): Loading from cache
  - Installing symfony/http-foundation (v4.0.3): Loading from cache
  - Installing symfony/yaml (v4.0.3): Loading from cache
  - Installing symfony/framework-bundle (v4.0.3): Loading from cache
  - Installing symfony/http-kernel (v4.0.3): Loading from cache
  - Installing symfony/event-dispatcher (v4.0.3): Loading from cache
  - Installing psr/log (1.0.2): Loading from cache
  - Installing symfony/debug (v4.0.3): Loading from cache
  - Installing symfony/finder (v4.0.3): Loading from cache
  - Installing symfony/filesystem (v4.0.3): Loading from cache
  - Installing psr/container (1.0.0): Loading from cache
  - Installing symfony/dependency-injection (v4.0.3): Loading from cache
  - Installing symfony/config (v4.0.3): Loading from cache
  - Installing psr/simple-cache (1.0.0): Loading from cache
  - Installing psr/cache (1.0.1): Loading from cache
  - Installing symfony/cache (v4.0.3): Loading from cache
  - Installing symfony/dotenv (v4.0.3): Loading from cache
Writing lock file
Generating autoload files
Symfony operations: 4 recipes (2129a2681f34fe264097ab9136e6141a)
  - Configuring symfony/flex (>=1.0): From github.com/symfony/recipes:master
  - Configuring symfony/framework-bundle (>=3.3): From github.com/symfony/recipes:master
  - Configuring symfony/console (>=3.3): From github.com/symfony/recipes:master
  - Configuring symfony/routing (>=4.0): From github.com/symfony/recipes:master
Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 255
!!  PHP Fatal error:  Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Undefined constant 'ContainerCPr692s\srcDev' in /home/project/symfony_will_crash/var/cache/dev-server/srcDev-serverDebugProjectContainer.php:5
!!  Stack trace:
...

The main thing is compiler tries to create container with filename and classname uses our $APP_ENV and in this case it will be srcDev-serverDebugProjectContainer which is not valid for php syntax.

One of suggestions is always to use normalized environment name. In our case it will be devserver.
Another one is throw an exception if env is not valid.
What do think ?

ed-fruty · 2018-01-24T14:35:58Z

Regex can be changed for something like /[^\w]+/i or other.

nicolas-grekas · 2018-01-25T08:38:46Z

Thanks for the report. Envs have implicit requirements, good idea to enforce them.
Adding a new protected method is a no-go as we don't want this to be extensible.
But normalizing inline when setting the env /OR/ throwing an InvalidArgumentException is legit IMHO.
I'm not sure about the proposed regexp, how did you design it?

dunglas · 2018-01-26T14:56:55Z

Good catch, the format should be enforced, this kind of issues is a pain to debug, especially for newcomers. I suggest to throw an exception specifying the valid characters instead of normalizing (the normalization introduces some complexity, and probably some more WTF later).

ed-fruty · 2018-01-27T21:20:18Z

@dunglas, @nicolas-grekas I suggest to normalize environment value instead of exception.
I reviewed kernel code again and found almost the same normalization for kernel name getter.

So, it would be strangeous behaviour when in one place would be normalization and in another one - exception.

Can I extract normalization for this two places in separate private method and then update this PR ?

nicolas-grekas · 2018-01-30T07:59:17Z

@ed-fruty go on my side for normalization, but with a BC-safe regular expression:
$this->environment = preg_replace('/[^a-zA-Z0-9_\x7f-\xff]/', '', $environment);
the normalization in place for the name should not be changed, and thus no need for any private method, just this line in the constructor is enough IMHO.

nicolas-grekas · 2018-01-30T08:02:03Z

wait, actually this patch would be better:

--- a/src/Symfony/Component/HttpKernel/Kernel.php
+++ b/src/Symfony/Component/HttpKernel/Kernel.php
@@ -480,7 +480,7 @@ abstract class Kernel implements KernelInterface, TerminableInterface
      */
     protected function getContainerClass()
     {
-        return $this->name.ucfirst($this->environment).($this->debug ? 'Debug' : '').'ProjectContainer';
+        return $this->name.ucfirst(preg_replace('/[^a-zA-Z0-9_\x7f-\xff]/', '', $this->environment)).($this->debug ? 'Debug' : '').'ProjectContainer';
     }
 
     /**

ed-fruty · 2018-01-30T09:35:21Z

Updated PR.
As I understood checks were failed due to http connection errors.

nicolas-grekas

Looks good to me at the fix side. Tests need to be implemented, and this PR should be retargetted/rebased at branch 2.7, where the issue also exists. As PHP 5.3 is the minimum version there, PHP 7 syntax cannot be used.

nicolas-grekas · 2018-01-30T11:05:08Z

src/Symfony/Component/HttpKernel/Tests/KernelTest.php

@@ -523,6 +523,44 @@ public function testProjectDirExtension()
        $this->assertSame('foo', $kernel->getContainer()->getParameter('kernel.project_dir'));
    }

+    /**
+     * @param $env
+     * @param $expected


pleas remove both @param

nicolas-grekas · 2018-01-30T11:05:24Z

src/Symfony/Component/HttpKernel/Tests/KernelTest.php

+        $kernel = new class($env, true) extends Kernel {
+            public function registerContainerConfiguration(LoaderInterface $loader)
+            {
+                // TODO: Implement registerContainerConfiguration() method.


TODO :) (same below)

Tobion · 2018-02-01T01:43:48Z

The environment is also used for the cache folder. So certain special characters will make this fail as well, esp. on Windows. So that sohuld be fixed as well if we want make env chars failure tolerant.

ed-fruty · 2018-02-02T16:01:51Z

I create my branch from 4.0 and now I can't rebase it to 2.7. I getting errors in git, related to unresolved conflicts...
Can I create another one branch based on 2.7 with my fixes and create another PR ?

nicolas-grekas · 2018-02-03T10:54:38Z

Can I create another one branch based on 2.7 with my fixes and create another PR ?

sure, if you're more comfortable this way no pb!

nicolas-grekas · 2018-03-19T20:36:19Z

Closing as this needs to be submitted on 2.7 (and tests be updated to work on PHP 5.3)
@ed-fruty waiting for your PR :)

Added normalized env getter in kernel for correct container compile.

30a4e8f

carsonbot added Status: Needs Review Bug labels Jan 24, 2018

ed-fruty changed the base branch from master to 4.0 January 24, 2018 14:27

ed-fruty added 2 commits January 24, 2018 16:38

Fixed fabbot.io style checks.

d84ff0e

Fixed fabbot.io style checks.

9efb708

nicolas-grekas added this to the 2.7 milestone Jan 25, 2018

Normalized container classname in kernel.

8f33598

nicolas-grekas requested changes Jan 30, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 30, 2018

Changed tests for container class normalization to support php5.3.

5cab56b

ed-fruty changed the base branch from 4.0 to 2.7 February 2, 2018 13:59

ed-fruty changed the base branch from 2.7 to 4.0 February 2, 2018 14:27

nicolas-grekas changed the base branch from 4.0 to 2.7 March 19, 2018 20:27

nicolas-grekas changed the base branch from 2.7 to 4.0 March 19, 2018 20:34

nicolas-grekas closed this Mar 19, 2018

Tobion mentioned this pull request Apr 11, 2018

Clearing the cache fails on environment with a hyphen #26884

Closed

xabbuh mentioned this pull request Apr 24, 2018

[HttpKernel] Normalized container classname in kernel #27029

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Added normalized env getter in kernel for correct container compile. #25915

Added normalized env getter in kernel for correct container compile. #25915

Uh oh!

ed-fruty commented Jan 24, 2018

Uh oh!

ed-fruty commented Jan 24, 2018

Uh oh!

nicolas-grekas commented Jan 25, 2018

Uh oh!

dunglas commented Jan 26, 2018 •

edited

Loading

Uh oh!

ed-fruty commented Jan 27, 2018 •

edited

Loading

Uh oh!

nicolas-grekas commented Jan 30, 2018

Uh oh!

nicolas-grekas commented Jan 30, 2018

Uh oh!

ed-fruty commented Jan 30, 2018 •

edited

Loading

Uh oh!

nicolas-grekas left a comment

Uh oh!

nicolas-grekas Jan 30, 2018

Uh oh!

nicolas-grekas Jan 30, 2018

Uh oh!

Tobion commented Feb 1, 2018

Uh oh!

ed-fruty commented Feb 2, 2018

Uh oh!

nicolas-grekas commented Feb 3, 2018

Uh oh!

nicolas-grekas commented Mar 19, 2018

Uh oh!

Uh oh!

Uh oh!

Added normalized env getter in kernel for correct container compile. #25915

Added normalized env getter in kernel for correct container compile. #25915

Uh oh!

Conversation

ed-fruty commented Jan 24, 2018

Uh oh!

ed-fruty commented Jan 24, 2018

Uh oh!

nicolas-grekas commented Jan 25, 2018

Uh oh!

dunglas commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ed-fruty commented Jan 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Jan 30, 2018

Uh oh!

nicolas-grekas commented Jan 30, 2018

Uh oh!

ed-fruty commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jan 30, 2018

Choose a reason for hiding this comment

Uh oh!

Tobion commented Feb 1, 2018

Uh oh!

ed-fruty commented Feb 2, 2018

Uh oh!

nicolas-grekas commented Feb 3, 2018

Uh oh!

nicolas-grekas commented Mar 19, 2018

Uh oh!

Uh oh!

dunglas commented Jan 26, 2018 •

edited

Loading

ed-fruty commented Jan 27, 2018 •

edited

Loading

ed-fruty commented Jan 30, 2018 •

edited

Loading