bug #25146 [DI] Dont resolve envs in service ids (nicolas-grekas)

This PR was merged into the 3.3 branch. Discussion ---------- [DI] Dont resolve envs in service ids | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25123 | License | MIT | Doc PR | - Env placeholders should not be forbidden in **private** service identifiers. Instead, they should just be *not* resolved. This is effectively used in many bundle extensions fr good reasons (see eg. SecurityBundle and linked issue). Commits ------- 7921255faf [DI] Dont resolve envs in service ids
2017-11-24 05:54:00 -08:00 · 2017-11-24 05:54:00 -08:00 · ca5b15a53b
commit ca5b15a53b
parent 5c954c79c0 7921255faf
8 changed files with 269 additions and 27 deletions
--- a/src/Symfony/Component/DependencyInjection/Compiler/CheckDefinitionValidityPass.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/CheckDefinitionValidityPass.php
@ -77,16 +77,20 @@ class CheckDefinitionValidityPass implements CompilerPassInterface
                }
            }

-            $resolvedId = $container->resolveEnvPlaceholders($id, null, $usedEnvs);
-            if (null !== $usedEnvs) {
-                throw new EnvParameterException(array($resolvedId), null, 'A service name ("%s") cannot contain dynamic values.');
+            if ($definition->isPublic()) {
+                $resolvedId = $container->resolveEnvPlaceholders($id, null, $usedEnvs);
+                if (null !== $usedEnvs) {
+                    throw new EnvParameterException(array($resolvedId), null, 'A service name ("%s") cannot contain dynamic values.');
+                }
            }
        }

        foreach ($container->getAliases() as $id => $alias) {
-            $resolvedId = $container->resolveEnvPlaceholders($id, null, $usedEnvs);
-            if (null !== $usedEnvs) {
-                throw new EnvParameterException(array($resolvedId), null, 'An alias name ("%s") cannot contain dynamic values.');
+            if ($alias->isPublic()) {
+                $resolvedId = $container->resolveEnvPlaceholders($id, null, $usedEnvs);
+                if (null !== $usedEnvs) {
+                    throw new EnvParameterException(array($resolvedId), null, 'An alias name ("%s") cannot contain dynamic values.');
+                }
            }
        }
    }
--- a/src/Symfony/Component/DependencyInjection/Compiler/ResolveEnvPlaceholdersPass.php
+++ b/src/Symfony/Component/DependencyInjection/Compiler/ResolveEnvPlaceholdersPass.php
@ -35,7 +35,7 @@ class ResolveEnvPlaceholdersPass extends AbstractRecursivePass

        $value = parent::processValue($value, $isRoot);

-        if ($value && is_array($value)) {
+        if ($value && is_array($value) && !$isRoot) {
            $value = array_combine($this->container->resolveEnvPlaceholders(array_keys($value), true), $value);
        }

--- a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
+++ b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@ -680,6 +680,7 @@ EOF;
    private function addNewInstance(Definition $definition, $return, $instantiation, $id)
    {
        $class = $this->dumpValue($definition->getClass());
+        $return = '        '.$return.$instantiation;

        $arguments = array();
        foreach ($definition->getArguments() as $value) {
@ -695,7 +696,7 @@ EOF;

                if ($callable[0] instanceof Reference
                    || ($callable[0] instanceof Definition && $this->definitionVariables->contains($callable[0]))) {
-                    return sprintf("        $return{$instantiation}%s->%s(%s);\n", $this->dumpValue($callable[0]), $callable[1], $arguments ? implode(', ', $arguments) : '');
+                    return $return.sprintf("%s->%s(%s);\n", $this->dumpValue($callable[0]), $callable[1], $arguments ? implode(', ', $arguments) : '');
                }

                $class = $this->dumpValue($callable[0]);
@ -705,24 +706,24 @@ EOF;
                        throw new RuntimeException(sprintf('Cannot dump definition: The "%s" service is defined to be created by a factory but is missing the service reference, did you forget to define the factory service id or class?', $id));
                    }

-                    return sprintf("        $return{$instantiation}%s::%s(%s);\n", $this->dumpLiteralClass($class), $callable[1], $arguments ? implode(', ', $arguments) : '');
+                    return $return.sprintf("%s::%s(%s);\n", $this->dumpLiteralClass($class), $callable[1], $arguments ? implode(', ', $arguments) : '');
                }

                if (0 === strpos($class, 'new ')) {
-                    return sprintf("        $return{$instantiation}(%s)->%s(%s);\n", $this->dumpValue($callable[0]), $callable[1], $arguments ? implode(', ', $arguments) : '');
+                    return $return.sprintf("(%s)->%s(%s);\n", $this->dumpValue($callable[0]), $callable[1], $arguments ? implode(', ', $arguments) : '');
                }

-                return sprintf("        $return{$instantiation}call_user_func(array(%s, '%s')%s);\n", $this->dumpValue($callable[0]), $callable[1], $arguments ? ', '.implode(', ', $arguments) : '');
+                return $return.sprintf("call_user_func(array(%s, '%s')%s);\n", $this->dumpValue($callable[0]), $callable[1], $arguments ? ', '.implode(', ', $arguments) : '');
            }

-            return sprintf("        $return{$instantiation}%s(%s);\n", $this->dumpLiteralClass($this->dumpValue($callable)), $arguments ? implode(', ', $arguments) : '');
+            return $return.sprintf("%s(%s);\n", $this->dumpLiteralClass($this->dumpValue($callable)), $arguments ? implode(', ', $arguments) : '');
        }

        if (false !== strpos($class, '$')) {
-            return sprintf("        \$class = %s;\n\n        $return{$instantiation}new \$class(%s);\n", $class, implode(', ', $arguments));
+            return sprintf("        \$class = %s;\n\n%snew \$class(%s);\n", $class, $return, implode(', ', $arguments));
        }

-        return sprintf("        $return{$instantiation}new %s(%s);\n", $this->dumpLiteralClass($class), implode(', ', $arguments));
+        return $return.sprintf("new %s(%s);\n", $this->dumpLiteralClass($class), implode(', ', $arguments));
    }

    /**
@ -890,7 +891,7 @@ EOF;
        ksort($normalizedIds);
        foreach ($normalizedIds as $id => $normalizedId) {
            if ($this->container->has($normalizedId)) {
-                $code .= '            '.$this->export($id).' => '.$this->export($normalizedId).",\n";
+                $code .= '            '.$this->doExport($id).' => '.$this->doExport($normalizedId).",\n";
            }
        }

@ -912,7 +913,7 @@ EOF;
        $code = "        \$this->methodMap = array(\n";
        ksort($definitions);
        foreach ($definitions as $id => $definition) {
-            $code .= '            '.$this->export($id).' => '.$this->export($this->generateMethodName($id)).",\n";
+            $code .= '            '.$this->doExport($id).' => '.$this->doExport($this->generateMethodName($id)).",\n";
        }

        return $code."        );\n";
@ -933,7 +934,7 @@ EOF;
        ksort($definitions);
        foreach ($definitions as $id => $definition) {
            if (!$definition->isPublic()) {
-                $code .= '            '.$this->export($id)." => true,\n";
+                $code .= '            '.$this->doExport($id)." => true,\n";
            }
        }

@ -966,7 +967,7 @@ EOF;
            while (isset($aliases[$id])) {
                $id = (string) $aliases[$id];
            }
-            $code .= '            '.$this->export($alias).' => '.$this->export($id).",\n";
+            $code .= '            '.$this->doExport($alias).' => '.$this->doExport($id).",\n";
        }

        return $code."        );\n";
@ -1688,9 +1689,9 @@ EOF;
    private function export($value)
    {
        if (null !== $this->targetDirRegex && is_string($value) && preg_match($this->targetDirRegex, $value, $matches, PREG_OFFSET_CAPTURE)) {
-            $prefix = $matches[0][1] ? $this->doExport(substr($value, 0, $matches[0][1])).'.' : '';
+            $prefix = $matches[0][1] ? $this->doExport(substr($value, 0, $matches[0][1]), true).'.' : '';
            $suffix = $matches[0][1] + strlen($matches[0][0]);
-            $suffix = isset($value[$suffix]) ? '.'.$this->doExport(substr($value, $suffix)) : '';
+            $suffix = isset($value[$suffix]) ? '.'.$this->doExport(substr($value, $suffix), true) : '';
            $dirname = '__DIR__';

            if (0 < $offset = 1 + $this->targetDirMaxMatches - count($matches)) {
@ -1704,10 +1705,10 @@ EOF;
            return $dirname;
        }

-        return $this->doExport($value);
+        return $this->doExport($value, true);
    }

-    private function doExport($value)
+    private function doExport($value, $resolveEnv = false)
    {
        if (is_string($value) && false !== strpos($value, "\n")) {
            $cleanParts = explode("\n", $value);
@ -1717,7 +1718,7 @@ EOF;
            $export = var_export($value, true);
        }

-        if ("'" === $export[0] && $export !== $resolvedExport = $this->container->resolveEnvPlaceholders($export, "'.\$this->getEnv('%s').'")) {
+        if ($resolveEnv && "'" === $export[0] && $export !== $resolvedExport = $this->container->resolveEnvPlaceholders($export, "'.\$this->getEnv('%s').'")) {
            $export = $resolvedExport;
            if ("'" === $export[1]) {
                $export = substr($export, 3);
--- a/src/Symfony/Component/DependencyInjection/Tests/Compiler/CheckDefinitionValidityPassTest.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Compiler/CheckDefinitionValidityPassTest.php
@ -12,6 +12,7 @@
 namespace Symfony\Component\DependencyInjection\Tests\Compiler;

 use PHPUnit\Framework\TestCase;
+use Symfony\Component\DependencyInjection\Alias;
 use Symfony\Component\DependencyInjection\Compiler\CheckDefinitionValidityPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;

@ -79,11 +80,11 @@ class CheckDefinitionValidityPassTest extends TestCase
    /**
     * @expectedException \Symfony\Component\DependencyInjection\Exception\EnvParameterException
     */
-    public function testDynamicServiceName()
+    public function testDynamicPublicServiceName()
    {
        $container = new ContainerBuilder();
        $env = $container->getParameterBag()->get('env(BAR)');
-        $container->register("foo.$env", 'class');
+        $container->register("foo.$env", 'class')->setPublic(true);

        $this->process($container);
    }
@ -91,15 +92,27 @@ class CheckDefinitionValidityPassTest extends TestCase
    /**
     * @expectedException \Symfony\Component\DependencyInjection\Exception\EnvParameterException
     */
-    public function testDynamicAliasName()
+    public function testDynamicPublicAliasName()
    {
        $container = new ContainerBuilder();
        $env = $container->getParameterBag()->get('env(BAR)');
-        $container->setAlias("foo.$env", 'class');
+        $container->setAlias("foo.$env", new Alias('class', true));

        $this->process($container);
    }

+    public function testDynamicPrivateName()
+    {
+        $container = new ContainerBuilder();
+        $env = $container->getParameterBag()->get('env(BAR)');
+        $container->register("foo.$env", 'class')->setPublic(false);
+        $container->setAlias("bar.$env", new Alias('class', false));
+
+        $this->process($container);
+
+        $this->addToAssertionCount(1);
+    }
+
    protected function process(ContainerBuilder $container)
    {
        $pass = new CheckDefinitionValidityPass();
--- a/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/ContainerBuilderTest.php
@ -665,6 +665,22 @@ class ContainerBuilderTest extends TestCase
        $container->compile(true);
    }

+    public function testEnvInId()
+    {
+        $container = include __DIR__.'/Fixtures/containers/container_env_in_id.php';
+        $container->compile(true);
+
+        $expected = array(
+            'service_container',
+            'foo',
+            'bar',
+            'bar_%env(BAR)%',
+        );
+        $this->assertSame($expected, array_keys($container->getDefinitions()));
+
+        $this->assertSame(array('baz_bar'), array_keys($container->getDefinition('foo')->getArgument(1)));
+    }
+
    /**
     * @expectedException \LogicException
     */
--- a/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php
@ -327,6 +327,15 @@ class PhpDumperTest extends TestCase
        $this->assertStringEqualsFile(self::$fixturesPath.'/php/services24.php', $dumper->dump());
    }

+    public function testEnvInId()
+    {
+        $container = include self::$fixturesPath.'/containers/container_env_in_id.php';
+        $container->compile();
+        $dumper = new PhpDumper($container);
+
+        $this->assertStringEqualsFile(self::$fixturesPath.'/php/services_env_in_id.php', $dumper->dump());
+    }
+
    public function testEnvParameter()
    {
        $container = new ContainerBuilder();
--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_env_in_id.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container_env_in_id.php
@ -0,0 +1,22 @@
+<?php
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Dumper\PhpDumper;
+use Symfony\Component\DependencyInjection\Reference;
+
+$container = new ContainerBuilder();
+
+$container->setParameter('env(BAR)', 'bar');
+
+$container->register('foo', 'stdClass')->setPublic(true)
+   ->addArgument(new Reference('bar_%env(BAR)%'))
+   ->addArgument(array('baz_%env(BAR)%' => new Reference('baz_%env(BAR)%')));
+
+$container->register('bar', 'stdClass')->setPublic(true)
+   ->addArgument(new Reference('bar_%env(BAR)%'));
+
+$container->register('bar_%env(BAR)%', 'stdClass')->setPublic(false);
+$container->register('baz_%env(BAR)%', 'stdClass')->setPublic(false);
+
+return $container;
--- a/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_env_in_id.php
+++ b/src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services_env_in_id.php
@ -0,0 +1,177 @@
+<?php
+
+use Symfony\Component\DependencyInjection\Argument\RewindableGenerator;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\DependencyInjection\Container;
+use Symfony\Component\DependencyInjection\Exception\InvalidArgumentException;
+use Symfony\Component\DependencyInjection\Exception\LogicException;
+use Symfony\Component\DependencyInjection\Exception\RuntimeException;
+use Symfony\Component\DependencyInjection\ParameterBag\FrozenParameterBag;
+
+/**
+ * This class has been auto-generated
+ * by the Symfony Dependency Injection Component.
+ *
+ * @final since Symfony 3.3
+ */
+class ProjectServiceContainer extends Container
+{
+    private $parameters;
+    private $targetDirs = array();
+
+    public function __construct()
+    {
+        $this->parameters = $this->getDefaultParameters();
+
+        $this->services = array();
+        $this->normalizedIds = array(
+            'bar_%env(bar)%' => 'bar_%env(BAR)%',
+        );
+        $this->methodMap = array(
+            'bar' => 'getBarService',
+            'bar_%env(BAR)%' => 'getBarenvBARService',
+            'foo' => 'getFooService',
+        );
+        $this->privates = array(
+            'bar_%env(BAR)%' => true,
+        );
+
+        $this->aliases = array();
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function compile()
+    {
+        throw new LogicException('You cannot compile a dumped container that was already compiled.');
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function isCompiled()
+    {
+        return true;
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function isFrozen()
+    {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Use the isCompiled() method instead.', __METHOD__), E_USER_DEPRECATED);
+
+        return true;
+    }
+
+    /**
+     * Gets the public 'bar' shared service.
+     *
+     * @return \stdClass
+     */
+    protected function getBarService()
+    {
+        return $this->services['bar'] = new \stdClass(${($_ = isset($this->services['bar_%env(BAR)%']) ? $this->services['bar_%env(BAR)%'] : $this->getBarenvBARService()) && false ?: '_'});
+    }
+
+    /**
+     * Gets the public 'foo' shared service.
+     *
+     * @return \stdClass
+     */
+    protected function getFooService()
+    {
+        return $this->services['foo'] = new \stdClass(${($_ = isset($this->services['bar_%env(BAR)%']) ? $this->services['bar_%env(BAR)%'] : $this->getBarenvBARService()) && false ?: '_'}, array('baz_'.$this->getEnv('BAR') => new \stdClass()));
+    }
+
+    /**
+     * Gets the private 'bar_%env(BAR)%' shared service.
+     *
+     * @return \stdClass
+     */
+    protected function getBarenvBARService()
+    {
+        return $this->services['bar_%env(BAR)%'] = new \stdClass();
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function getParameter($name)
+    {
+        $name = strtolower($name);
+
+        if (!(isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]))) {
+            throw new InvalidArgumentException(sprintf('The parameter "%s" must be defined.', $name));
+        }
+        if (isset($this->loadedDynamicParameters[$name])) {
+            return $this->loadedDynamicParameters[$name] ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
+        }
+
+        return $this->parameters[$name];
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function hasParameter($name)
+    {
+        $name = strtolower($name);
+
+        return isset($this->parameters[$name]) || array_key_exists($name, $this->parameters) || isset($this->loadedDynamicParameters[$name]);
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function setParameter($name, $value)
+    {
+        throw new LogicException('Impossible to call set() on a frozen ParameterBag.');
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function getParameterBag()
+    {
+        if (null === $this->parameterBag) {
+            $parameters = $this->parameters;
+            foreach ($this->loadedDynamicParameters as $name => $loaded) {
+                $parameters[$name] = $loaded ? $this->dynamicParameters[$name] : $this->getDynamicParameter($name);
+            }
+            $this->parameterBag = new FrozenParameterBag($parameters);
+        }
+
+        return $this->parameterBag;
+    }
+
+    private $loadedDynamicParameters = array();
+    private $dynamicParameters = array();
+
+    /**
+     * Computes a dynamic parameter.
+     *
+     * @param string The name of the dynamic parameter to load
+     *
+     * @return mixed The value of the dynamic parameter
+     *
+     * @throws InvalidArgumentException When the dynamic parameter does not exist
+     */
+    private function getDynamicParameter($name)
+    {
+        throw new InvalidArgumentException(sprintf('The dynamic parameter "%s" must be defined.', $name));
+    }
+
+    /**
+     * Gets the default parameters.
+     *
+     * @return array An array of the default parameters
+     */
+    protected function getDefaultParameters()
+    {
+        return array(
+            'env(bar)' => 'bar',
+        );
+    }
+}