Cleaning up logic of the proxy factory by moving closure generation to own private methods

lib/Doctrine/ORM/Proxy/ProxyFactory.php

@@ -19,26 +19,29 @@
 
 namespace Doctrine\ORM\Proxy;
 
-use Doctrine\ORM\EntityManager;
+use Doctrine\Common\Persistence\Mapping\ClassMetadata;
-use Doctrine\ORM\EntityNotFoundException;
+use Doctrine\Common\Proxy\AbstractProxyFactory;
-
+use Doctrine\Common\Proxy\ProxyDefinition;
 use Doctrine\Common\Util\ClassUtils;
 use Doctrine\Common\Proxy\Proxy;
 use Doctrine\Common\Proxy\ProxyGenerator;
 use Doctrine\ORM\ORMInvalidArgumentException;
+use Doctrine\ORM\Persisters\BasicEntityPersister;
+use Doctrine\ORM\EntityManager;
+use Doctrine\ORM\EntityNotFoundException;
 
 /**
  * This factory is used to create proxy objects for entities at runtime.
  *
  * @author Roman Borschel <roman@code-factory.org>
  * @author Giorgio Sironi <piccoloprincipeazzurro@gmail.com>
- * @author Marco Pivetta <ocramius@gmail.com>
+ * @author Marco Pivetta  <ocramius@gmail.com>
  * @since 2.0
  */
-class ProxyFactory
+class ProxyFactory extends AbstractProxyFactory
 {
     /**
-     * @var EntityManager The EntityManager this factory is bound to.
+     * @var \Doctrine\ORM\EntityManager The EntityManager this factory is bound to.
      */
     private $em;
 
@@ -47,183 +50,73 @@ class ProxyFactory
      */
     private $uow;
 
-    /**
-     * @var ProxyGenerator the proxy generator responsible for creating the proxy classes/files.
-     */
-    private $proxyGenerator;
-
-    /**
-     * @var bool Whether to automatically (re)generate proxy classes.
-     */
-    private $autoGenerate;
-
     /**
      * @var string
      */
     private $proxyNs;
 
-    /**
-     * @var string
-     */
-    private $proxyDir;
-
-    /**
-     * @var array definitions (indexed by requested class name) for the proxy classes.
-     *            Each element is an array containing following items:
-     *            "fqcn"             - FQCN of the proxy class
-     *            "initializer"      - Closure to be used as proxy __initializer__
-     *            "cloner"           - Closure to be used as proxy __cloner__
-     *            "identifierFields" - list of field names for the identifiers
-     *            "reflectionFields" - ReflectionProperties for the fields
-     */
-    private $definitions = array();
-
     /**
      * Initializes a new instance of the <tt>ProxyFactory</tt> class that is
      * connected to the given <tt>EntityManager</tt>.
      *
-     * @param EntityManager $em           The EntityManager the new factory works for.
+     * @param \Doctrine\ORM\EntityManager $em           The EntityManager the new factory works for.
-     * @param string        $proxyDir     The directory to use for the proxy classes. It must exist.
+     * @param string                      $proxyDir     The directory to use for the proxy classes. It must exist.
-     * @param string        $proxyNs      The namespace to use for the proxy classes.
+     * @param string                      $proxyNs      The namespace to use for the proxy classes.
-     * @param boolean       $autoGenerate Whether to automatically generate proxy classes.
+     * @param boolean                     $autoGenerate Whether to automatically generate proxy classes.
      */
     public function __construct(EntityManager $em, $proxyDir, $proxyNs, $autoGenerate = false)
     {
-        $this->em           = $em;
+        $proxyGenerator = new ProxyGenerator($proxyDir, $proxyNs);
-        $this->uow          = $em->getUnitOfWork();
+
-        $this->proxyDir     = $proxyDir;
+        $proxyGenerator->setPlaceholder('baseProxyInterface', 'Doctrine\ORM\Proxy\Proxy');
-        $this->proxyNs      = $proxyNs;
+        parent::__construct($proxyGenerator, $em->getMetadataFactory(), $autoGenerate);
-        $this->autoGenerate = $autoGenerate;
+
+        $this->em      = $em;
+        $this->uow     = $em->getUnitOfWork();
+        $this->proxyNs = $proxyNs;
+
     }
 
     /**
-     * Gets a reference proxy instance for the entity of the given type and identified by
+     * {@inheritDoc}
-     * the given identifier.
-     *
-     * @param  string $className
-     * @param  mixed  $identifier
-     * @return object
      */
-    public function getProxy($className, $identifier)
+    protected function skipClass(ClassMetadata $metadata)
     {
-        if ( ! isset($this->definitions[$className])) {
+        /* @var $metadata \Doctrine\ORM\Mapping\ClassMetadataInfo */
-            $this->initProxyDefinitions($className);
+        return $metadata->isMappedSuperclass || $metadata->getReflectionClass()->isAbstract();
-        }
-
-        $definition         = $this->definitions[$className];
-        $fqcn               = $definition['fqcn'];
-        $identifierFields   = $definition['identifierFields'];
-        /* @var $reflectionFields \ReflectionProperty[] */
-        $reflectionFields   = $definition['reflectionFields'];
-        $proxy              = new $fqcn($definition['initializer'], $definition['cloner']);
-
-        foreach ($identifierFields as $idField) {
-            $reflectionFields[$idField]->setValue($proxy, $identifier[$idField]);
-        }
-
-        return $proxy;
     }
 
     /**
-     * @param \Doctrine\Common\Proxy\Proxy $proxy
+     * {@inheritDoc}
-     *
-     * @return \Doctrine\Common\Proxy\Proxy
-     *
-     * @throws \Doctrine\ORM\ORMInvalidArgumentException
      */
-    public function resetUninitializedProxy(Proxy $proxy)
+    protected function createProxyDefinition($className)
     {
-        if ($proxy->__isInitialized()) {
+        $classMetadata   = $this->em->getClassMetadata($className);
-            throw new ORMInvalidArgumentException('Provided proxy must not be initialized');
-        }
-
-        $className = $this->em->getClassMetadata(get_class($proxy))->getName();
-
-        if ( ! isset($this->definitions[$className])) {
-            $this->initProxyDefinitions($className);
-        }
-
-        $proxy->__setInitializer($this->definitions[$className]['initializer']);
-        $proxy->__setCloner($this->definitions[$className]['cloner']);
-
-        return $proxy;
-    }
-
-    /**
-     * Generates proxy classes for all given classes.
-     *
-     * @param \Doctrine\Common\Persistence\Mapping\ClassMetadata[] $classes The classes (ClassMetadata instances)
-     *                                                                      for which to generate proxies.
-     * @param string $proxyDir The target directory of the proxy classes. If not specified, the
-     *                      directory configured on the Configuration of the EntityManager used
-     *                      by this factory is used.
-     * @return int Number of generated proxies.
-     */
-    public function generateProxyClasses(array $classes, $proxyDir = null)
-    {
-        $generated = 0;
-
-        foreach ($classes as $class) {
-            /* @var $class \Doctrine\ORM\Mapping\ClassMetadataInfo */
-            if ($class->isMappedSuperclass || $class->getReflectionClass()->isAbstract()) {
-                continue;
-            }
-
-            $generator = $this->getProxyGenerator();
-
-            $proxyFileName = $generator->getProxyFileName($class->getName(), $proxyDir);
-            $generator->generateProxyClass($class, $proxyFileName);
-            $generated += 1;
-        }
-
-        return $generated;
-    }
-
-    /**
-     * @param ProxyGenerator $proxyGenerator
-     */
-    public function setProxyGenerator(ProxyGenerator $proxyGenerator)
-    {
-        $this->proxyGenerator = $proxyGenerator;
-    }
-
-    /**
-     * @return ProxyGenerator
-     */
-    public function getProxyGenerator()
-    {
-        if (null === $this->proxyGenerator) {
-            $this->proxyGenerator = new ProxyGenerator($this->proxyDir, $this->proxyNs);
-            $this->proxyGenerator->setPlaceholder('<baseProxyInterface>', 'Doctrine\ORM\Proxy\Proxy');
-        }
-
-        return $this->proxyGenerator;
-    }
-
-    /**
-     * @param string $className
-     */
-    private function initProxyDefinitions($className)
-    {
-        $classMetadata = $this->em->getClassMetadata($className);
-        $className     = $classMetadata->getName();
-        $fqcn          = ClassUtils::generateProxyClassName($className, $this->proxyNs);
-
-        if ( ! class_exists($fqcn, false)) {
-            $generator = $this->getProxyGenerator();
-            $fileName = $generator->getProxyFileName($className);
-
-            if ($this->autoGenerate) {
-                $generator->generateProxyClass($classMetadata);
-            }
-
-            require $fileName;
-        }
-
         $entityPersister = $this->uow->getEntityPersister($className);
 
+        return new ProxyDefinition(
+            ClassUtils::generateProxyClassName($className, $this->proxyNs),
+            $classMetadata->getIdentifierFieldNames(),
+            $classMetadata->getReflectionProperties(),
+            $this->createInitializer($classMetadata, $entityPersister),
+            $this->createCloner($classMetadata, $entityPersister)
+        );
+    }
+
+    /**
+     * Creates a closure capable of initializing a proxy
+     *
+     * @param \Doctrine\Common\Persistence\Mapping\ClassMetadata $classMetadata
+     * @param \Doctrine\ORM\Persisters\BasicEntityPersister      $entityPersister
+     *
+     * @return \Closure
+     *
+     * @throws \Doctrine\ORM\EntityNotFoundException
+     */
+    private function createInitializer(ClassMetadata $classMetadata, BasicEntityPersister $entityPersister)
+    {
         if ($classMetadata->getReflectionClass()->hasMethod('__wakeup')) {
-            $initializer = function (Proxy $proxy) use ($entityPersister, $classMetadata) {
+            return function (Proxy $proxy) use ($entityPersister, $classMetadata) {
                 $proxy->__setInitializer(null);
                 $proxy->__setCloner(null);
 
@@ -242,36 +135,49 @@ class ProxyFactory
                 $proxy->__setInitialized(true);
                 $proxy->__wakeup();
 
-                if (null === $entityPersister->load($classMetadata->getIdentifierValues($proxy), $proxy)) {
-                    throw new EntityNotFoundException();
-                }
-            };
-        } else {
-            $initializer = function (Proxy $proxy) use ($entityPersister, $classMetadata) {
-                $proxy->__setInitializer(null);
-                $proxy->__setCloner(null);
-
-                if ($proxy->__isInitialized()) {
-                    return;
-                }
-
-                $properties = $proxy->__getLazyProperties();
-
-                foreach ($properties as $propertyName => $property) {
-                    if (!isset($proxy->$propertyName)) {
-                        $proxy->$propertyName = $properties[$propertyName];
-                    }
-                }
-
-                $proxy->__setInitialized(true);
-
                 if (null === $entityPersister->load($classMetadata->getIdentifierValues($proxy), $proxy)) {
                     throw new EntityNotFoundException();
                 }
             };
         }
 
-        $cloner = function (Proxy $proxy) use ($entityPersister, $classMetadata) {
+        return function (Proxy $proxy) use ($entityPersister, $classMetadata) {
+            $proxy->__setInitializer(null);
+            $proxy->__setCloner(null);
+
+            if ($proxy->__isInitialized()) {
+                return;
+            }
+
+            $properties = $proxy->__getLazyProperties();
+
+            foreach ($properties as $propertyName => $property) {
+                if (!isset($proxy->$propertyName)) {
+                    $proxy->$propertyName = $properties[$propertyName];
+                }
+            }
+
+            $proxy->__setInitialized(true);
+
+            if (null === $entityPersister->load($classMetadata->getIdentifierValues($proxy), $proxy)) {
+                throw new EntityNotFoundException();
+            }
+        };
+    }
+
+    /**
+     * Creates a closure capable of finalizing state a cloned proxy
+     *
+     * @param \Doctrine\Common\Persistence\Mapping\ClassMetadata $classMetadata
+     * @param \Doctrine\ORM\Persisters\BasicEntityPersister      $entityPersister
+     *
+     * @return \Closure
+     *
+     * @throws \Doctrine\ORM\EntityNotFoundException
+     */
+    private function createCloner(ClassMetadata $classMetadata, BasicEntityPersister $entityPersister)
+    {
+        return function (Proxy $proxy) use ($entityPersister, $classMetadata) {
             if ($proxy->__isInitialized()) {
                 return;
             }
@@ -294,13 +200,5 @@ class ProxyFactory
                 }
             }
         };
-
-        $this->definitions[$className] = array(
-            'fqcn'             => $fqcn,
-            'initializer'      => $initializer,
-            'cloner'           => $cloner,
-            'identifierFields' => $classMetadata->getIdentifierFieldNames(),
-            'reflectionFields' => $classMetadata->getReflectionProperties(),
-        );
     }
 }

lib/Doctrine/ORM/UnitOfWork.php

@@ -666,9 +666,7 @@ class UnitOfWork implements PropertyChangedListener
 
         // Look for changes in associations of the entity
         foreach ($class->associationMappings as $field => $assoc) {
-            $val = $class->reflFields[$field]->getValue($entity);
+            if (($val = $class->reflFields[$field]->getValue($entity)) !== null) {
-
-            if (null !== $val) {
                 $this->computeAssociationChanges($assoc, $val);
                 if (!isset($this->entityChangeSets[$oid]) &&
                     $assoc['isOwningSide'] &&
@@ -1818,9 +1816,9 @@ class UnitOfWork implements PropertyChangedListener
             }
 
             if ($class->isVersioned) {
-                $reflField = $class->reflFields[$class->versionField];
+                $reflField          = $class->reflFields[$class->versionField];
                 $managedCopyVersion = $reflField->getValue($managedCopy);
-                $entityVersion = $reflField->getValue($entity);
+                $entityVersion      = $reflField->getValue($entity);
 
                 // Throw exception if versions dont match.
                 if ($managedCopyVersion != $entityVersion) {
@@ -1832,17 +1830,14 @@ class UnitOfWork implements PropertyChangedListener
             foreach ($class->reflClass->getProperties() as $prop) {
                 $name = $prop->name;
                 $prop->setAccessible(true);
-
                 if ( ! isset($class->associationMappings[$name])) {
                     if ( ! $class->isIdentifier($name)) {
                         $prop->setValue($managedCopy, $prop->getValue($entity));
                     }
                 } else {
                     $assoc2 = $class->associationMappings[$name];
-
                     if ($assoc2['type'] & ClassMetadata::TO_ONE) {
                         $other = $prop->getValue($entity);
-
                         if ($other === null) {
                             $prop->setValue($managedCopy, null);
                         } else if ($other instanceof Proxy && !$other->__isInitialized__) {
@@ -1864,7 +1859,6 @@ class UnitOfWork implements PropertyChangedListener
                         }
                     } else {
                         $mergeCol = $prop->getValue($entity);
-
                         if ($mergeCol instanceof PersistentCollection && !$mergeCol->isInitialized()) {
                             // do not merge fields marked lazy that have not been fetched.
                             // keep the lazy persistent collection of the managed copy.
@@ -1872,7 +1866,6 @@ class UnitOfWork implements PropertyChangedListener
                         }
 
                         $managedCol = $prop->getValue($managedCopy);
-
                         if (!$managedCol) {
                             $managedCol = new PersistentCollection($this->em,
                                     $this->em->getClassMetadata($assoc2['targetEntity']),
@@ -2497,6 +2490,7 @@ class UnitOfWork implements PropertyChangedListener
 
             if ($entity instanceof Proxy && ! $entity->__isInitialized()) {
                 $entity->__setInitialized(true);
+
                 $overrideLocalValues = true;
 
                 if ($entity instanceof NotifyPropertyChanged) {

lib/vendor/doctrine-common

@@ -1 +1 @@
-Subproject commit 0aa165610e2fdd6617e0e2b91c35fedf92aea2f7
+Subproject commit d5843a462a4dfe4a42daf4645fc867c431a4170e

tests/Doctrine/Tests/Models/DDC1734/DDC1734Article.php

@@ -1,37 +0,0 @@
-<?php
-
-namespace Doctrine\Tests\Models\DDC1734;
-
-/**
- * @Entity
- */
-class DDC1734Article
-{
-
-    /**
-     * @Id @Column(type="integer")
-     * @GeneratedValue
-     */
-    protected $id;
-
-    /**
-     * @Column(type="string")
-     */
-    protected $name;
-
-    public function __construct($name)
-    {
-        $this->name = $name;
-    }
-
-    public function getId()
-    {
-        return $this->id;
-    }
-
-    public function getName()
-    {
-        return $this->name;
-    }
-
-}

tests/Doctrine/Tests/ORM/Functional/Ticket/DDC1734Test.php

@@ -2,39 +2,44 @@
 
 namespace Doctrine\Tests\ORM\Functional\Ticket;
 
-use Doctrine\Tests\Models\DDC1734\DDC1734Article;
+use Doctrine\Tests\Models\CMS\CmsGroup;
-
-require_once __DIR__ . '/../../../TestInit.php';
 
 class DDC1734Test extends \Doctrine\Tests\OrmFunctionalTestCase
 {
-
+    /**
+     * {@inheritDoc}
+     */
     protected function setUp()
     {
-        $this->useModelSet('ddc1734');
+        $this->useModelSet('cms');
         parent::setUp();
     }
 
     /**
      * This test is DDC-1734 minus the serialization, i.e. it works
+     *
      * @group DDC-1734
      */
     public function testMergeWorksOnNonSerializedProxies()
     {
-        $article = new DDC1734Article("Foo");
+        $group = new CmsGroup();
-        $this->_em->persist($article);
+
+        $group->setName('Foo');
+        $this->_em->persist($group);
         $this->_em->flush();
-        // Get a proxy of the entity
         $this->_em->clear();
-        $proxy = $this->getProxy($article);
+
+        $proxy = $this->getProxy($group);
+
         $this->assertInstanceOf('Doctrine\ORM\Proxy\Proxy', $proxy);
-        $this->assertFalse($proxy->__isInitialized__);
+        $this->assertFalse($proxy->__isInitialized());
-        // Detach
+
         $this->_em->detach($proxy);
         $this->_em->clear();
-        // Merge
+
         $proxy = $this->_em->merge($proxy);
-        $this->assertEquals("Foo", $proxy->getName(), "The entity is broken");
+
+        $this->assertEquals('Foo', $proxy->getName(), 'The entity is broken');
     }
 
     /**
@@ -42,37 +47,43 @@ class DDC1734Test extends \Doctrine\Tests\OrmFunctionalTestCase
      * - A non-initialized proxy is detached and serialized (the identifier of the proxy is *not* serialized)
      * - the object is deserialized and merged (to turn into an entity)
      * - the entity is broken because it has no identifier and no field defined
+     *
      * @group DDC-1734
      */
     public function testMergeWorksOnSerializedProxies()
     {
-        $article = new DDC1734Article("Foo");
+        $group = new CmsGroup();
-        $this->_em->persist($article);
+
+        $group->setName('Foo');
+        $this->_em->persist($group);
         $this->_em->flush();
-        // Get a proxy of the entity
         $this->_em->clear();
-        $proxy = $this->getProxy($article);
+
+        $proxy = $this->getProxy($group);
+
         $this->assertInstanceOf('Doctrine\ORM\Proxy\Proxy', $proxy);
         $this->assertFalse($proxy->__isInitialized());
-        // Detach and serialize
+
         $this->_em->detach($proxy);
         $serializedProxy = serialize($proxy);
         $this->_em->clear();
-        // Unserialize and merge
+
-        /** @var $unserializedProxy DDC1734Article */
+        $unserializedProxy = $this->_em->merge(unserialize($serializedProxy));
-        $unserializedProxy = unserialize($serializedProxy);
+        $this->assertEquals('Foo', $unserializedProxy->getName(), 'The entity is broken');
-        // Merge
-        $unserializedProxy = $this->_em->merge($unserializedProxy);
-        $this->assertEquals("Foo", $unserializedProxy->getName(), "The entity is broken");
     }
 
+    /**
+     * @param object $object
+     *
+     * @return \Doctrine\Common\Proxy\Proxy
+     */
     private function getProxy($object)
     {
         $metadataFactory = $this->_em->getMetadataFactory();
-        $identifier      = $metadataFactory->getMetadataFor(get_class($object))->getIdentifierValues($object);
+        $className       = get_class($object);
-        $proxyFactory    = $this->_em->getProxyFactory();
+        $identifier      = $metadataFactory->getMetadataFor($className)->getIdentifierValues($object);
 
-        return $proxyFactory->getProxy(get_class($object), $identifier);
+        return $this->_em->getProxyFactory()->getProxy($className, $identifier);
     }
 
 }

tests/Doctrine/Tests/ORM/Proxy/ProxyFactoryTest.php

@@ -2,6 +2,7 @@
 
 namespace Doctrine\Tests\ORM\Proxy;
 
+use Doctrine\ORM\Mapping\ClassMetadata;
 use Doctrine\ORM\Proxy\ProxyFactory;
 use Doctrine\Common\Proxy\ProxyGenerator;
 use Doctrine\Tests\Mocks\ConnectionMock;
@@ -9,13 +10,11 @@ use Doctrine\Tests\Mocks\EntityManagerMock;
 use Doctrine\Tests\Mocks\UnitOfWorkMock;
 use Doctrine\Tests\Mocks\DriverMock;
 
-require_once __DIR__ . '/../../TestInit.php';
-
 /**
  * Test the proxy generator. Its work is generating on-the-fly subclasses of a given model, which implement the Proxy pattern.
  * @author Giorgio Sironi <piccoloprincipeazzurro@gmail.com>
  */
-class ProxtFactoryTest extends \Doctrine\Tests\OrmTestCase
+class ProxyFactoryTest extends \Doctrine\Tests\OrmTestCase
 {
     /**
      * @var ConnectionMock
@@ -73,7 +72,7 @@ class ProxtFactoryTest extends \Doctrine\Tests\OrmTestCase
      */
     public function testSkipAbstractClassesOnGeneration()
     {
-        $cm = new \Doctrine\ORM\Mapping\ClassMetadata(__NAMESPACE__ . '\\AbstractClass');
+        $cm = new ClassMetadata(__NAMESPACE__ . '\\AbstractClass');
         $cm->initializeReflection(new \Doctrine\Common\Persistence\Mapping\RuntimeReflectionService);
         $this->assertNotNull($cm->reflClass);
 

tests/Doctrine/Tests/OrmFunctionalTestCase.php

@@ -134,9 +134,6 @@ abstract class OrmFunctionalTestCase extends OrmTestCase
             'Doctrine\Tests\Models\DDC117\DDC117Editor',
             'Doctrine\Tests\Models\DDC117\DDC117Link',
         ),
-        'ddc1734' => array(
-            'Doctrine\Tests\Models\DDC1734\DDC1734Article',
-        ),
         'stockexchange' => array(
             'Doctrine\Tests\Models\StockExchange\Bond',
             'Doctrine\Tests\Models\StockExchange\Stock',