[FrameworkBundle] Fixed issue when a parameter contains a '%' #30930

lyrixx · Apr 6, 2019

Q	A
Branch?	master
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#31431
License	MIT
Doc PR

On my computer:

dump(get_cfg_var('xdebug.file_link_format'));
"subl://%f:%l"

When I ran bin/console debug:config framework I got this exception:


In ParameterBag.php line 100:

  [Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException]
  The parameter "templating.helper.code.file_link_format" has a
dependency on a non-existent parameter "f:".

Exception trace:
 () at
/home/gregoire/dev/github.com/lyrixx/symfony/src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php:100
...

This issue was introduced here / cc @ro0NL

This PR does not really fix the issue: I'm able to debug the config, The
the debug:container --env-vars does not work anymore. How could we fix
both issue? cc @nicolas-grekas

nicolas-grekas · Apr 7, 2019

Status: needs work

ro0NL · Apr 8, 2019

Hm, looking at the dumped XML.. i see:

<parameter key="templating.helper.code.file_link_format">subl://%f:%l</parameter>
<parameter key="debug.file_link_format">subl://%f:%l</parameter>

if i escape % manually, it's solved.

ro0NL · Apr 8, 2019

symfony/src/Symfony/Component/DependencyInjection/Dumper/XmlDumper.php

Lines 70 to 72 in f539932

    
           if ($this->container->isCompiled()) { 
        
               $data = $this->escape($data); 
        
           }

shouldnt we always escape at the output level? 🤔

lyrixx · Apr 8, 2019

@ro0NL Unfortunately, the dumped container does not really work. It contains escaped values, and un-escaped values. This is really not simple. IMHO, the simplest (and a bit dirty) solution is to continue in the way I started this PR :/ We should not try to recompile a compiled container, instead we should grep the dumped container, and extract all env var. This PR is almost done, but I'm not able to get the real default value. But I did not get enough time to work on it

ro0NL · Apr 8, 2019

We should not try to recompile a compiled container

but the xml is not compiled yet, at most it's an optimized dump from a compiled source. But IIUC we should be able to compile it again (and get the same result).

from ContainerDebugCommand::getContainerBuilder() is at least weird we compile the container conditionally.

ro0NL · Apr 8, 2019

i mean if we cant load a dumped XML and compile it as such.. that's another bug no?

nicolas-grekas · Apr 8, 2019

I don't think it's a bug, there are tests making it pretty clear it's on purpose: if you dump a compiled container, you should not compile it again. That's the root of the issue here. If you dumped a non-compiled container, then the dumped file should keep references to parameters - thus encode %.
We should build with these constraints in mind - not against them.

ro0NL · Apr 8, 2019

if you dump a compiled container, you should not compile it again

This leaves the container(builder) in PHP uncompiled though (i.e. isCompiled() = false). It feels like a weird discrepancy, where XmlFileLoader should mark the container compiled if it loads a compiled container.

nicolas-grekas · Apr 8, 2019

XmlFileLoader should mark the container compiled if it loads a compiled container.

I agree, would you mind having a look?

ro0NL · Apr 8, 2019

Hm this is hard :) in XML we could consider differentiating based on e.g. compiled="true" .. but what about YAML? This also implies a new feature, as such i think the current dumps are intended to be uncompiled.

If so, we need to find another way to track getEnvCounters() which is perhaps easier :/ (edit: ah i see the current approach is updated already).

nicolas-grekas · Apr 8, 2019

preg_match_all() to the rescue, that's way enough to me for the targeted feature.

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php

src/Symfony/Bundle/FrameworkBundle/Command/ContainerDebugCommand.php

nicolas-grekas · May 9, 2019

@lyrixx can you check this diff I just push-forced? https://github.com/symfony/symfony/compare/f682985882cd36351c55b9da27a16d02e58a6216..f1941605b3d1dc4a8010431b934b0a1d2a5d3fd6
What's missing here?

nicolas-grekas · May 9, 2019

@ro0NL I would appreciate your thoughts here too, since this builds on a PR of yours :)

lyrixx · May 9, 2019

OH, I was working on it ATM !

lyrixx · May 9, 2019

@nicolas-grekas your patch did not work, it did not use the real default value. See my patch, I pushed force too (after you)

lyrixx · May 9, 2019

Looks like everything is OK:

>…/dev/labs/symfony/website-skeleton(4.3-percent *) git di
diff --git a/config/packages/doctrine.yaml b/config/packages/doctrine.yaml
index 9521fad..fc592a8 100644
--- a/config/packages/doctrine.yaml
+++ b/config/packages/doctrine.yaml
@@ -3,7 +3,7 @@ parameters:
     # This allows you to run cache:warmup even if your
     # environment variables are not available yet.
     # You should not need to change this value.
-    env(DATABASE_URL): ''
+    env(DATABASE_URL): 'THE DEFAULT VALUEEEE'
 
 doctrine:
     dbal:
>…/dev/labs/symfony/website-skeleton(4.3-percent *) bin/console  debug:cont --env-vars -v   

Symfony Container Environment Variables
=======================================

 ------------------- ------------------------ ------------------------------------------------------ 
  Name                Default value            Real value                                            
 ------------------- ------------------------ ------------------------------------------------------ 
  APP_SECRET          n/a                      "48a7a9817123a382c84981b98d8748f1"                    
  DATABASE_URL        "THE DEFAULT VALUEEEE"   "mysql://db_user:db_password@127.0.0.1:3306/db_name"  
  MAILER_URL          n/a                      "null://localhost"                                    
  VAR_DUMPER_SERVER   "127.0.0.1:9912"         n/a                                                   
 ------------------- ------------------------ ------------------------------------------------------ 

 // Note real values might be different between web and CLI.                                                            

>…/dev/labs/symfony/website-skeleton(4.3-percent *) DATABASE_URL=HELLO bin/console  debug:cont --env-vars -v

Symfony Container Environment Variables
=======================================

 ------------------- ------------------------ ------------------------------------ 
  Name                Default value            Real value                          
 ------------------- ------------------------ ------------------------------------ 
  APP_SECRET          n/a                      "48a7a9817123a382c84981b98d8748f1"  
  DATABASE_URL        "THE DEFAULT VALUEEEE"   "HELLO"                             
  MAILER_URL          n/a                      "null://localhost"                  
  VAR_DUMPER_SERVER   "127.0.0.1:9912"         n/a                                 
 ------------------- ------------------------ ------------------------------------ 

 // Note real values might be different between web and CLI.

ro0NL · May 9, 2019

You have requested a non-existent parameter "debug.container.dump"

seems now the other case is failing :) not sure if both cases are tested actually. in general the regex approach works for me 👍

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php

lyrixx · May 9, 2019

seems now the other case is failing :) not sure if both cases are tested actually. in general the regex approach works for me 👍

How did you get this error? I saw this is thrown is the CI but did you get it locally?

ro0NL · May 9, 2019

also looking at CI :)

lyrixx · May 10, 2019

I'm fixing some issues, but I don't get why your regex is better. With

        preg_match_all('{([%"])env\(((?:\w++:)*+\w++)\)\1}', $file, $envVars);
        dump($envVars);die;

I got:

array:3 [
  0 => array:8 [
    0 => ""env(DATABASE_URL)""
    1 => "%env(APP_SECRET)%"
    2 => ""env(VAR_DUMPER_SERVER)""
    3 => "%env(APP_SECRET)%"
    4 => "%env(resolve:DATABASE_URL)%"
    5 => "%env(MAILER_URL)%"
    6 => "%env(VAR_DUMPER_SERVER)%"
    7 => "%env(VAR_DUMPER_SERVER)%"
  ]
  1 => array:8 [
    0 => """
    1 => "%"
    2 => """
    3 => "%"
    4 => "%"
    5 => "%"
    6 => "%"
    7 => "%"
  ]
  2 => array:8 [
    0 => "DATABASE_URL"
    1 => "APP_SECRET"
    2 => "VAR_DUMPER_SERVER"
    3 => "APP_SECRET"
    4 => "resolve:DATABASE_URL"
    5 => "MAILER_URL"
    6 => "VAR_DUMPER_SERVER"
    7 => "VAR_DUMPER_SERVER"
  ]
]

Where I got before:

array:2 [
  0 => array:6 [
    0 => "%env(APP_SECRET)%"
    1 => "%env(APP_SECRET)%"
    2 => "%env(resolve:DATABASE_URL)%"
    3 => "%env(MAILER_URL)%"
    4 => "%env(VAR_DUMPER_SERVER)%"
    5 => "%env(VAR_DUMPER_SERVER)%"
  ]
  1 => array:6 [
    0 => "APP_SECRET"
    1 => "APP_SECRET"
    2 => "resolve:DATABASE_URL"
    3 => "MAILER_URL"
    4 => "VAR_DUMPER_SERVER"
    5 => "VAR_DUMPER_SERVER"
  ]
]

Could you give me some example of what I need to match

ro0NL · May 10, 2019

i thought it was nessecary to include default envs, in order to obtain the default values. But now you obtain them directly from the parameter bag, so it's not needed anymore.

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php

On my computer: ``` dump(get_cfg_var('xdebug.file_link_format')); "subl://%f:%l" ``` When I ran `bin/console debug:config framework` I got this exception: ``` In ParameterBag.php line 100: [Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException] The parameter "templating.helper.code.file_link_format" has a dependency on a non-existent parameter "f:". Exception trace: () at /home/gregoire/dev/github.com/lyrixx/symfony/src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php:100 ... ``` This issue was introduced [here](https://github.com/symfony/symfony/pull/27684/files#diff-b3847149480405e1de881530b4c75ab5L212)

lyrixx · May 10, 2019

Here we go ...
I had to move a test because trailing spaces are so boring ...

nicolas-grekas · May 10, 2019

I don't get why your regex is better

because mine is more specific: .* vs (?:\w++:)*+\w++, which matches how EnvParameterBar works.

lyrixx · May 10, 2019

It should be OK. Tests pass locally and I guess it's the expected behavior on a regular app :)
@ro0NL Is it OK for you?

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php

nicolas-grekas · May 11, 2019

Thank you @lyrixx.

…'%' (lyrixx) This PR was merged into the 4.3 branch. Discussion ---------- [FrameworkBundle] Fixed issue when a parameter contains a '%' | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #31431 | License | MIT | Doc PR | --- On my computer: ``` dump(get_cfg_var('xdebug.file_link_format')); "subl://%f:%l" ``` When I ran `bin/console debug:config framework` I got this exception: ``` In ParameterBag.php line 100: [Symfony\Component\DependencyInjection\Exception\ParameterNotFoundException] The parameter "templating.helper.code.file_link_format" has a dependency on a non-existent parameter "f:". Exception trace: () at /home/gregoire/dev/github.com/lyrixx/symfony/src/Symfony/Component/DependencyInjection/ParameterBag/ParameterBag.php:100 ... ``` This issue was introduced [here](https://github.com/symfony/symfony/pull/27684/files#diff-b3847149480405e1de881530b4c75ab5L212) / cc @ro0NL This PR does not really fix the issue: I'm able to debug the config, The the `debug:container --env-vars` does not work anymore. How could we fix both issue? cc @nicolas-grekas Commits ------- 7bcd714 [FrameworkBundle] Fixed issue when a parameter container a '%'

carsonbot added Status: Needs Review FrameworkBundle Bug labels Apr 6, 2019

lyrixx changed the title ~~[WIP] [FrameworkBundle] Fixed issue when a parameter container a '%'~~ [WIP] [FrameworkBundle] Fixed issue when a parameter contains a '%' Apr 6, 2019

nicolas-grekas added this to the next milestone Apr 7, 2019

nicolas-grekas added the ⭐️ EUFOSSA Hackathon label Apr 7, 2019

carsonbot added Status: Needs Work and removed Status: Needs Review labels Apr 7, 2019

ro0NL reviewed Apr 8, 2019

View reviewed changes

fabpot modified the milestones: next, 4.3 May 7, 2019

ro0NL mentioned this pull request May 8, 2019

The parameter "templating.helper.code.file_link_format" has a dependency on a non-existent parameter "f&line=" #31431

Closed

nicolas-grekas changed the base branch from master to 4.3 May 9, 2019 14:01

nicolas-grekas force-pushed the fix-config branch from f682985 to f194160 Compare May 9, 2019 17:12

lyrixx force-pushed the fix-config branch from f194160 to cd9dd43 Compare May 9, 2019 17:21

lyrixx force-pushed the fix-config branch from cd9dd43 to 53b0501 Compare May 9, 2019 17:23

ro0NL reviewed May 9, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php Outdated Show resolved Hide resolved

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php Outdated Show resolved Hide resolved

ro0NL reviewed May 9, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php Outdated Show resolved Hide resolved

nicolas-grekas changed the title ~~[WIP] [FrameworkBundle] Fixed issue when a parameter contains a '%'~~ [FrameworkBundle] Fixed issue when a parameter contains a '%' May 10, 2019

lyrixx force-pushed the fix-config branch from 53b0501 to 0436579 Compare May 10, 2019 08:57

nicolas-grekas reviewed May 10, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php Outdated Show resolved Hide resolved

lyrixx force-pushed the fix-config branch from 0436579 to 7bcd714 Compare May 10, 2019 09:48

ro0NL approved these changes May 10, 2019

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Console/Descriptor/Descriptor.php Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Work labels May 10, 2019

nicolas-grekas approved these changes May 10, 2019

View reviewed changes

nicolas-grekas merged commit 7bcd714 into symfony:4.3 May 11, 2019

lyrixx deleted the fix-config branch May 11, 2019 18:00

fabpot mentioned this pull request May 22, 2019

Release v4.3.0-BETA2 #31577

Merged

ro0NL mentioned this pull request Nov 24, 2019

[ContainerLint] linting process fails when ide option picked under framework bundle #34526

Closed

LANGERGabrielle mentioned this pull request Mar 4, 2022

[FrameworkBundle] Fix container:lint and #[Autoconfigure(binds: ...)] failing #45629

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[FrameworkBundle] Fixed issue when a parameter contains a '%' #30930

[FrameworkBundle] Fixed issue when a parameter contains a '%' #30930

Uh oh!

Conversation

lyrixx commented Apr 6, 2019 • edited by nicolas-grekas Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Apr 7, 2019

Uh oh!

ro0NL commented Apr 8, 2019

Uh oh!

ro0NL commented Apr 8, 2019

Uh oh!

lyrixx commented Apr 8, 2019

Uh oh!

ro0NL commented Apr 8, 2019

Uh oh!

ro0NL commented Apr 8, 2019

Uh oh!

nicolas-grekas commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented Apr 8, 2019

Uh oh!

nicolas-grekas commented Apr 8, 2019

Uh oh!

ro0NL commented Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Apr 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nicolas-grekas commented May 9, 2019

Uh oh!

nicolas-grekas commented May 9, 2019

Uh oh!

lyrixx commented May 9, 2019

Uh oh!

lyrixx commented May 9, 2019

Uh oh!

lyrixx commented May 9, 2019

Uh oh!

ro0NL commented May 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lyrixx commented May 9, 2019

Uh oh!

ro0NL commented May 9, 2019

Uh oh!

lyrixx commented May 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ro0NL commented May 10, 2019

Uh oh!

Uh oh!

lyrixx commented May 10, 2019

Uh oh!

nicolas-grekas commented May 10, 2019

Uh oh!

lyrixx commented May 10, 2019

Uh oh!

Uh oh!

nicolas-grekas commented May 11, 2019

Uh oh!

Uh oh!

lyrixx commented Apr 6, 2019 •

edited by nicolas-grekas

Loading

nicolas-grekas commented Apr 8, 2019 •

edited

Loading

ro0NL commented Apr 8, 2019 •

edited

Loading

lyrixx commented May 10, 2019 •

edited

Loading