merged branch Seldaek/fix-container-opt (PR #8193)

This PR was merged into the 2.3 branch.

Discussion
----------

[DI] Fixes access of aliases shared services

Fixes #8096 and I noticed that the aliases methods weren't really needed anymore so I removed them. I think it's fine since they were protected, but did it in a separate commit in case you just want the bugfix.

Note that while the DI tests pass, I didn't run this patch as part of a real app. I'd appreciate if someone has the time to verify it still works given it's slightly critical code.

Commits
-------

81b122d [DependencyInjection] Add support for aliases of aliases + regression test
d8c0ef7 [DependencyInjection] Rename ContainerBuilder::$aliases to avoid conflicting with the parent class
bb797ee [DependencyInjection] Remove get*Alias*Service methods from compiled containers
379f5e0 [DependencyInjection] Fix aliased access of shared services, fixes #8096

src/Symfony/Component/DependencyInjection/Container.php

@@ -69,6 +69,7 @@ class Container implements IntrospectableContainerInterface
 
     protected $services;
     protected $methodMap;
+    protected $aliases;
     protected $scopes;
     protected $scopeChildren;
     protected $scopedServices;
@@ -253,6 +254,12 @@ class Container implements IntrospectableContainerInterface
     {
         $id = strtolower($id);
 
+        // resolve aliases
+        if (isset($this->aliases[$id])) {
+            $id = $this->aliases[$id];
+        }
+
+        // re-use shared service instance if it exists
         if (array_key_exists($id, $this->services)) {
             return $this->services[$id];
         }
@@ -264,6 +271,7 @@ class Container implements IntrospectableContainerInterface
         if (isset($this->methodMap[$id])) {
             $method = $this->methodMap[$id];
         } elseif (method_exists($this, $method = 'get'.strtr($id, array('_' => '', '.' => '_')).'Service')) {
+            // $method is set to the right value, proceed
         } else {
             if (self::EXCEPTION_ON_INVALID_REFERENCE === $invalidBehavior) {
                 if (!$id) {

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -57,7 +57,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
     /**
      * @var Alias[]
      */
-    private $aliases = array();
+    private $aliasDefinitions = array();
 
     /**
      * @var ResourceInterface[]
@@ -404,7 +404,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
             $this->obsoleteDefinitions[$id] = $this->definitions[$id];
         }
 
-        unset($this->definitions[$id], $this->aliases[$id]);
+        unset($this->definitions[$id], $this->aliasDefinitions[$id]);
 
         parent::set($id, $service, $scope);
 
@@ -438,7 +438,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
     {
         $id = strtolower($id);
 
-        return isset($this->definitions[$id]) || isset($this->aliases[$id]) || parent::has($id);
+        return isset($this->definitions[$id]) || isset($this->aliasDefinitions[$id]) || parent::has($id);
     }
 
     /**
@@ -475,8 +475,8 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
                 throw new LogicException(sprintf('The service "%s" has a circular reference to itself.', $id), 0, $e);
             }
 
-            if (!$this->hasDefinition($id) && isset($this->aliases[$id])) {
+            if (!$this->hasDefinition($id) && isset($this->aliasDefinitions[$id])) {
-                return $this->get($this->aliases[$id]);
+                return $this->get($this->aliasDefinitions[$id]);
             }
 
             try {
@@ -640,7 +640,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
      */
     public function getServiceIds()
     {
-        return array_unique(array_merge(array_keys($this->getDefinitions()), array_keys($this->aliases), parent::getServiceIds()));
+        return array_unique(array_merge(array_keys($this->getDefinitions()), array_keys($this->aliasDefinitions), parent::getServiceIds()));
     }
 
     /**
@@ -666,7 +666,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
      */
     public function setAliases(array $aliases)
     {
-        $this->aliases = array();
+        $this->aliasDefinitions = array();
         $this->addAliases($aliases);
     }
 
@@ -697,7 +697,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
 
         unset($this->definitions[$alias]);
 
-        $this->aliases[$alias] = $id;
+        $this->aliasDefinitions[$alias] = $id;
     }
 
     /**
@@ -709,7 +709,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
      */
     public function removeAlias($alias)
     {
-        unset($this->aliases[strtolower($alias)]);
+        unset($this->aliasDefinitions[strtolower($alias)]);
     }
 
     /**
@@ -723,7 +723,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
      */
     public function hasAlias($id)
     {
-        return isset($this->aliases[strtolower($id)]);
+        return isset($this->aliasDefinitions[strtolower($id)]);
     }
 
     /**
@@ -735,7 +735,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
      */
     public function getAliases()
     {
-        return $this->aliases;
+        return $this->aliasDefinitions;
     }
 
     /**
@@ -757,7 +757,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
             throw new InvalidArgumentException(sprintf('The service alias "%s" does not exist.', $id));
         }
 
-        return $this->aliases[$id];
+        return $this->aliasDefinitions[$id];
     }
 
     /**
@@ -837,7 +837,7 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
 
         $id = strtolower($id);
 
-        unset($this->aliases[$id]);
+        unset($this->aliasDefinitions[$id]);
 
         return $this->definitions[$id] = $definition;
     }

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php

@@ -594,39 +594,6 @@ EOF;
         return $code;
     }
 
-    /**
-     * Adds a service alias.
-     *
-     * @param string $alias
-     * @param string $id
-     *
-     * @return string
-     */
-    private function addServiceAlias($alias, $id)
-    {
-        $name = Container::camelize($alias);
-        $type = 'Object';
-
-        if ($this->container->hasDefinition($id)) {
-            $class = $this->container->getDefinition($id)->getClass();
-            $type = 0 === strpos($class, '%') ? 'Object' : $class;
-        }
-
-        return <<<EOF
-
-    /**
-     * Gets the $alias service alias.
-     *
-     * @return $type An instance of the $id service
-     */
-    protected function get{$name}Service()
-    {
-        return {$this->getServiceCall($id)};
-    }
-
-EOF;
-    }
-
     /**
      * Adds multiple services
      *
@@ -634,7 +601,7 @@ EOF;
      */
     private function addServices()
     {
-        $publicServices = $privateServices = $aliasServices = $synchronizers = '';
+        $publicServices = $privateServices = $synchronizers = '';
         $definitions = $this->container->getDefinitions();
         ksort($definitions);
         foreach ($definitions as $id => $definition) {
@@ -647,13 +614,7 @@ EOF;
             $synchronizers .= $this->addServiceSynchronizer($id, $definition);
         }
 
-        $aliases = $this->container->getAliases();
+        return $publicServices.$synchronizers.$privateServices;
-        ksort($aliases);
-        foreach ($aliases as $alias => $id) {
-            $aliasServices .= $this->addServiceAlias($alias, $id);
-        }
-
-        return $publicServices.$aliasServices.$synchronizers.$privateServices;
     }
 
     /**
@@ -799,6 +760,9 @@ EOF;
             $code .= "        \$this->scopeChildren = ".$this->dumpValue($this->container->getScopeChildren()).";\n";
         }
 
+        $code .= $this->addMethodMap();
+        $code .= $this->addAliases();
+
         $code .= <<<EOF
     }
 
@@ -846,19 +810,8 @@ EOF;
             $code .= "        \$this->scopeChildren = array();\n";
         }
 
-        // build method map
+        $code .= $this->addMethodMap();
-        $code .= "        \$this->methodMap = array(\n";
+        $code .= $this->addAliases();
-        $definitions = $this->container->getDefinitions();
-        ksort($definitions);
-        foreach ($definitions as $id => $definition) {
-            $code .= '            '.var_export($id, true).' => '.var_export('get'.Container::camelize($id).'Service', true).",\n";
-        }
-        $aliases = $this->container->getAliases();
-        ksort($aliases);
-        foreach ($aliases as $alias => $id) {
-            $code .= '            '.var_export($alias, true).' => '.var_export('get'.Container::camelize($id).'Service', true).",\n";
-        }
-        $code .= "        );\n";
 
         $code .= <<<EOF
     }
@@ -868,6 +821,50 @@ EOF;
         return $code;
     }
 
+    /**
+     * Adds the methodMap property definition
+     *
+     * @return string
+     */
+    private function addMethodMap()
+    {
+        if (!$definitions = $this->container->getDefinitions()) {
+            return '';
+        }
+
+        $code = "        \$this->methodMap = array(\n";
+        ksort($definitions);
+        foreach ($definitions as $id => $definition) {
+            $code .= '            '.var_export($id, true).' => '.var_export('get'.Container::camelize($id).'Service', true).",\n";
+        }
+
+        return $code . "        );\n";
+    }
+
+    /**
+     * Adds the aliases property definition
+     *
+     * @return string
+     */
+    private function addAliases()
+    {
+        if (!$aliases = $this->container->getAliases()) {
+            return '';
+        }
+
+        $code = "        \$this->aliases = array(\n";
+        ksort($aliases);
+        foreach ($aliases as $alias => $id) {
+            $id = (string) $id;
+            while (isset($aliases[$id])) {
+                $id = (string) $aliases[$id];
+            }
+            $code .= '            '.var_export($alias, true).' => '.var_export($id, true).",\n";
+        }
+
+        return $code . "        );\n";
+    }
+
     /**
      * Adds default parameters method.
      *

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

@@ -15,6 +15,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
 use Symfony\Component\DependencyInjection\Parameter;
 use Symfony\Component\DependencyInjection\Reference;
 use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\DependencyInjection\Alias;
 use Symfony\Component\DependencyInjection\Exception\RuntimeException;
 
 /**
@@ -182,10 +183,10 @@ class XmlDumper extends Dumper
      * Adds a service alias.
      *
      * @param string      $alias
-     * @param string      $id
+     * @param Alias       $id
      * @param \DOMElement $parent
      */
-    private function addServiceAlias($alias, $id, \DOMElement $parent)
+    private function addServiceAlias($alias, Alias $id, \DOMElement $parent)
     {
         $service = $this->document->createElement('service');
         $service->setAttribute('id', $alias);
@@ -213,7 +214,11 @@ class XmlDumper extends Dumper
             $this->addService($definition, $id, $services);
         }
 
-        foreach ($this->container->getAliases() as $alias => $id) {
+        $aliases = $this->container->getAliases();
+        foreach ($aliases as $alias => $id) {
+            while (isset($aliases[(string) $id])) {
+                $id = $aliases[(string) $id];
+            }
             $this->addServiceAlias($alias, $id, $services);
         }
         $parent->appendChild($services);

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

@@ -182,7 +182,11 @@ class YamlDumper extends Dumper
             $code .= $this->addService($id, $definition);
         }
 
-        foreach ($this->container->getAliases() as $alias => $id) {
+        $aliases = $this->container->getAliases();
+        foreach ($aliases as $alias => $id) {
+            while (isset($aliases[(string) $id])) {
+                $id = $aliases[(string) $id];
+            }
             $code .= $this->addServiceAlias($alias, $id);
         }
 

src/Symfony/Component/DependencyInjection/Tests/Dumper/PhpDumperTest.php

@@ -120,6 +120,20 @@ class PhpDumperTest extends \PHPUnit_Framework_TestCase
         }
     }
 
+    public function testAliases()
+    {
+        $container = include self::$fixturesPath.'/containers/container9.php';
+        $container->compile();
+        $dumper = new PhpDumper($container);
+        eval('?>'.$dumper->dump(array('class' => 'Symfony_DI_PhpDumper_Test_Aliases')));
+
+        $container = new \Symfony_DI_PhpDumper_Test_Aliases();
+        $container->set('foo', $foo = new \stdClass);
+        $this->assertSame($foo, $container->get('foo'));
+        $this->assertSame($foo, $container->get('alias_for_foo'));
+        $this->assertSame($foo, $container->get('alias_for_alias'));
+    }
+
     public function testOverrideServiceWhenUsingADumpedContainer()
     {
         require_once self::$fixturesPath.'/php/services9.php';

src/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/container9.php

@@ -43,6 +43,7 @@ $container->getParameterBag()->add(array(
     'foo' => 'bar',
 ));
 $container->setAlias('alias_for_foo', 'foo');
+$container->setAlias('alias_for_alias', 'alias_for_foo');
 $container->
     register('method_call1', 'FooClass')->
     setFile(realpath(__DIR__.'/../includes/foo.php'))->

src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9.php

@@ -24,6 +24,23 @@ class ProjectServiceContainer extends Container
     public function __construct()
     {
         parent::__construct(new ParameterBag($this->getDefaultParameters()));
+        $this->methodMap = array(
+            'bar' => 'getBarService',
+            'baz' => 'getBazService',
+            'depends_on_request' => 'getDependsOnRequestService',
+            'factory_service' => 'getFactoryServiceService',
+            'foo' => 'getFooService',
+            'foo.baz' => 'getFoo_BazService',
+            'foo_bar' => 'getFooBarService',
+            'foo_with_inline' => 'getFooWithInlineService',
+            'inlined' => 'getInlinedService',
+            'method_call1' => 'getMethodCall1Service',
+            'request' => 'getRequestService',
+        );
+        $this->aliases = array(
+            'alias_for_alias' => 'foo',
+            'alias_for_foo' => 'foo',
+        );
     }
 
     /**
@@ -198,16 +215,6 @@ class ProjectServiceContainer extends Container
         throw new RuntimeException('You have requested a synthetic service ("request"). The DIC does not know how to construct this service.');
     }
 
-    /**
-     * Gets the alias_for_foo service alias.
-     *
-     * @return FooClass An instance of the foo service
-     */
-    protected function getAliasForFooService()
-    {
-        return $this->get('foo');
-    }
-
     /**
      * Updates the 'request' service.
      */

src/Symfony/Component/DependencyInjection/Tests/Fixtures/php/services9_compiled.php

@@ -44,7 +44,10 @@ class ProjectServiceContainer extends Container
             'foo_with_inline' => 'getFooWithInlineService',
             'method_call1' => 'getMethodCall1Service',
             'request' => 'getRequestService',
-            'alias_for_foo' => 'getFooService',
+        );
+        $this->aliases = array(
+            'alias_for_alias' => 'foo',
+            'alias_for_foo' => 'foo',
         );
     }
 
@@ -217,16 +220,6 @@ class ProjectServiceContainer extends Container
         throw new RuntimeException('You have requested a synthetic service ("request"). The DIC does not know how to construct this service.');
     }
 
-    /**
-     * Gets the alias_for_foo service alias.
-     *
-     * @return FooClass An instance of the foo service
-     */
-    protected function getAliasForFooService()
-    {
-        return $this->get('foo');
-    }
-
     /**
      * Updates the 'request' service.
      */

src/Symfony/Component/DependencyInjection/Tests/Fixtures/xml/services9.xml

@@ -74,5 +74,6 @@
       </call>
     </service>
     <service id="alias_for_foo" alias="foo"/>
+    <service id="alias_for_alias" alias="foo"/>
   </services>
 </container>

src/Symfony/Component/DependencyInjection/Tests/Fixtures/yaml/services9.yml

@@ -69,3 +69,4 @@ services:
             - [setRequest, ['@?request']]
 
     alias_for_foo: @foo
+    alias_for_alias: @foo