Skip to content
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

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

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@lyrixx
Copy link
Member

lyrixx commented Apr 6, 2019

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
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

[FrameworkBundle] Fixed issue when a parameter container a '%'
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 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 nicolas-grekas added this to the next milestone Apr 7, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 7, 2019

Status: needs work

--
@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented 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

This comment has been minimized.

Copy link
Contributor

ro0NL commented Apr 8, 2019

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

shouldnt we always escape at the output level? 🤔

@lyrixx

This comment has been minimized.

Copy link
Member Author

lyrixx commented 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

This comment has been minimized.

Copy link
Contributor

ro0NL commented 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

This comment has been minimized.

Copy link
Contributor

ro0NL commented Apr 8, 2019

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented 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

This comment has been minimized.

Copy link
Contributor

ro0NL commented 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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 8, 2019

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

I agree, would you mind having a look?

@ro0NL

This comment has been minimized.

Copy link
Contributor

ro0NL commented 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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 8, 2019

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

@@ -322,15 +322,21 @@ public static function getClassDescription(string $class, string &$resolvedClass
private function getContainerEnvVars(ContainerBuilder $container): array
{
$file = file_get_contents($container->getParameter('debug.container.dump'));

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 8, 2019

Contributor

id would preserve the getEnvCounters() if the container is compiled

$processor = 'string';
if (false !== $i = strrpos($name = $env, ':')) {
$name = substr($env, $i + 1);
$processor = substr($env, 0, $i);
}
// dump($container->getParameter('env(DATABASE_URL)'));die;

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 8, 2019

Contributor

you should capture e.g. <parameter key="env(DATABASE_URL)"></parameter> as well

@@ -223,15 +223,14 @@ protected function getContainerBuilder()
if (!$kernel->isDebug() || !(new ConfigCache($kernel->getContainer()->getParameter('debug.container.dump'), true))->isFresh()) {
$buildContainer = \Closure::bind(function () { return $this->buildContainer(); }, $kernel, \get_class($kernel));
$container = $buildContainer();
$container->getCompilerPassConfig()->setRemovingPasses([]);
$container->compile();
} else {
(new XmlFileLoader($container = new ContainerBuilder(), new FileLocator()))->load($kernel->getContainer()->getParameter('debug.container.dump'));
$container->setParameter('container.build_hash', $hash = ContainerBuilder::hash(__METHOD__));
$container->setParameter('container.build_id', hash('crc32', $hash.time()));

This comment has been minimized.

Copy link
@ro0NL

ro0NL Apr 8, 2019

Contributor

these 2 params can be removed, it was only needed for compilation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.