[DI] Optional class for named services

symfony · fabpot · Jan 7, 2017 · Jan 2, 2017 · Oct 21, 2016 · Jan 7, 2017
commit 71b17c779077fffb595325f439c84d6a53dee784
diff --git a/src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php b/src/Symfony/Component/DependencyInjection/Compiler/FactoryReturnTypePass.php
@@ -17,9 +17,21 @@

 /**
 * @author Guilhem N. <egetick@gmail.com>
+ *
+ * @deprecated since version 3.3, to be removed in 4.0.
 */
 class FactoryReturnTypePass implements CompilerPassInterface
 {
+    private $resolveClassPass;
+
+    public function __construct(ResolveClassPass $resolveClassPass = null)
+    {
+        if (null === $resolveClassPass) {
+            @trigger_error('The '.__CLASS__.' class is deprecated since version 3.3 and will be removed in 4.0.', E_USER_DEPRECATED);
+        }
+        $this->resolveClassPass = $resolveClassPass;
+    }
+
    /**
     * {@inheritdoc}
     */
@@ -29,21 +41,22 @@ public function process(ContainerBuilder $container)
        if (!method_exists(\ReflectionMethod::class, 'getReturnType')) {
            return;
        }
+        $resolveClassPassChanges = null !== $this->resolveClassPass ? $this->resolveClassPass->getChanges() : array();

        foreach ($container->getDefinitions() as $id => $definition) {
-            $this->updateDefinition($container, $id, $definition);
+            $this->updateDefinition($container, $id, $definition, $resolveClassPassChanges);
        }
    }

-    private function updateDefinition(ContainerBuilder $container, $id, Definition $definition, array $previous = array())
+    private function updateDefinition(ContainerBuilder $container, $id, Definition $definition, array $resolveClassPassChanges, array $previous = array())
    {
        // circular reference
        if (isset($previous[$id])) {
            return;
        }

        $factory = $definition->getFactory();
-        if (null === $factory || null !== $definition->getClass()) {
+        if (null === $factory || (!isset($resolveClassPassChanges[$id]) && null !== $definition->getClass())) {
            return;
        }

@@ -58,7 +71,7 @@ private function updateDefinition(ContainerBuilder $container, $id, Definition $
            if ($factory[0] instanceof Reference) {
                $previous[$id] = true;
                $factoryDefinition = $container->findDefinition((string) $factory[0]);
-                $this->updateDefinition($container, strtolower($factory[0]), $factoryDefinition, $previous);
+                $this->updateDefinition($container, strtolower($factory[0]), $factoryDefinition, $resolveClassPassChanges, $previous);
                $class = $factoryDefinition->getClass();
            } else {
                $class = $factory[0];
@@ -83,6 +96,9 @@ private function updateDefinition(ContainerBuilder $container, $id, Definition $
                }
            }

+            if (null !== $returnType && (!isset($resolveClassPassChanges[$id]) || $returnType !== $resolveClassPassChanges[$id])) {
+                @trigger_error(sprintf('Relying on its factory\'s return-type to define the class of service "%s" is deprecated since Symfony 3.3 and won\'t work in 4.0. Set the "class" attribute to "%s" on the service definition instead.', $id, $returnType), E_USER_DEPRECATED);
+            }
            $definition->setClass($returnType);
        }
    }

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php b/src/Symfony/Component/DependencyInjection/Compiler/PassConfig.php
@@ -41,10 +41,11 @@ public function __construct()

        $this->optimizationPasses = array(array(
            new ExtensionCompilerPass(),
+            $resolveClassPass = new ResolveClassPass(),
            new ResolveDefinitionTemplatesPass(),
            new DecoratorServicePass(),
            new ResolveParameterPlaceHoldersPass(),
-            new FactoryReturnTypePass(),
+            new FactoryReturnTypePass($resolveClassPass),
            new CheckDefinitionValidityPass(),
            new ResolveReferencesToAliasesPass(),
            new ResolveInvalidReferencesPass(),

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveClassPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveClassPass.php
@@ -0,0 +1,52 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\DependencyInjection\Compiler;
+
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\ChildDefinition;
+
+/**
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class ResolveClassPass implements CompilerPassInterface
+{
+    private $changes = array();
+
+    /**
+     * {@inheritdoc}
+     */
+    public function process(ContainerBuilder $container)
+    {
+        foreach ($container->getDefinitions() as $id => $definition) {
+            if ($definition instanceof ChildDefinition || $definition->isSynthetic() || null !== $definition->getClass()) {
+                continue;
+            }
+            if (preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+(?:\\\\[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*+)*+$/', $id)) {
+                $this->changes[$id] = $container->getCaseSensitiveId($id);
+                $definition->setClass($this->changes[$id]);
+            }
+        }
+    }
+
+    /**
+     * @internal
+     *
+     * @deprecated since 3.3, to be removed in 4.0.
+     */
+    public function getChanges()
+    {
+        $changes = $this->changes;
+        $this->changes = array();
+
+        return $changes;
+    }
+}
diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -103,6 +103,11 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
     */
    private $envCounters = array();

+    /**
+     * @var array a map of case less to case sensitive ids
+     */
+    private $caseSensitiveIds = array();
+
    /**
     * Sets the track resources flag.
     *
@@ -367,14 +372,18 @@ public function getCompiler()
     */
    public function set($id, $service)
    {
-        $id = strtolower($id);
+        $caseSensitiveId = $id;
+        $id = strtolower($caseSensitiveId);

        if ($this->isFrozen() && (isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())) {
            // setting a synthetic service on a frozen container is alright
            throw new BadMethodCallException(sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a frozen container is not allowed.', $id));
        }

        unset($this->definitions[$id], $this->aliasDefinitions[$id]);
+        if ($id !== $caseSensitiveId) {
+            $this->caseSensitiveIds[$id] = $caseSensitiveId;
+        }

        parent::set($id, $service);
    }
@@ -628,7 +637,8 @@ public function setAliases(array $aliases)
     */
    public function setAlias($alias, $id)
    {
-        $alias = strtolower($alias);
+        $caseSensitiveAlias = $alias;
+        $alias = strtolower($caseSensitiveAlias);

        if (is_string($id)) {
            $id = new Alias($id);
@@ -641,6 +651,9 @@ public function setAlias($alias, $id)
        }

        unset($this->definitions[$alias]);
+        if ($alias !== $caseSensitiveAlias) {
+            $this->caseSensitiveIds[$alias] = $caseSensitiveAlias;
+        }

        $this->aliasDefinitions[$alias] = $id;
    }
@@ -778,9 +791,13 @@ public function setDefinition($id, Definition $definition)
            throw new BadMethodCallException('Adding definition to a frozen container is not allowed');
        }

-        $id = strtolower($id);
+        $caseSensitiveId = $id;
+        $id = strtolower($caseSensitiveId);

        unset($this->aliasDefinitions[$id]);
+        if ($id !== $caseSensitiveId) {
+            $this->caseSensitiveIds[$id] = $caseSensitiveId;
+        }

        return $this->definitions[$id] = $definition;
    }
@@ -839,6 +856,20 @@ public function findDefinition($id)
        return $this->getDefinition($id);
    }

+    /**
+     * Returns the case sensitive id used at registration time.
+     *
+     * @param string $id
+     *
+     * @return string
+     */
+    public function getCaseSensitiveId($id)
+    {
+        $id = strtolower($id);
+
+        return isset($this->caseSensitiveIds[$id]) ? $this->caseSensitiveIds[$id] : $id;
+    }
+
    /**
     * Creates a service for a service definition.
     *

diff --git a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
@@ -303,7 +303,7 @@ private function processAnonymousServices(\DOMDocument $xml, $file)
        if (false !== $nodes = $xpath->query('//container:argument[@type="service"][not(@id)]|//container:property[@type="service"][not(@id)]')) {
            foreach ($nodes as $node) {
                // give it a unique name
-                $id = sprintf('%s_%d', hash('sha256', $file), ++$count);
+                $id = sprintf('%d_%s', ++$count, hash('sha256', $file));
                $node->setAttribute('id', $id);

                if ($services = $this->getChildren($node, 'service')) {
@@ -321,15 +321,15 @@ private function processAnonymousServices(\DOMDocument $xml, $file)
        if (false !== $nodes = $xpath->query('//container:services/container:service[not(@id)]')) {
            foreach ($nodes as $node) {
                // give it a unique name
-                $id = sprintf('%s_%d', hash('sha256', $file), ++$count);
+                $id = sprintf('%d_%s', ++$count, hash('sha256', $file));
                $node->setAttribute('id', $id);
                $definitions[$id] = array($node, $file, true);
            }
        }

        // resolve definitions
-        krsort($definitions);
-        foreach ($definitions as $id => list($domElement, $file, $wild)) {
+        uksort($definitions, 'strnatcmp');
+        foreach (array_reverse($definitions) as $id => list($domElement, $file, $wild)) {
            if (null !== $definition = $this->parseDefinition($domElement, $file)) {
                $this->container->setDefinition($id, $definition);
            }

diff --git a/src/Symfony/Component/DependencyInjection/Tests/Compiler/FactoryReturnTypePassTest.php b/src/Symfony/Component/DependencyInjection/Tests/Compiler/FactoryReturnTypePassTest.php
@@ -20,6 +20,8 @@

 /**
 * @author Guilhem N. <egetick@gmail.com>
+ *
+ * @group legacy
 */
 class FactoryReturnTypePassTest extends \PHPUnit_Framework_TestCase
 {
@@ -103,17 +105,16 @@ public function testCircularReference()
        $this->assertNull($factory2->getClass());
    }

+    /**
+     * @requires function ReflectionMethod::getReturnType
+     * @expectedDeprecation Relying on its factory's return-type to define the class of service "factory" is deprecated since Symfony 3.3 and won't work in 4.0. Set the "class" attribute to "Symfony\Component\DependencyInjection\Tests\Fixtures\FactoryDummy" on the service definition instead.
+     */
    public function testCompile()
    {
        $container = new ContainerBuilder();

        $factory = $container->register('factory');
        $factory->setFactory(array(FactoryDummy::class, 'createFactory'));
-
-        if (!method_exists(\ReflectionMethod::class, 'getReturnType')) {
-            $this->setExpectedException(\RuntimeException::class, 'Please add the class to service "factory" even if it is constructed by a factory since we might need to add method calls based on compile-time checks.');
-        }
-
        $container->compile();

        $this->assertEquals(FactoryDummy::class, $container->getDefinition('factory')->getClass());
-Original file line number
+Diff line change
@@ -303,7 +303,7 @@ private function processAnonymousServices(\DOMDocument $xml, $file)
             if (false !== $nodes = $xpath->query('//container:argument[@type="service"][not(@id)]|//container:property[@type="service"][not(@id)]')) {
                 foreach ($nodes as $node) {
                     // give it a unique name
-                    $id = sprintf('%s_%d', hash('sha256', $file), ++$count);
+                    $id = sprintf('%d_%s', ++$count, hash('sha256', $file));
             Copy link

  
      
    
  

  
      

  
  Member


      

  Author


  

  
    
      

      
            nicolas-grekas
  

      

      

      


        Jan 3, 2017


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      just made these changes so that anonymous classes won't have their class set to a random string (+ added corresponding regexp check in ResolveClassPass)
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



    
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  

  
    
      

      
            dunglas
  

      

      

      


        Jan 3, 2017


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      Why hashing it? Wouldn't it be more explicit with the plain file name?
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
      
        
            
    All reactions
  


          
          
        
      
    

    



    
        
  
    
        
    
  


      
          
  
      
            Copy link

  
      
    
  

  
      

  
  Member


      

  Author


  

  
    
      

      
            nicolas-grekas
  

      

      

      


        Jan 3, 2017


      
    

  


        
      
  
  
  
    

    There was a problem hiding this comment.


  

 
  
    

    Choose a reason for hiding this comment

    
      The reason will be displayed to describe this comment to others. Learn more.
    

    
      
      


  


  
    
      not related to this PR :)
    
  
  


    

        
      
  
  
    
  
  
  
    
    Sorry, something went wrong.
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.


  
  


          
      
  
    
    
            dunglas reacted with thumbs up emoji
      
        
            
    All reactions
  


          
              
                👍
                  1 reaction
                     $node->setAttribute('id', $id);
                     if ($services = $this->getChildren($node, 'service')) {
@@ -321,15 +321,15 @@ private function processAnonymousServices(\DOMDocument $xml, $file)
             if (false !== $nodes = $xpath->query('//container:services/container:service[not(@id)]')) {
                 foreach ($nodes as $node) {
                     // give it a unique name
-                    $id = sprintf('%s_%d', hash('sha256', $file), ++$count);
+                    $id = sprintf('%d_%s', ++$count, hash('sha256', $file));
                     $node->setAttribute('id', $id);
                     $definitions[$id] = array($node, $file, true);
                 }
             }
             // resolve definitions
-            krsort($definitions);
-            foreach ($definitions as $id => list($domElement, $file, $wild)) {
+            uksort($definitions, 'strnatcmp');
+            foreach (array_reverse($definitions) as $id => list($domElement, $file, $wild)) {
                 if (null !== $definition = $this->parseDefinition($domElement, $file)) {
                     $this->container->setDefinition($id, $definition);
                 }
-          Expand Down