Merge pull request #1143 from MajorCaiger/master

Fixed a bug so that a versioned entity with a oneToOne id can be created

lib/Doctrine/ORM/Persisters/BasicEntityPersister.php

@@ -31,6 +31,7 @@ use Doctrine\ORM\Query;
 use Doctrine\ORM\PersistentCollection;
 use Doctrine\ORM\Mapping\MappingException;
 use Doctrine\ORM\Mapping\ClassMetadata;
+use Doctrine\ORM\Utility\IdentifierFlattener;
 
 use Doctrine\Common\Util\ClassUtils;
 use Doctrine\Common\Collections\Criteria;
@@ -76,6 +77,7 @@ use Doctrine\Common\Collections\Expr\Comparison;
  * @author Benjamin Eberlei <kontakt@beberlei.de>
  * @author Alexander <iam.asm89@gmail.com>
  * @author Fabio B. Silva <fabio.bat.silva@gmail.com>
+ * @author Rob Caiger <rob@clocal.co.uk>
  * @since 2.0
  */
 class BasicEntityPersister implements EntityPersister
@@ -206,6 +208,13 @@ class BasicEntityPersister implements EntityPersister
      */
     protected $quoteStrategy;
 
+    /**
+     * The IdentifierFlattener used for manipulating identifiers
+     *
+     * @var \Doctrine\ORM\Utility\IdentifierFlattener
+     */
+    private $identifierFlattener;
+
     /**
      * Initializes a new <tt>BasicEntityPersister</tt> that uses the given EntityManager
      * and persists instances of the class described by the given ClassMetadata descriptor.
@@ -215,11 +224,12 @@ class BasicEntityPersister implements EntityPersister
      */
     public function __construct(EntityManager $em, ClassMetadata $class)
     {
-        $this->em               = $em;
+        $this->em                  = $em;
-        $this->class            = $class;
+        $this->class               = $class;
-        $this->conn             = $em->getConnection();
+        $this->conn                = $em->getConnection();
-        $this->platform         = $this->conn->getDatabasePlatform();
+        $this->platform            = $this->conn->getDatabasePlatform();
-        $this->quoteStrategy    = $em->getConfiguration()->getQuoteStrategy();
+        $this->quoteStrategy       = $em->getConfiguration()->getQuoteStrategy();
+        $this->identifierFlattener = new IdentifierFlattener($em->getUnitOfWork(), $em->getMetadataFactory());
     }
 
     /**
@@ -338,7 +348,9 @@ class BasicEntityPersister implements EntityPersister
              . ' FROM '  . $tableName
              . ' WHERE ' . implode(' = ? AND ', $identifier) . ' = ?';
 
-        $value = $this->conn->fetchColumn($sql, array_values((array) $id));
+        $flatId = $this->identifierFlattener->flattenIdentifier($versionedClass, (array) $id);
+
+        $value = $this->conn->fetchColumn($sql, array_values($flatId));
 
         return Type::getType($versionedClass->fieldMappings[$versionField]['type'])->convertToPHPValue($value, $this->platform);
     }

lib/Doctrine/ORM/UnitOfWork.php

@@ -45,6 +45,7 @@ use Doctrine\ORM\Persisters\SingleTablePersister;
 use Doctrine\ORM\Persisters\JoinedSubclassPersister;
 use Doctrine\ORM\Persisters\OneToManyPersister;
 use Doctrine\ORM\Persisters\ManyToManyPersister;
+use Doctrine\ORM\Utility\IdentifierFlattener;
 
 /**
  * The UnitOfWork is responsible for tracking changes to objects during an
@@ -56,6 +57,7 @@ use Doctrine\ORM\Persisters\ManyToManyPersister;
  * @author      Guilherme Blanco <guilhermeblanco@hotmail.com>
  * @author      Jonathan Wage <jonwage@gmail.com>
  * @author      Roman Borschel <roman@code-factory.org>
+ * @author      Rob Caiger <rob@clocal.co.uk>
  * @internal    This class contains highly performance-sensitive code.
  */
 class UnitOfWork implements PropertyChangedListener
@@ -241,6 +243,13 @@ class UnitOfWork implements PropertyChangedListener
      */
     private $listenersInvoker;
 
+    /**
+     * The IdentifierFlattener used for manipulating identifiers
+     *
+     * @var \Doctrine\ORM\Utility\IdentifierFlattener
+     */
+    private $identifierFlattener;
+
     /**
      * Orphaned entities that are scheduled for removal.
      *
@@ -274,10 +283,11 @@ class UnitOfWork implements PropertyChangedListener
      */
     public function __construct(EntityManager $em)
     {
-        $this->em               = $em;
+        $this->em                  = $em;
-        $this->evm              = $em->getEventManager();
+        $this->evm                 = $em->getEventManager();
-        $this->listenersInvoker = new ListenersInvoker($em);
+        $this->listenersInvoker    = new ListenersInvoker($em);
-        $this->hasCache         = $em->getConfiguration()->isSecondLevelCacheEnabled();
+        $this->hasCache            = $em->getConfiguration()->isSecondLevelCacheEnabled();
+        $this->identifierFlattener = new IdentifierFlattener($this, $em->getMetadataFactory());
     }
 
     /**
@@ -1427,7 +1437,7 @@ class UnitOfWork implements PropertyChangedListener
         }
 
         if ($class->containsForeignIdentifier) {
-            $id = $this->flattenIdentifier($class, $id);
+            $id = $this->identifierFlattener->flattenIdentifier($class, $id);
         }
 
         switch (true) {
@@ -1752,31 +1762,6 @@ class UnitOfWork implements PropertyChangedListener
         return $this->doMerge($entity, $visited);
     }
 
-    /**
-     * convert foreign identifiers into scalar foreign key values to avoid object to string conversion failures.
-     *
-     * @param ClassMetadata $class
-     * @param array $id
-     * @return array
-     */
-    private function flattenIdentifier($class, $id)
-    {
-        $flatId = array();
-
-        foreach ($id as $idField => $idValue) {
-            if (isset($class->associationMappings[$idField])) {
-                $targetClassMetadata = $this->em->getClassMetadata($class->associationMappings[$idField]['targetEntity']);
-                $associatedId        = $this->getEntityIdentifier($idValue);
-
-                $flatId[$idField] = $associatedId[$targetClassMetadata->identifier[0]];
-            } else {
-                $flatId[$idField] = $idValue;
-            }
-        }
-
-        return $flatId;
-    }
-
     /**
      * Executes a merge operation on an entity.
      *
@@ -1826,7 +1811,7 @@ class UnitOfWork implements PropertyChangedListener
                 $this->persistNew($class, $managedCopy);
             } else {
                 $flatId = ($class->containsForeignIdentifier)
-                    ? $this->flattenIdentifier($class, $id)
+                    ? $this->identifierFlattener->flattenIdentifier($class, $id)
                     : $id;
 
                 $managedCopy = $this->tryGetById($flatId, $class->rootEntityName);
@@ -3361,10 +3346,10 @@ class UnitOfWork implements PropertyChangedListener
 
         $id1 = isset($this->entityIdentifiers[$oid1])
             ? $this->entityIdentifiers[$oid1]
-            : $this->flattenIdentifier($class, $class->getIdentifierValues($entity1));
+            : $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($entity1));
         $id2 = isset($this->entityIdentifiers[$oid2])
             ? $this->entityIdentifiers[$oid2]
-            : $this->flattenIdentifier($class, $class->getIdentifierValues($entity2));
+            : $this->identifierFlattener->flattenIdentifier($class, $class->getIdentifierValues($entity2));
 
         return $id1 === $id2 || implode(' ', $id1) === implode(' ', $id2);
     }

lib/Doctrine/ORM/Utility/IdentifierFlattener.php

@@ -0,0 +1,89 @@
+<?php
+/*
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ * This software consists of voluntary contributions made by many individuals
+ * and is licensed under the MIT license. For more information, see
+ * <http://www.doctrine-project.org>.
+ */
+
+
+namespace Doctrine\ORM\Utility;
+
+use Doctrine\ORM\UnitOfWork;
+use Doctrine\ORM\Mapping\ClassMetadata;
+use Doctrine\Common\Persistence\Mapping\ClassMetadataFactory;
+
+/**
+ * The IdentifierFlattener utility now houses some of the identifier manipulation logic from unit of work, so that it
+ * can be re-used elsewhere.
+ *
+ * @since       2.5
+ * @author      Rob Caiger <rob@clocal.co.uk>
+ */
+final class IdentifierFlattener
+{
+    /**
+     * The UnitOfWork used to coordinate object-level transactions.
+     *
+     * @var \Doctrine\ORM\UnitOfWork
+     */
+    private $unitOfWork;
+
+    /**
+     * The metadata factory, used to retrieve the ORM metadata of entity classes.
+     *
+     * @var \Doctrine\Common\Persistence\Mapping\ClassMetadataFactory
+     */
+    private $metadataFactory;
+
+    /**
+     * Initializes a new IdentifierFlattener instance, bound to the given EntityManager.
+     *
+     * @param \Doctrine\ORM\UnitOfWork $unitOfWork
+     * @param \Doctrine\Common\Persistence\Mapping\ClassMetadataFactory $metadataFactory
+     */
+    public function __construct(UnitOfWork $unitOfWork, ClassMetadataFactory $metadataFactory)
+    {
+        $this->unitOfWork = $unitOfWork;
+        $this->metadataFactory = $metadataFactory;
+    }
+
+    /**
+     * convert foreign identifiers into scalar foreign key values to avoid object to string conversion failures.
+     *
+     * @param \Doctrine\ORM\Mapping\ClassMetadata $class
+     * @param array $id
+     * @return array
+     */
+    public function flattenIdentifier(ClassMetadata $class, array $id)
+    {
+        $flatId = array();
+
+        foreach ($id as $idField => $idValue) {
+            if (isset($class->associationMappings[$idField]) && is_object($idValue)) {
+                $targetClassMetadata = $this->metadataFactory->getMetadataFor(
+                    $class->associationMappings[$idField]['targetEntity']
+                );
+
+                $associatedId = $this->unitOfWork->getEntityIdentifier($idValue);
+
+                $flatId[$idField] = $associatedId[$targetClassMetadata->identifier[0]];
+            } else {
+                $flatId[$idField] = $idValue;
+            }
+        }
+
+        return $flatId;
+    }
+}

tests/Doctrine/Tests/Models/VersionedOneToOne/FirstRelatedEntity.php

@@ -0,0 +1,32 @@
+<?php
+
+namespace Doctrine\Tests\Models\VersionedOneToOne;
+
+/**
+ * @author Rob Caiger <rob@clocal.co.uk>
+ *
+ * @Entity
+ * @Table(name="first_entity")
+ */
+class FirstRelatedEntity
+{
+    /**
+     * @Id
+     * @OneToOne(targetEntity="SecondRelatedEntity", fetch="EAGER")
+     * @JoinColumn(name="second_entity_id", referencedColumnName="id")
+     */
+    public $secondEntity;
+
+    /**
+     * @Column(name="name")
+     */
+    public $name;
+
+    /**
+     * Version column
+     *
+     * @Column(type="integer", name="version")
+     * @Version
+     */
+    public $version;
+}

tests/Doctrine/Tests/Models/VersionedOneToOne/SecondRelatedEntity.php

@@ -0,0 +1,32 @@
+<?php
+
+namespace Doctrine\Tests\Models\VersionedOneToOne;
+
+/**
+ * @author Rob Caiger <rob@clocal.co.uk>
+ *
+ * @Entity
+ * @Table(name="second_entity")
+ */
+class SecondRelatedEntity
+{
+    /**
+     * @Id
+     * @Column(name="id", type="integer")
+     * @GeneratedValue(strategy="AUTO")
+     */
+    public $id;
+
+    /**
+     * @Column(name="name")
+     */
+    public $name;
+
+    /**
+     * Version column
+     *
+     * @Column(type="integer", name="version")
+     * @Version
+     */
+    public $version;
+}

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

@@ -493,7 +493,7 @@ class DDC117Test extends \Doctrine\Tests\OrmFunctionalTestCase
      * @group DDC-117
      */
     public function testIndexByOnCompositeKeyField()
-    {   
+    {
         $article = $this->_em->find("Doctrine\Tests\Models\DDC117\DDC117Article", $this->article1->id());
 
         $this->assertInstanceOf('Doctrine\Tests\Models\DDC117\DDC117Article', $article);

tests/Doctrine/Tests/ORM/Functional/VersionedOneToOneTest.php

@@ -0,0 +1,62 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Functional;
+
+use Doctrine\Tests\Models\VersionedOneToOne\FirstRelatedEntity;
+use Doctrine\Tests\Models\VersionedOneToOne\SecondRelatedEntity;
+use Doctrine\ORM\ORMException;
+
+/**
+ * Tests that an entity with a OneToOne relationship defined as the id, with a version field can be created.
+ *
+ * @author Rob Caiger <rob@clocal.co.uk>
+ *
+ * @group VersionedOneToOne
+ */
+class VersionedOneToOneTest extends \Doctrine\Tests\OrmFunctionalTestCase
+{
+    protected function setUp()
+    {
+        parent::setUp();
+
+        try {
+            $this->_schemaTool->createSchema(
+                array(
+                    $this->_em->getClassMetadata('Doctrine\Tests\Models\VersionedOneToOne\FirstRelatedEntity'),
+                    $this->_em->getClassMetadata('Doctrine\Tests\Models\VersionedOneToOne\SecondRelatedEntity')
+                )
+            );
+        } catch (ORMException $e) {
+        }
+    }
+
+    /**
+     * This test case tests that a versionable entity, that has a oneToOne relationship as it's id can be created
+     *  without this bug fix (DDC-3318), you could not do this
+     */
+    public function testSetVersionOnCreate()
+    {
+        $secondRelatedEntity = new SecondRelatedEntity();
+        $secondRelatedEntity->name = 'Bob';
+
+        $this->_em->persist($secondRelatedEntity);
+        $this->_em->flush();
+
+        $firstRelatedEntity = new FirstRelatedEntity();
+        $firstRelatedEntity->name = 'Fred';
+        $firstRelatedEntity->secondEntity = $secondRelatedEntity;
+
+        $this->_em->persist($firstRelatedEntity);
+        $this->_em->flush();
+
+        $firstEntity = $this->_em->getRepository('Doctrine\Tests\Models\VersionedOneToOne\FirstRelatedEntity')
+            ->findOneBy(array('name' => 'Fred'));
+
+        $secondEntity = $this->_em->getRepository('Doctrine\Tests\Models\VersionedOneToOne\SecondRelatedEntity')
+            ->findOneBy(array('name' => 'Bob'));
+
+        $this->assertSame($firstRelatedEntity, $firstEntity);
+        $this->assertSame($secondRelatedEntity, $secondEntity);
+        $this->assertSame($firstEntity->secondEntity, $secondEntity);
+    }
+}

tests/Doctrine/Tests/ORM/Utility/IdentifierFlattenerTest.php

@@ -0,0 +1,131 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Utility;
+
+use Doctrine\Tests\OrmFunctionalTestCase;
+use Doctrine\Tests\Models\VersionedOneToOne\FirstRelatedEntity;
+use Doctrine\Tests\Models\VersionedOneToOne\SecondRelatedEntity;
+use Doctrine\Tests\Models\Cache\City;
+use Doctrine\Tests\Models\Cache\Flight;
+use Doctrine\ORM\ORMException;
+use Doctrine\ORM\Utility\IdentifierFlattener;
+/**
+ * Test the IdentifierFlattener utility class
+ *
+ * @author Rob Caiger <rob@clocal.co.uk>
+ * @covers \Doctrine\ORM\Utility\IdentifierFlattener
+ */
+class IdentifierFlattenerTest extends OrmFunctionalTestCase
+{
+    /**
+     * Identifier flattener
+     *
+     * @var \Doctrine\ORM\Utility\IdentifierFlattener
+     */
+    private $identifierFlattener;
+
+    protected function setUp()
+    {
+        parent::setUp();
+
+        $this->identifierFlattener = new IdentifierFlattener(
+            $this->_em->getUnitOfWork(),
+            $this->_em->getMetadataFactory()
+        );
+
+        try {
+            $this->_schemaTool->createSchema(
+                array(
+                    $this->_em->getClassMetadata('Doctrine\Tests\Models\VersionedOneToOne\FirstRelatedEntity'),
+                    $this->_em->getClassMetadata('Doctrine\Tests\Models\VersionedOneToOne\SecondRelatedEntity'),
+                    $this->_em->getClassMetadata('Doctrine\Tests\Models\Cache\Flight'),
+                    $this->_em->getClassMetadata('Doctrine\Tests\Models\Cache\City')
+                )
+            );
+        } catch (ORMException $e) {
+        }
+    }
+
+    /**
+     * @group utilities
+     */
+    public function testFlattenIdentifierWithOneToOneId()
+    {
+        $secondRelatedEntity = new SecondRelatedEntity();
+        $secondRelatedEntity->name = 'Bob';
+
+        $this->_em->persist($secondRelatedEntity);
+        $this->_em->flush();
+
+        $firstRelatedEntity = new FirstRelatedEntity();
+        $firstRelatedEntity->name = 'Fred';
+        $firstRelatedEntity->secondEntity = $secondRelatedEntity;
+
+        $this->_em->persist($firstRelatedEntity);
+        $this->_em->flush();
+
+        $firstEntity = $this->_em->getRepository('Doctrine\Tests\Models\VersionedOneToOne\FirstRelatedEntity')
+            ->findOneBy(array('name' => 'Fred'));
+
+        $class = $this->_em->getClassMetadata('Doctrine\Tests\Models\VersionedOneToOne\FirstRelatedEntity');
+
+        $id = $class->getIdentifierValues($firstEntity);
+
+        $this->assertCount(1, $id, 'We should have 1 identifier');
+
+        $this->assertArrayHasKey('secondEntity', $id, 'It should be called secondEntity');
+
+        $this->assertInstanceOf(
+            '\Doctrine\Tests\Models\VersionedOneToOne\SecondRelatedEntity',
+            $id['secondEntity'],
+            'The entity should be an instance of SecondRelatedEntity'
+        );
+
+        $flatIds = $this->identifierFlattener->flattenIdentifier($class, $id);
+
+        $this->assertCount(1, $flatIds, 'We should have 1 flattened id');
+
+        $this->assertArrayHasKey('secondEntity', $flatIds, 'It should be called secondEntity');
+
+        $this->assertSame($id['secondEntity']->id, $flatIds['secondEntity']);
+    }
+
+    /**
+     * @group utilities
+     */
+    public function testFlattenIdentifierWithMutlipleIds()
+    {
+        $leeds = new City('Leeds');
+        $london = new City('London');
+
+        $this->_em->persist($leeds);
+        $this->_em->persist($london);
+        $this->_em->flush();
+
+        $flight = new Flight($leeds, $london);
+
+        $this->_em->persist($flight);
+        $this->_em->flush();
+
+        $class = $this->_em->getClassMetadata('Doctrine\Tests\Models\Cache\Flight');
+        $id = $class->getIdentifierValues($flight);
+
+        $this->assertCount(2, $id);
+
+        $this->assertArrayHasKey('leavingFrom', $id);
+        $this->assertArrayHasKey('goingTo', $id);
+
+        $this->assertSame($leeds, $id['leavingFrom']);
+        $this->assertSame($london, $id['goingTo']);
+
+        $flatIds = $this->identifierFlattener->flattenIdentifier($class, $id);
+
+        $this->assertCount(2, $flatIds);
+
+        $this->assertArrayHasKey('leavingFrom', $flatIds);
+        $this->assertArrayHasKey('goingTo', $flatIds);
+
+        $this->assertSame($id['leavingFrom']->getId(), $flatIds['leavingFrom']);
+        $this->assertSame($id['goingTo']->getId(), $flatIds['goingTo']);
+    }
+}