Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:4.3fromlyrixx:fix-config
May 11, 2019

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedApr 6, 2019•
edited by nicolas-grekas
Loading

QA
Branch?master
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#31431
LicenseMIT
Doc PR

On my computer:

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

When I ranbin/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 adependency 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 introducedhere / cc@ro0NL

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

@lyrixxlyrixx 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-grekasnicolas-grekas added this to thenext milestoneApr 7, 2019
@nicolas-grekas
Copy link
Member

Status: needs work

@ro0NL
Copy link
Contributor

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

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

if i escape% manually, it's solved.

@ro0NL
Copy link
Contributor

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

shouldnt we always escape at the output level? 🤔

@lyrixx
Copy link
MemberAuthor

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

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).

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

@ro0NL
Copy link
Contributor

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

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 8, 2019•
edited
Loading

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

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

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

I agree, would you mind having a look?

@ro0NL
Copy link
Contributor

ro0NL commentedApr 8, 2019•
edited
Loading

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 trackgetEnvCounters() which is perhaps easier :/ (edit: ah i see the current approach is updated already).

@nicolas-grekas
Copy link
Member

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

@nicolas-grekas
Copy link
Member

@nicolas-grekas
Copy link
Member

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

@lyrixx
Copy link
MemberAuthor

OH, I was working on it ATM !

@lyrixx
Copy link
MemberAuthor

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

Looks like everything is OK:

>…/dev/labs/symfony/website-skeleton(4.3-percent *) git didiff --git a/config/packages/doctrine.yaml b/config/packages/doctrine.yamlindex 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 -vSymfony 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
Copy link
Contributor

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 👍

@lyrixx
Copy link
MemberAuthor

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

also looking at CI :)

@nicolas-grekasnicolas-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
Copy link
MemberAuthor

lyrixx commentedMay 10, 2019•
edited
Loading

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

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.

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 adependency 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
Copy link
MemberAuthor

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

@nicolas-grekas
Copy link
Member

I don't get why your regex is better

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

ro0NL and lyrixx reacted with thumbs up emoji

@lyrixx
Copy link
MemberAuthor

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?

@nicolas-grekas
Copy link
Member

Thank you@lyrixx.

@nicolas-grekasnicolas-grekas merged commit7bcd714 intosymfony:4.3May 11, 2019
nicolas-grekas added a commit that referenced this pull requestMay 11, 2019
…'%' (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 adependency 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@ro0NLThis PR does not really fix the issue: I'm able to debug the config, Thethe `debug:container --env-vars` does not work anymore. How could we fixboth issue? cc@nicolas-grekasCommits-------7bcd714 [FrameworkBundle] Fixed issue when a parameter container a '%'
@lyrixxlyrixx deleted the fix-config branchMay 11, 2019 18:00
@fabpotfabpot mentioned this pull requestMay 22, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@ro0NLro0NLro0NL approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

5 participants

@lyrixx@nicolas-grekas@ro0NL@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp