[DependencyInjection] fixed placeholder management in parameter values

src/Symfony/Components/DependencyInjection/Container.php

@@ -72,10 +72,17 @@ class Container implements ContainerInterface, \ArrayAccess
     }
 
     /**
-     * Freezes the container parameter bag.
+     * Freezes the container.
+     *
+     * This method does two things:
+     *
+     *  * Parameter values are resolved;
+     *  * The parameter bag is freezed.
      */
     public function freeze()
     {
+        $this->parameterBag->resolve();
+
         $this->parameterBag = new FrozenParameterBag($this->parameterBag->all());
     }
 

src/Symfony/Components/DependencyInjection/ContainerBuilder.php

@@ -79,6 +79,10 @@ class ContainerBuilder extends Container implements AnnotatedContainerInterface
      */
     public function loadFromExtension(LoaderExtensionInterface $extension, $tag, array $values = array())
     {
+        if (true === $this->isFrozen()) {
+            throw new \LogicException('Cannot load from an extension on a frozen container.');
+        }
+
         $namespace = $extension->getAlias();
 
         $this->addObjectResource($extension);
@@ -187,6 +191,10 @@ class ContainerBuilder extends Container implements AnnotatedContainerInterface
      */
     public function merge(ContainerBuilder $container)
     {
+        if (true === $this->isFrozen()) {
+            throw new \LogicException('Cannot merge on a frozen container.');
+        }
+
         $this->addDefinitions($container->getDefinitions());
         $this->addAliases($container->getAliases());
         $this->parameterBag->add($container->getParameterBag()->all());
@@ -215,10 +223,16 @@ class ContainerBuilder extends Container implements AnnotatedContainerInterface
     }
 
     /**
-     * Commits the extension configuration into the main configuration
-     * and resolves parameter values.
+     * Freezes the container.
+     *
+     * This method does four things:
+     *
+     *  * The extension configurations are merged;
+     *  * Parameter values are resolved;
+     *  * The parameter bag is freezed;
+     *  * Extension loading is disabled.
      */
-    public function commit()
+    public function freeze()
     {
         $parameters = $this->parameterBag->all();
         $definitions = $this->definitions;
@@ -233,9 +247,7 @@ class ContainerBuilder extends Container implements AnnotatedContainerInterface
         $this->addAliases($aliases);
         $this->parameterBag->add($parameters);
 
-        foreach ($this->parameterBag->all() as $key => $value) {
-            $this->parameterBag->set($key, $this->getParameterBag()->resolveValue($value));
-        }
+        parent::freeze();
     }
 
     /**

src/Symfony/Components/DependencyInjection/Dumper/XmlDumper.php

@@ -42,7 +42,13 @@ class XmlDumper extends Dumper
             return '';
         }
 
-        return sprintf("  <parameters>\n%s  </parameters>\n", $this->convertParameters($this->escape($this->container->getParameterBag()->all()), 'parameter', 4));
+        if ($this->container->isFrozen()) {
+            $parameters = $this->escape($this->container->getParameterBag()->all());
+        } else {
+            $parameters = $this->container->getParameterBag()->all();
+        }
+
+        return sprintf("  <parameters>\n%s  </parameters>\n", $this->convertParameters($parameters, 'parameter', 4));
     }
 
     protected function addService($id, $definition)

src/Symfony/Components/DependencyInjection/Dumper/YamlDumper.php

@@ -128,7 +128,13 @@ class YamlDumper extends Dumper
             return '';
         }
 
-        return Yaml::dump(array('parameters' => $this->prepareParameters($this->container->getParameterBag()->all())), 2);
+        if ($this->container->isFrozen()) {
+            $parameters = $this->prepareParameters($this->container->getParameterBag()->all());
+        } else {
+            $parameters = $this->container->getParameterBag()->all();
+        }
+
+        return Yaml::dump(array('parameters' => $parameters), 2);
     }
 
     /**

src/Symfony/Components/DependencyInjection/ParameterBag/ParameterBag.php

@@ -105,6 +105,16 @@ class ParameterBag implements ParameterBagInterface
         return array_key_exists(strtolower($name), $this->parameters);
     }
 
+    /**
+     * Replaces parameter placeholders (%name%) by their values for all parameters.
+     */
+    public function resolve()
+    {
+        foreach ($this->parameters as $key => $value) {
+            $this->parameters[$key] = $this->resolveValue($value);
+        }
+    }
+
     /**
      * Replaces parameter placeholders (%name%) by their values.
      *
@@ -120,31 +130,24 @@ class ParameterBag implements ParameterBagInterface
                 $args[$this->resolveValue($k)] = $this->resolveValue($v);
             }
 
-            $value = $args;
-        } else if (is_string($value)) {
-            if (preg_match('/^%([^%]+)%$/', $value, $match)) {
-                // we do this to deal with non string values (boolean, integer, ...)
-                // the preg_replace_callback converts them to strings
-                if (!array_key_exists($name = strtolower($match[1]), $this->parameters)) {
-                    throw new \RuntimeException(sprintf('The parameter "%s" must be defined.', $name));
-                }
-
-                $value = $this->parameters[$name];
-            } else {
-                $parameters = $this->parameters;
-                $replaceParameter = function ($match) use ($parameters, $value)
-                {
-                    if (!array_key_exists($name = strtolower($match[2]), $parameters)) {
-                        throw new \RuntimeException(sprintf('The parameter "%s" must be defined (used in the following expression: "%s").', $name, $value));
-                    }
-
-                    return $parameters[$name];
-                };
-
-                $value = str_replace('%%', '%', preg_replace_callback('/(?<!%)(%)([^%]+)\1/', $replaceParameter, $value));
-            }
+            return $args;
         }
 
-        return $value;
+        if (!is_string($value)) {
+            return $value;
+        }
+
+        if (preg_match('/^%([^%]+)%$/', $value, $match)) {
+            // we do this to deal with non string values (boolean, integer, ...)
+            // the preg_replace_callback converts them to strings
+            return $this->get(strtolower($match[1]));
+        }
+
+        return str_replace('%%', '%', preg_replace_callback(array('/(?<!%)%([^%]+)%/'), array($this, 'resolveValueCallback'), $value));
+    }
+
+    protected function resolveValueCallback($match)
+    {
+        return $this->get(strtolower($match[1]));
     }
 }

tests/Symfony/Tests/Components/DependencyInjection/ContainerBuilderTest.php

@@ -313,14 +313,14 @@ class ContainerBuilderTest extends \PHPUnit_Framework_TestCase
         $config = new ContainerBuilder(new ParameterBag(array('foo' => '%bar%')));
         $container->merge($config);
 ////// FIXME
-        $container->commit();
+        $container->freeze();
         $this->assertEquals(array('bar' => 'foo', 'foo' => 'foo'), $container->getParameterBag()->all(), '->merge() evaluates the values of the parameters towards already defined ones');
 
         $container = new ContainerBuilder(new ParameterBag(array('bar' => 'foo')));
         $config = new ContainerBuilder(new ParameterBag(array('foo' => '%bar%', 'baz' => '%foo%')));
         $container->merge($config);
 ////// FIXME
-        $container->commit();
+        $container->freeze();
         $this->assertEquals(array('bar' => 'foo', 'foo' => 'foo', 'baz' => 'foo'), $container->getParameterBag()->all(), '->merge() evaluates the values of the parameters towards already defined ones');
 
         $container = new ContainerBuilder();

tests/Symfony/Tests/Components/DependencyInjection/CrossCheckTest.php

@@ -32,22 +32,24 @@ class CrossCheckTest extends \PHPUnit_Framework_TestCase
         $loaderClass = 'Symfony\\Components\\DependencyInjection\\Loader\\'.ucfirst($type).'FileLoader';
         $dumperClass = 'Symfony\\Components\\DependencyInjection\\Dumper\\'.ucfirst($type).'Dumper';
 
-        $container1 = new ContainerBuilder();
-        $loader1 = new $loaderClass($container1);
-        $loader1->load(self::$fixturesPath.'/'.$type.'/'.$fixture);
-        $container1->setParameter('path', self::$fixturesPath.'/includes');
+        $tmp = tempnam('sf_service_container', 'sf');
+
+        $loader1 = new $loaderClass();
+        file_put_contents($tmp, file_get_contents(self::$fixturesPath.'/'.$type.'/'.$fixture));
+        $container1 = $loader1->load($tmp);
 
         $dumper = new $dumperClass($container1);
-        $tmp = tempnam('sf_service_container', 'sf');
         file_put_contents($tmp, $dumper->dump());
 
-        $container2 = new ContainerBuilder();
-        $loader2 = new $loaderClass($container2);
-        $loader2->load($tmp);
-        $container2->setParameter('path', self::$fixturesPath.'/includes');
+        $loader2 = new $loaderClass();
+        $container2 = $loader2->load($tmp);
 
         unlink($tmp);
 
+        $this->assertEquals($container2->getAliases(), $container1->getAliases(), 'loading a dump from a previously loaded container returns the same container');
+        $this->assertEquals($container2->getDefinitions(), $container1->getDefinitions(), 'loading a dump from a previously loaded container returns the same container');
+        $this->assertEquals($container2->getParameterBag()->all(), $container1->getParameterBag()->all(), '->getParameterBag() returns the same value for both containers');
+
         $this->assertEquals(serialize($container2), serialize($container1), 'loading a dump from a previously loaded container returns the same container');
 
         $this->assertEquals($container2->getParameterBag()->all(), $container1->getParameterBag()->all(), '->getParameterBag() returns the same value for both containers');

tests/Symfony/Tests/Components/DependencyInjection/Fixtures/containers/container8.php

@@ -4,8 +4,9 @@ use Symfony\Components\DependencyInjection\ContainerBuilder;
 use Symfony\Components\DependencyInjection\ParameterBag\ParameterBag;
 
 $container = new ContainerBuilder(new ParameterBag(array(
-    'FOO'    => 'bar',
-    'bar'    => 'foo is %foo bar',
+    'FOO'    => '%baz%',
+    'baz'    => 'bar',
+    'bar'    => 'foo is %%foo bar',
     'values' => array(true, false, null, 0, 1000.3, 'true', 'false', 'null'),
 )));
 

tests/Symfony/Tests/Components/DependencyInjection/Fixtures/php/services8.php

@@ -47,8 +47,9 @@ class ProjectServiceContainer extends Container
     protected function getDefaultParameters()
     {
         return array(
-            'foo' => 'bar',
-            'bar' => 'foo is %foo bar',
+            'foo' => '%baz%',
+            'baz' => 'bar',
+            'bar' => 'foo is %%foo bar',
             'values' => array(
                 0 => true,
                 1 => false,

tests/Symfony/Tests/Components/DependencyInjection/Fixtures/xml/services8.xml

@@ -4,7 +4,8 @@
     xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
     xsi:schemaLocation="http://www.symfony-project.org/schema/dic/services http://www.symfony-project.org/schema/dic/services/services-1.0.xsd">
   <parameters>
-    <parameter key="foo">bar</parameter>
+    <parameter key="foo">%baz%</parameter>
+    <parameter key="baz">bar</parameter>
     <parameter key="bar">foo is %%foo bar</parameter>
     <parameter key="values" type="collection">
       <parameter>true</parameter>

tests/Symfony/Tests/Components/DependencyInjection/Fixtures/yaml/services8.yml

@@ -1,5 +1,6 @@
 parameters:
-  foo: bar
+  foo: '%baz%'
+  baz: bar
   bar: 'foo is %%foo bar'
   values: [true, false, null, 0, 1000.3, 'true', 'false', 'null']
 

tests/Symfony/Tests/Components/DependencyInjection/Loader/XmlFileLoaderTest.php

@@ -173,7 +173,7 @@ class XmlFileLoaderTest extends \PHPUnit_Framework_TestCase
 
         // extension without an XSD
         $config = $loader->load('extensions/services1.xml');
-        $config->commit();
+        $config->freeze();
         $services = $config->getDefinitions();
         $parameters = $config->getParameterBag()->all();
 
@@ -185,7 +185,7 @@ class XmlFileLoaderTest extends \PHPUnit_Framework_TestCase
 
         // extension with an XSD
         $config = $loader->load('extensions/services2.xml');
-        $config->commit();
+        $config->freeze();
         $services = $config->getDefinitions();
         $parameters = $config->getParameterBag()->all();
 

tests/Symfony/Tests/Components/DependencyInjection/Loader/YamlFileLoaderTest.php

@@ -107,7 +107,7 @@ class YamlFileLoaderTest extends \PHPUnit_Framework_TestCase
         $loader = new ProjectLoader3(self::$fixturesPath.'/yaml');
 
         $config = $loader->load('services10.yml');
-        $config->commit();
+        $config->freeze();
         $services = $config->getDefinitions();
         $parameters = $config->getParameterBag()->all();
 

tests/Symfony/Tests/Components/DependencyInjection/ParameterBag/ParameterBagTest.php

@@ -97,18 +97,18 @@ class ParameterBagTest extends \PHPUnit_Framework_TestCase
         $bag = new ParameterBag(array());
         try {
             $bag->resolveValue('%foobar%', array());
-            $this->fail('->resolveValue() throws a RuntimeException if a placeholder references a non-existant parameter');
+            $this->fail('->resolveValue() throws an InvalidArgumentException if a placeholder references a non-existant parameter');
         } catch (\Exception $e) {
-            $this->assertInstanceOf('\RuntimeException', $e, '->resolveValue() throws a RuntimeException if a placeholder references a non-existant parameter');
-            $this->assertEquals('The parameter "foobar" must be defined.', $e->getMessage(), '->resolveValue() throws a RuntimeException if a placeholder references a non-existant parameter');
+            $this->assertInstanceOf('\InvalidArgumentException', $e, '->resolveValue() throws an InvalidArgumentException if a placeholder references a non-existant parameter');
+            $this->assertEquals('The parameter "foobar" must be defined.', $e->getMessage(), '->resolveValue() throws an InvalidArgumentException if a placeholder references a non-existant parameter');
         }
 
         try {
             $bag->resolveValue('foo %foobar% bar', array());
-            $this->fail('->resolveValue() throws a RuntimeException if a placeholder references a non-existant parameter');
+            $this->fail('->resolveValue() throws an InvalidArgumentException if a placeholder references a non-existant parameter');
         } catch (\Exception $e) {
-            $this->assertInstanceOf('\RuntimeException', $e, '->resolveValue() throws a RuntimeException if a placeholder references a non-existant parameter');
-            $this->assertEquals('The parameter "foobar" must be defined (used in the following expression: "foo %foobar% bar").', $e->getMessage(), '->resolveValue() throws a RuntimeException if a placeholder references a non-existant parameter');
+            $this->assertInstanceOf('\InvalidArgumentException', $e, '->resolveValue() throws an InvalidArgumentException if a placeholder references a non-existant parameter');
+            $this->assertEquals('The parameter "foobar" must be defined.', $e->getMessage(), '->resolveValue() throws an InvalidArgumentException if a placeholder references a non-existant parameter');
         }
     }
 }