From dd398ce577ce54d5d4cb37e86e8ebf8bff869b0f Mon Sep 17 00:00:00 2001 From: Rob Caiger Date: Thu, 7 Aug 2014 14:56:27 +0100 Subject: [PATCH] - Fixed the basic entity persister so that versioned OneToOne entities can be created - Created an IdentifierFlattener utility class - Moved the logic for the flatten identifier method into the new utility class - Replaced calls for private flattenIdentifier to use new utility - Added appropriate unit tests --- .../ORM/Persisters/BasicEntityPersister.php | 24 +++- lib/Doctrine/ORM/UnitOfWork.php | 51 +++---- .../ORM/Utility/IdentifierFlattener.php | 89 ++++++++++++ .../VersionedOneToOne/FirstRelatedEntity.php | 32 +++++ .../VersionedOneToOne/SecondRelatedEntity.php | 32 +++++ .../ORM/Functional/Ticket/DDC117Test.php | 2 +- .../ORM/Functional/VersionedOneToOneTest.php | 62 +++++++++ .../ORM/Utility/IdentifierFlattenerTest.php | 131 ++++++++++++++++++ 8 files changed, 383 insertions(+), 40 deletions(-) create mode 100644 lib/Doctrine/ORM/Utility/IdentifierFlattener.php create mode 100644 tests/Doctrine/Tests/Models/VersionedOneToOne/FirstRelatedEntity.php create mode 100644 tests/Doctrine/Tests/Models/VersionedOneToOne/SecondRelatedEntity.php create mode 100644 tests/Doctrine/Tests/ORM/Functional/VersionedOneToOneTest.php create mode 100644 tests/Doctrine/Tests/ORM/Utility/IdentifierFlattenerTest.php diff --git a/lib/Doctrine/ORM/Persisters/BasicEntityPersister.php b/lib/Doctrine/ORM/Persisters/BasicEntityPersister.php index 4be7ee78e..7fd070f82 100644 --- a/lib/Doctrine/ORM/Persisters/BasicEntityPersister.php +++ b/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 * @author Alexander * @author Fabio B. Silva + * @author Rob Caiger * @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 BasicEntityPersister 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->class = $class; - $this->conn = $em->getConnection(); - $this->platform = $this->conn->getDatabasePlatform(); - $this->quoteStrategy = $em->getConfiguration()->getQuoteStrategy(); + $this->em = $em; + $this->class = $class; + $this->conn = $em->getConnection(); + $this->platform = $this->conn->getDatabasePlatform(); + $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); } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 61a3c7ecc..699926f43 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/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 * @author Jonathan Wage * @author Roman Borschel + * @author Rob Caiger * @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->evm = $em->getEventManager(); - $this->listenersInvoker = new ListenersInvoker($em); - $this->hasCache = $em->getConfiguration()->isSecondLevelCacheEnabled(); + $this->em = $em; + $this->evm = $em->getEventManager(); + $this->listenersInvoker = new ListenersInvoker($em); + $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); } diff --git a/lib/Doctrine/ORM/Utility/IdentifierFlattener.php b/lib/Doctrine/ORM/Utility/IdentifierFlattener.php new file mode 100644 index 000000000..bb84c6151 --- /dev/null +++ b/lib/Doctrine/ORM/Utility/IdentifierFlattener.php @@ -0,0 +1,89 @@ +. + */ + + +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 + */ +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; + } +} diff --git a/tests/Doctrine/Tests/Models/VersionedOneToOne/FirstRelatedEntity.php b/tests/Doctrine/Tests/Models/VersionedOneToOne/FirstRelatedEntity.php new file mode 100644 index 000000000..19b0d2f88 --- /dev/null +++ b/tests/Doctrine/Tests/Models/VersionedOneToOne/FirstRelatedEntity.php @@ -0,0 +1,32 @@ + + * + * @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; +} diff --git a/tests/Doctrine/Tests/Models/VersionedOneToOne/SecondRelatedEntity.php b/tests/Doctrine/Tests/Models/VersionedOneToOne/SecondRelatedEntity.php new file mode 100644 index 000000000..cb5dcf66c --- /dev/null +++ b/tests/Doctrine/Tests/Models/VersionedOneToOne/SecondRelatedEntity.php @@ -0,0 +1,32 @@ + + * + * @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; +} diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC117Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC117Test.php index 7bd576a63..9c28b7e88 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC117Test.php +++ b/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); diff --git a/tests/Doctrine/Tests/ORM/Functional/VersionedOneToOneTest.php b/tests/Doctrine/Tests/ORM/Functional/VersionedOneToOneTest.php new file mode 100644 index 000000000..f0a89ba5e --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/VersionedOneToOneTest.php @@ -0,0 +1,62 @@ + + * + * @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); + } +} diff --git a/tests/Doctrine/Tests/ORM/Utility/IdentifierFlattenerTest.php b/tests/Doctrine/Tests/ORM/Utility/IdentifierFlattenerTest.php new file mode 100644 index 000000000..63536c886 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Utility/IdentifierFlattenerTest.php @@ -0,0 +1,131 @@ + + * @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']); + } +}