[DependencyInjection] Create explicit factoryClass property for Defin…

…itions Previously, the Definition class was used both for type inference and factory construction (if factoryService was absent). This is fine for cases where classes create instances of themselves (e.g. getInstance() or create()), but leads to ambiguity when we have a separate factory class.
symfony · jmikola · Feb 5, 2011 · Jan 24, 2011 · Feb 5, 2011 · Feb 2, 2011
commit 743f25a287ebcd0cc6b58e984e19544108b6ba28
diff --git a/src/Symfony/Bundle/DoctrineBundle/DependencyInjection/DoctrineExtension.php b/src/Symfony/Bundle/DoctrineBundle/DependencyInjection/DoctrineExtension.php
@@ -408,6 +408,7 @@ protected function loadOrmEntityManager(array $entityManager, ContainerBuilder $
             new Reference(sprintf('doctrine.orm.%s_configuration', $entityManager['name']))
         );
         $ormEmDef = new Definition('%doctrine.orm.entity_manager_class%', $ormEmArgs);
+        $ormEmDef->setFactoryClass('%doctrine.orm.entity_manager_class%');
         $ormEmDef->setFactoryMethod('create');
         $ormEmDef->addTag('doctrine.orm.entity_manager');
         $container->setDefinition($entityManagerService, $ormEmDef);

diff --git a/...Symfony/Bundle/DoctrineBundle/Tests/DependencyInjection/AbstractDoctrineExtensionTest.php b/...Symfony/Bundle/DoctrineBundle/Tests/DependencyInjection/AbstractDoctrineExtensionTest.php
@@ -157,6 +157,7 @@ public function testDependencyInjectionConfigurationDefaults()
 
         $definition = $container->getDefinition('doctrine.orm.default_entity_manager');
         $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.orm.entity_manager', $definition->getTags());
 
@@ -198,6 +199,7 @@ public function testSingleEntityManagerConfiguration()
 
         $definition = $container->getDefinition('doctrine.orm.default_entity_manager');
         $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.orm.entity_manager', $definition->getTags());
 
@@ -239,6 +241,7 @@ public function testLoadSimpleSingleConnection()
 
         $definition = $container->getDefinition('doctrine.orm.default_entity_manager');
         $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.orm.entity_manager', $definition->getTags());
 
@@ -279,6 +282,7 @@ public function testLoadSingleConnection()
 
         $definition = $container->getDefinition('doctrine.orm.default_entity_manager');
         $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.orm.entity_manager', $definition->getTags());
 
@@ -313,6 +317,7 @@ public function testLoadMultipleConnections()
 
         $definition = $container->getDefinition('doctrine.orm.dm1_entity_manager');
         $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.orm.entity_manager', $definition->getTags());
 
@@ -334,6 +339,7 @@ public function testLoadMultipleConnections()
 
         $definition = $container->getDefinition('doctrine.orm.dm2_entity_manager');
         $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.orm.entity_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.orm.entity_manager', $definition->getTags());
 

diff --git a/src/Symfony/Bundle/DoctrineMongoDBBundle/DependencyInjection/DoctrineMongoDBExtension.php b/src/Symfony/Bundle/DoctrineMongoDBBundle/DependencyInjection/DoctrineMongoDBExtension.php
@@ -165,6 +165,7 @@ protected function loadDocumentManager(array $documentManager, ContainerBuilder
             new Reference($eventManagerId),
         );
         $odmDmDef = new Definition('%doctrine.odm.mongodb.document_manager_class%', $odmDmArgs);
+        $odmDmDef->setFactoryClass('%doctrine.odm.mongodb.document_manager_class%');
         $odmDmDef->setFactoryMethod('create');
         $odmDmDef->addTag('doctrine.odm.mongodb.document_manager');
         $container->setDefinition(sprintf('doctrine.odm.mongodb.%s_document_manager', $documentManager['name']), $odmDmDef);

diff --git a/...y/Bundle/DoctrineMongoDBBundle/Tests/DependencyInjection/AbstractMongoDBExtensionTest.php b/...y/Bundle/DoctrineMongoDBBundle/Tests/DependencyInjection/AbstractMongoDBExtensionTest.php
@@ -65,6 +65,7 @@ public function testDependencyInjectionConfigurationDefaults()
 
         $definition = $container->getDefinition('doctrine.odm.mongodb.default_document_manager');
         $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.odm.mongodb.document_manager', $definition->getTags());
 
@@ -92,6 +93,7 @@ public function testSingleDocumentManagerConfiguration()
 
         $definition = $container->getDefinition('doctrine.odm.mongodb.default_document_manager');
         $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.odm.mongodb.document_manager', $definition->getTags());
 
@@ -126,6 +128,7 @@ public function testLoadSimpleSingleConnection()
 
         $definition = $container->getDefinition('doctrine.odm.mongodb.default_document_manager');
         $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.odm.mongodb.document_manager', $definition->getTags());
 
@@ -154,6 +157,7 @@ public function testLoadSingleConnection()
 
         $definition = $container->getDefinition('doctrine.odm.mongodb.default_document_manager');
         $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.odm.mongodb.document_manager', $definition->getTags());
 
@@ -184,6 +188,7 @@ public function testLoadMultipleConnections()
 
         $definition = $container->getDefinition('doctrine.odm.mongodb.dm1_document_manager');
         $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.odm.mongodb.document_manager', $definition->getTags());
 
@@ -199,6 +204,7 @@ public function testLoadMultipleConnections()
 
         $definition = $container->getDefinition('doctrine.odm.mongodb.dm2_document_manager');
         $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getClass());
+        $this->assertEquals('%doctrine.odm.mongodb.document_manager_class%', $definition->getFactoryClass());
         $this->assertEquals('create', $definition->getFactoryMethod());
         $this->assertArrayHasKey('doctrine.odm.mongodb.document_manager', $definition->getTags());
 

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/CheckDefinitionValidityPass.php b/src/Symfony/Component/DependencyInjection/Compiler/CheckDefinitionValidityPass.php
@@ -41,7 +41,7 @@ public function process(ContainerBuilder $container)
 
             // non-synthetic, non-abstract service has class
             if (!$definition->isAbstract() && !$definition->isSynthetic() && !$definition->getClass()) {
-                if ($definition->getFactoryService()) {
+                if ($definition->getFactoryClass() || $definition->getFactoryService()) {
                     throw new \RuntimeException(sprintf(
                         'Please add the class to service "%s" even if it is constructed '
                        .'by a factory since we might need to add method calls based on '

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveDefinitionTemplatesPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveDefinitionTemplatesPass.php
@@ -49,8 +49,9 @@ protected function resolveDefinition($id, DefinitionDecorator $definition)
         $def->setClass($parentDef->getClass());
         $def->setArguments($parentDef->getArguments());
         $def->setMethodCalls($parentDef->getMethodCalls());
-        $def->setFactoryService($parentDef->getFactoryService());
+        $def->setFactoryClass($parentDef->getFactoryClass());
         $def->setFactoryMethod($parentDef->getFactoryMethod());
+        $def->setFactoryService($parentDef->getFactoryService());
         $def->setConfigurator($parentDef->getConfigurator());
         $def->setFile($parentDef->getFile());
         $def->setPublic($parentDef->isPublic());
@@ -60,6 +61,9 @@ protected function resolveDefinition($id, DefinitionDecorator $definition)
         if (isset($changes['class'])) {
             $def->setClass($definition->getClass());
         }
+        if (isset($changes['factory_class'])) {
+            $def->setFactoryClass($definition->getFactoryClass());
+        }
         if (isset($changes['factory_method'])) {
             $def->setFactoryMethod($definition->getFactoryMethod());
         }

diff --git a/src/Symfony/Component/DependencyInjection/Compiler/ResolveInterfaceInjectorsPass.php b/src/Symfony/Component/DependencyInjection/Compiler/ResolveInterfaceInjectorsPass.php
@@ -32,7 +32,7 @@ public function process(ContainerBuilder $container)
 
             $loaded = false;
             foreach ($container->getInterfaceInjectors() as $injector) {
-                if (null !== $definition->getFactoryService()) {
+                if (null !== $definition->getFactoryClass() || null !== $definition->getFactoryService()) {
                     continue;
                 }
 

diff --git a/src/Symfony/Component/DependencyInjection/ContainerBuilder.php b/src/Symfony/Component/DependencyInjection/ContainerBuilder.php
@@ -695,10 +695,12 @@ protected function createService(Definition $definition, $id)
         $arguments = $this->resolveServices($this->getParameterBag()->resolveValue($definition->getArguments()));
 
         if (null !== $definition->getFactoryMethod()) {
-            if (null !== $definition->getFactoryService()) {
+            if (null !== $definition->getFactoryClass()) {
+                $factory = $this->getParameterBag()->resolveValue($definition->getFactoryClass());
+            } elseif (null !== $definition->getFactoryService()) {
                 $factory = $this->get($this->getParameterBag()->resolveValue($definition->getFactoryService()));
             } else {
-                $factory = $this->getParameterBag()->resolveValue($definition->getClass());
+                throw new \RuntimeException('Cannot create service from factory method without a factory service or factory class.');
             }
 
             $service = call_user_func_array(array($factory, $definition->getFactoryMethod()), $arguments);

diff --git a/src/Symfony/Component/DependencyInjection/Definition.php b/src/Symfony/Component/DependencyInjection/Definition.php
@@ -20,6 +20,7 @@ class Definition
 {
     protected $class;
     protected $file;
+    protected $factoryClass;
     protected $factoryMethod;
     protected $factoryService;
     protected $scope;
@@ -49,16 +50,41 @@ public function __construct($class = null, array $arguments = array())
         $this->abstract = false;
     }
 
+    /**
+     * Sets the name of the class that acts as a factory using the factory method,
+     * which will be invoked statically.
+     *
+     * @param  string $factoryClass The factory class name
+     *
+     * @return Definition The current instance
+     */
+    public function setFactoryClass($factoryClass)
+    {
+        $this->factoryClass = $factoryClass;
+
+        return $this;
+    }
+
+    /**
+     * Gets the factory class.
+     *
+     * @return string The factory class name
+     */
+    public function getFactoryClass()
+    {
+        return $this->factoryClass;
+    }
+
     /**
      * Sets the factory method able to create an instance of this class.
      *
-     * @param  string $method The method name
+     * @param  string $factoryMethod The factory method name
      *
      * @return Definition The current instance
      */
-    public function setFactoryMethod($method)
+    public function setFactoryMethod($factoryMethod)
     {
-        $this->factoryMethod = $method;
+        $this->factoryMethod = $factoryMethod;
 
         return $this;
     }
@@ -74,7 +100,7 @@ public function getFactoryMethod()
     }
 
     /**
-     * Sets the name of the service that acts as a factory using the constructor method.
+     * Sets the name of the service that acts as a factory using the factory method.
      *
      * @param string $factoryService The factory service id
      *

diff --git a/src/Symfony/Component/DependencyInjection/DefinitionDecorator.php b/src/Symfony/Component/DependencyInjection/DefinitionDecorator.php
@@ -37,11 +37,11 @@ public function setClass($class)
         return parent::setClass($class);
     }
 
-    public function setFactoryService($service)
+    public function setFactoryClass($class)
     {
-        $this->changes['factory_service'] = true;
+        $this->changes['factory_class'] = true;
 
-        return parent::setFactoryService($service);
+        return parent::setFactoryClass($class);
     }
 
     public function setFactoryMethod($method)
@@ -51,6 +51,13 @@ public function setFactoryMethod($method)
         return parent::setFactoryMethod($method);
     }
 
+    public function setFactoryService($service)
+    {
+        $this->changes['factory_service'] = true;
+
+        return parent::setFactoryService($service);
+    }
+
     public function setConfigurator($callable)
     {
         $this->changes['configurator'] = true;

diff --git a/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php b/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php
@@ -230,10 +230,12 @@ protected function addServiceInlinedDefinitions($id, $definition)
                 }
 
                 if (null !== $sDefinition->getFactoryMethod()) {
-                    if (null !== $sDefinition->getFactoryService()) {
+                    if (null !== $sDefinition->getFactoryClass()) {
+                        $code .= sprintf("        \$%s = call_user_func(array(%s, '%s')%s);\n", $name, $this->dumpValue($sDefinition->getFactoryClass()), $sDefinition->getFactoryMethod(), count($arguments) > 0 ? ', '.implode(', ', $arguments) : '');
+                    } elseif (null !== $sDefinition->getFactoryService()) {
                         $code .= sprintf("        \$%s = %s->%s(%s);\n", $name, $this->getServiceCall($sDefinition->getFactoryService()), $sDefinition->getFactoryMethod(), implode(', ', $arguments));
                     } else {
-                        $code .= sprintf("        \$%s = call_user_func(array(%s, '%s')%s);\n", $name, $class, $sDefinition->getFactoryMethod(), count($arguments) > 0 ? ', '.implode(', ', $arguments) : '');
+                        throw new \RuntimeException('Factory service or factory class must be defined in service definition for '.$id);
                     }
                 } elseif (false !== strpos($class, '$')) {
                     $code .= sprintf("        \$class = %s;\n        \$%s = new \$class(%s);\n", $class, $name, implode(', ', $arguments));
@@ -294,10 +296,12 @@ protected function addServiceInstance($id, $definition)
         }
 
         if (null !== $definition->getFactoryMethod()) {
-            if (null !== $definition->getFactoryService()) {
+            if (null !== $definition->getFactoryClass()) {
+                $code = sprintf("        $return{$instantiation}call_user_func(array(%s, '%s')%s);\n", $this->dumpValue($definition->getFactoryClass()), $definition->getFactoryMethod(), $arguments ? ', '.implode(', ', $arguments) : '');
+            } elseif (null !== $definition->getFactoryService()) {
                 $code = sprintf("        $return{$instantiation}%s->%s(%s);\n", $this->getServiceCall($definition->getFactoryService()), $definition->getFactoryMethod(), implode(', ', $arguments));
             } else {
-                $code = sprintf("        $return{$instantiation}call_user_func(array(%s, '%s')%s);\n", $class, $definition->getFactoryMethod(), $arguments ? ', '.implode(', ', $arguments) : '');
+                throw new \RuntimeException('Factory method requires a factory service or factory class in service definition for '.$id);
             }
         } elseif (false !== strpos($class, '$')) {
             $code = sprintf("        \$class = %s;\n        $return{$instantiation}new \$class(%s);\n", $class, implode(', ', $arguments));
@@ -404,8 +408,10 @@ protected function addService($id, $definition)
         $return = '';
         if ($definition->isSynthetic()) {
             $return = sprintf('@throws \RuntimeException always since this service is expected to be injected dynamically');
-        } else if ($class = $definition->getClass()) {
+        } elseif ($class = $definition->getClass()) {
             $return = sprintf("@return %s A %s instance.", 0 === strpos($class, '%') ? 'Object' : $class, $class);
+        } elseif ($definition->getFactoryClass()) {
+            $return = sprintf('@return Object An instance returned by %s::%s().', $definition->getFactoryClass(), $definition->getFactoryMethod());
         } elseif ($definition->getFactoryService()) {
             $return = sprintf('@return Object An instance returned by %s::%s().', $definition->getFactoryService(), $definition->getFactoryMethod());
         }
@@ -821,10 +827,12 @@ protected function dumpValue($value, $interpolate = true)
             }
 
             if (null !== $value->getFactoryMethod()) {
-                if (null !== $value->getFactoryService()) {
+                if (null !== $value->getFactoryClass()) {
+                    return sprintf("call_user_func(array(%s, '%s')%s)", $this->dumpValue($value->getFactoryClass()), $value->getFactoryMethod(), count($arguments) > 0 ? ', '.implode(', ', $arguments) : '');
+                } elseif (null !== $value->getFactoryService()) {
                     return sprintf("%s->%s(%s)", $this->getServiceCall($value->getFactoryService()), $value->getFactoryMethod(), implode(', ', $arguments));
                 } else {
-                    return sprintf("call_user_func(array(%s, '%s')%s)", $class, $value->getFactoryMethod(), count($arguments) > 0 ? ', '.implode(', ', $arguments) : '');
+                    throw new \RuntimeException('Cannot dump definitions which have factory method without factory service or factory class.');
                 }
             }
 

diff --git a/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php
@@ -143,7 +143,7 @@ protected function parseDefinition($id, $service, $file)
             $definition = new Definition();
         }
 
-        foreach (array('class', 'scope', 'public', 'factory-method', 'factory-service', 'synthetic', 'abstract') as $key) {
+        foreach (array('class', 'scope', 'public', 'factory-class', 'factory-method', 'factory-service', 'synthetic', 'abstract') as $key) {
             if (isset($service[$key])) {
                 $method = 'set'.str_replace('-', '', $key);
                 $definition->$method((string) $service->getAttributeAsPhp($key));

diff --git a/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php b/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php
@@ -165,6 +165,10 @@ protected function parseDefinition($id, $service, $file)
             $definition->setAbstract($service['abstract']);
         }
 
+        if (isset($service['factory_class'])) {
+            $definition->setFactoryClass($service['factory_class']);
+        }
+
         if (isset($service['factory_method'])) {
             $definition->setFactoryMethod($service['factory_method']);
         }

diff --git a/src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd b/src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd
@@ -105,6 +105,7 @@
     <xsd:attribute name="public" type="boolean" />
     <xsd:attribute name="synthetic" type="boolean" />
     <xsd:attribute name="abstract" type="boolean" />
+    <xsd:attribute name="factory-class" type="xsd:string" />
     <xsd:attribute name="factory-method" type="xsd:string" />
     <xsd:attribute name="factory-service" type="xsd:string" />
     <xsd:attribute name="alias" type="xsd:string" />