From 942004226ca1b0f0a6a62fcca78abe2f8c313d2a Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 13:15:43 +0100 Subject: [PATCH 01/20] DDC-2704 - basic test case verifying that merged transient properties are not handled when in an inheritance --- .../Functional/MergeSharedEntitiesTest.php | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php index 3dae34b67..b2a655963 100644 --- a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php @@ -90,6 +90,28 @@ class MergeSharedEntitiesTest extends OrmFunctionalTestCase $this->assertEquals($picture->file, $picture->otherFile, 'Identical entities must remain identical'); } + + /** + * @group DDC-2704 + */ + public function testMergeInheritedTransientProperties() + { + $admin1 = new MSEAdmin(); + $admin2 = new MSEAdmin(); + + $admin1->id = 123; + $admin2->id = 123; + + $this->_em->persist($admin1); + + $admin2->setSession('zeh current session data'); + + + $merged = $this->_em->merge($admin2); + + $this->assertSame($admin1, $merged); + $this->assertSame($admin2->getSession(), $admin1->getSession()); + } } /** @Entity */ @@ -111,3 +133,26 @@ class MSEFile /** @Column(type="integer") @Id @GeneratedValue(strategy="AUTO") */ public $id; } + +/** @MappedSuperclass */ +abstract class MSEUser +{ + private $session; // intentionally transient property + + public function getSession() + { + return $this->session; + } + + public function setSession($session) + { + $this->session = $session; + } +} + +/** @Entity */ +class MSEAdmin extends MSEUser +{ + /** @Column(type="integer") @Id @GeneratedValue(strategy="NONE") */ + public $id; +} From 302e6218bb4641be34b2327e094b080cf5f58ca7 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 13:16:18 +0100 Subject: [PATCH 02/20] DDC-2704 - renaming test case for clarity --- tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php index b2a655963..a0010734a 100644 --- a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php @@ -94,7 +94,7 @@ class MergeSharedEntitiesTest extends OrmFunctionalTestCase /** * @group DDC-2704 */ - public function testMergeInheritedTransientProperties() + public function testMergeInheritedTransientPrivateProperties() { $admin1 = new MSEAdmin(); $admin2 = new MSEAdmin(); From 5ae980e0f961bb6ee30b761e3ce3d7be5a9570d9 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 13:16:41 +0100 Subject: [PATCH 03/20] DDC-2704 - reducing test case clutter --- .../Tests/ORM/Functional/MergeSharedEntitiesTest.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php index a0010734a..d4929a1af 100644 --- a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php @@ -106,10 +106,7 @@ class MergeSharedEntitiesTest extends OrmFunctionalTestCase $admin2->setSession('zeh current session data'); - - $merged = $this->_em->merge($admin2); - - $this->assertSame($admin1, $merged); + $this->assertSame($admin1, $this->_em->merge($admin2)); $this->assertSame($admin2->getSession(), $admin1->getSession()); } } From 0a3d6966d6ff3216bc76948a04956b1f536efd23 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 14:21:51 +0100 Subject: [PATCH 04/20] DDC-2704 - providing hotfix - also storing inherited transient properties in the class metadata --- .../ORM/Mapping/ClassMetadataInfo.php | 76 ++++++++++++++++++- lib/Doctrine/ORM/UnitOfWork.php | 2 +- 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index c3c8d8c83..0968aa631 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -20,6 +20,7 @@ namespace Doctrine\ORM\Mapping; use BadMethodCallException; +use Doctrine\Common\Persistence\Mapping\ReflectionService; use Doctrine\Instantiator\Instantiator; use InvalidArgumentException; use RuntimeException; @@ -649,6 +650,12 @@ class ClassMetadataInfo implements ClassMetadata */ private $instantiator; + /** + * @var \ReflectionProperty[]|null (null if not yet initialized) - all instance properties of the class, + * transient or not, in accessible form. + */ + private $reflectionProperties; + /** * Initializes a new ClassMetadata instance that will hold the object-relational mapping * metadata of the class with the given name. @@ -667,13 +674,30 @@ class ClassMetadataInfo implements ClassMetadata /** * Gets the ReflectionProperties of the mapped class. * - * @return array An array of ReflectionProperty instances. + * @return \ReflectionProperty[] An array of ReflectionProperty instances. */ public function getReflectionProperties() { return $this->reflFields; } + /** + * Retrieves all ReflectionProperties of this class, considering inherited and transient ones + * + * Note that this method should only be used after `wakeupReflection` + */ + public function getAllReflectionProperties() + { + if (null === $this->reflectionProperties) { + throw new \RuntimeException(sprintf( + 'You cannot read the reflection properties before calling %s#wakeupReflection() first', + get_class($this) + )); + } + + return $this->reflectionProperties; + } + /** * Gets a ReflectionProperty for a specific field of the mapped class. * @@ -963,6 +987,8 @@ class ClassMetadataInfo implements ClassMetadata ? $reflService->getAccessibleProperty($mapping['declared'], $field) : $reflService->getAccessibleProperty($this->name, $field); } + + $this->initializeAllReflectionProperties($reflService); } /** @@ -3313,4 +3339,52 @@ class ClassMetadataInfo implements ClassMetadata return $sequencePrefix; } + + /** + * Initializes the internal `reflectionProperties` property + * + * @param ReflectionService $reflectionService + */ + private function initializeAllReflectionProperties(ReflectionService $reflectionService) + { + $class = $this->reflClass; + $properties = array(); + + do { + $className = $class->getName(); + + foreach ($class->getProperties() as $property) { + // static properties are not instance properties + if ($property->isStatic()) { + continue; + } + + // indexing by logical name to avoid duplicates + $logicalName = $property->getDeclaringClass()->getName() . $property->getName(); + $propertyName = $property->getName(); + $existingField = isset($this->reflFields[$propertyName]) ? $this->reflFields[$propertyName] : null; + + if (! $existingField) { + $properties[$logicalName] = $reflectionService->getAccessibleProperty($className, $propertyName); + + continue; + } + + // private properties are not inherited: need to handle them separately and precisely + if ($property->isPrivate() + && $existingField->getDeclaringClass()->getName() !== $property->getDeclaringClass()->getName() + ) { + $properties[$logicalName] = $reflectionService->getAccessibleProperty($className, $propertyName); + + continue; + } + + $properties[$logicalName] = $existingField; + } + + $parentClass = $class->getParentClass(); + } while ($parentClass && $class = $reflectionService->getClass($parentClass->getName())); + + $this->reflectionProperties = array_values($properties); + } } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index cd7c718d6..6e1d5428b 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -3348,7 +3348,7 @@ class UnitOfWork implements PropertyChangedListener { $class = $this->em->getClassMetadata(get_class($entity)); - foreach ($class->reflClass->getProperties() as $prop) { + foreach ($class->getAllReflectionProperties() as $prop) { $name = $prop->name; $prop->setAccessible(true); From 4e08c99b868be8d4e09e97af0a896b801e98b5ab Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 14:54:51 +0100 Subject: [PATCH 05/20] DDC-2704 - trying to get all reflection properties from an unitialized class metadata instance will result in a failure --- .../Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php index 0ffb41c18..5de685733 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php @@ -1125,6 +1125,18 @@ class ClassMetadataTest extends \Doctrine\Tests\OrmTestCase $this->assertInstanceOf(__NAMESPACE__ . '\\MyArrayObjectEntity', $classMetadata->newInstance()); } + + /** + * @group DDC-2704 + */ + public function testGetAllReflectionPropertiesFailsOnNonInitializedMetadata() + { + $classMetadata = new ClassMetadata(__NAMESPACE__ . '\\MyArrayObjectEntity'); + + $this->setExpectedException('RuntimeException'); + + $classMetadata->getAllReflectionProperties(); + } } /** From 1e6c071bb8450947046bcfbeaecc206f81a9fcad Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 14:55:42 +0100 Subject: [PATCH 06/20] DDC-2704 - trying to get all reflection properties from a partially initialized class metadata instance will result in a failure --- .../Tests/ORM/Mapping/ClassMetadataTest.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php index 5de685733..36b92f81d 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php @@ -3,6 +3,7 @@ namespace Doctrine\Tests\ORM\Mapping; use Doctrine\Common\Persistence\Mapping\RuntimeReflectionService; +use Doctrine\Common\Persistence\Mapping\StaticReflectionService; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Events; use Doctrine\ORM\Mapping\DefaultNamingStrategy; @@ -1137,6 +1138,20 @@ class ClassMetadataTest extends \Doctrine\Tests\OrmTestCase $classMetadata->getAllReflectionProperties(); } + + /** + * @group DDC-2704 + */ + public function testGetAllReflectionPropertiesFailsOnPartiallyInitializedMetadata() + { + $classMetadata = new ClassMetadata(__NAMESPACE__ . '\\MyArrayObjectEntity'); + + $classMetadata->initializeReflection(new StaticReflectionService()); + + $this->setExpectedException('RuntimeException'); + + $classMetadata->getAllReflectionProperties(); + } } /** From a4982a8dc25967e07f86f2d5b8885f3df709118b Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 15:03:22 +0100 Subject: [PATCH 07/20] DDC-2704 - handling partial initialization of the class as expected (class metadata may not hold reflection class after wakeup) --- lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 0968aa631..2c1befb5c 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -3347,20 +3347,24 @@ class ClassMetadataInfo implements ClassMetadata */ private function initializeAllReflectionProperties(ReflectionService $reflectionService) { - $class = $this->reflClass; - $properties = array(); + if (! $this->reflClass) { + return; + } + + $currentClass = $this->reflClass; + $properties = array(); do { - $className = $class->getName(); + $className = $currentClass->getName(); - foreach ($class->getProperties() as $property) { + foreach ($currentClass->getProperties() as $property) { // static properties are not instance properties if ($property->isStatic()) { continue; } // indexing by logical name to avoid duplicates - $logicalName = $property->getDeclaringClass()->getName() . $property->getName(); + $logicalName = $property->getDeclaringClass()->getName() . '::' . $property->getName(); $propertyName = $property->getName(); $existingField = isset($this->reflFields[$propertyName]) ? $this->reflFields[$propertyName] : null; @@ -3382,8 +3386,8 @@ class ClassMetadataInfo implements ClassMetadata $properties[$logicalName] = $existingField; } - $parentClass = $class->getParentClass(); - } while ($parentClass && $class = $reflectionService->getClass($parentClass->getName())); + $parentClass = $currentClass->getParentClass(); + } while ($parentClass && $currentClass = $reflectionService->getClass($parentClass->getName())); $this->reflectionProperties = array_values($properties); } From bd667b82d9160a97964ddc175261e593745c9960 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 15:24:48 +0100 Subject: [PATCH 08/20] DDC-2704 - carefully checking defined classes in `getAllReflectionProperties` --- .../Tests/ORM/Mapping/ClassMetadataTest.php | 37 +++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php index 36b92f81d..4a7882862 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php @@ -7,6 +7,8 @@ use Doctrine\Common\Persistence\Mapping\StaticReflectionService; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Events; use Doctrine\ORM\Mapping\DefaultNamingStrategy; +use Doctrine\Tests\Models\DirectoryTree\AbstractContentItem; +use Doctrine\Tests\Models\DirectoryTree\File; require_once __DIR__ . '/../../Models/Global/GlobalNamespaceModel.php'; @@ -1152,6 +1154,41 @@ class ClassMetadataTest extends \Doctrine\Tests\OrmTestCase $classMetadata->getAllReflectionProperties(); } + + /** + * @group DDC-2704 + */ + public function testGetAllReflectionPropertiesRetrievesCollidingPrivateProperties() + { + $classMetadata = new ClassMetadata(File::CLASSNAME); + + $classMetadata->initializeReflection(new RuntimeReflectionService()); + + $properties = $classMetadata->getAllReflectionProperties(); + + $this->assertInternalType('array', $properties); + $this->assertCount(5, $properties); + + $propertyNames = array_map( + function (\ReflectionProperty $property) { + return $property->getDeclaringClass()->getName() . '::' . $property->getName(); + }, + $properties + ); + + sort($propertyNames); + + $this->assertEquals( + [ + 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::id', + 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::name', + 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::nodeIsLoaded', + 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::parentDirectory', + 'Doctrine\Tests\Models\DirectoryTree\File::extension', + ], + $propertyNames + ); + } } /** From 885700d38cfcb436e00a529c455ba1dcaa774b31 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 15:25:10 +0100 Subject: [PATCH 09/20] DDC-2704 - should `initializeAllReflectionProperties` also on `initializeReflection` --- lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 2c1befb5c..6d96576c0 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -1009,6 +1009,8 @@ class ClassMetadataInfo implements ClassMetadata } $this->table['name'] = $this->namingStrategy->classToTableName($this->name); + + $this->initializeAllReflectionProperties($reflService); } /** From 3df9b4d122a58fd1ae05430b2ef9d82c48632647 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 15:25:35 +0100 Subject: [PATCH 10/20] DDC-2704 - synchronized model classes to new test code --- .../DirectoryTree/AbstractContentItem.php | 25 +++++++++++++++++++ .../Tests/Models/DirectoryTree/File.php | 2 ++ 2 files changed, 27 insertions(+) diff --git a/tests/Doctrine/Tests/Models/DirectoryTree/AbstractContentItem.php b/tests/Doctrine/Tests/Models/DirectoryTree/AbstractContentItem.php index 12b27eed3..d48b6e19c 100644 --- a/tests/Doctrine/Tests/Models/DirectoryTree/AbstractContentItem.php +++ b/tests/Doctrine/Tests/Models/DirectoryTree/AbstractContentItem.php @@ -24,6 +24,8 @@ namespace Doctrine\Tests\Models\DirectoryTree; */ abstract class AbstractContentItem { + const CLASSNAME = __CLASS__; + /** * @Id @Column(type="integer") @GeneratedValue */ @@ -37,6 +39,13 @@ abstract class AbstractContentItem /** @column(type="string") */ protected $name; + /** + * This field is transient and private on purpose + * + * @var bool + */ + private $nodeIsLoaded = false; + public function __construct(Directory $parentDir = null) { $this->parentDirectory = $parentDir; @@ -61,4 +70,20 @@ abstract class AbstractContentItem { return $this->parentDirectory; } + + /** + * @return bool + */ + public function getNodeIsLoaded() + { + return $this->nodeIsLoaded; + } + + /** + * @param bool $nodeIsLoaded + */ + public function setNodeIsLoaded($nodeIsLoaded) + { + $this->nodeIsLoaded = (bool) $nodeIsLoaded; + } } diff --git a/tests/Doctrine/Tests/Models/DirectoryTree/File.php b/tests/Doctrine/Tests/Models/DirectoryTree/File.php index 35132689a..0b8ed0c80 100644 --- a/tests/Doctrine/Tests/Models/DirectoryTree/File.php +++ b/tests/Doctrine/Tests/Models/DirectoryTree/File.php @@ -26,6 +26,8 @@ namespace Doctrine\Tests\Models\DirectoryTree; */ class File extends AbstractContentItem { + const CLASSNAME = __CLASS__; + /** @Column(type="string") */ protected $extension = "html"; From 30dcece1257d6a722d9080c7fdb116dc2954d737 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 15:30:47 +0100 Subject: [PATCH 11/20] DDC-2704 - covering code handling class metadata skipping of static properties --- .../Tests/Models/DirectoryTree/AbstractContentItem.php | 7 +++++++ tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php | 6 ++++++ 2 files changed, 13 insertions(+) diff --git a/tests/Doctrine/Tests/Models/DirectoryTree/AbstractContentItem.php b/tests/Doctrine/Tests/Models/DirectoryTree/AbstractContentItem.php index d48b6e19c..d2ba439a0 100644 --- a/tests/Doctrine/Tests/Models/DirectoryTree/AbstractContentItem.php +++ b/tests/Doctrine/Tests/Models/DirectoryTree/AbstractContentItem.php @@ -46,6 +46,13 @@ abstract class AbstractContentItem */ private $nodeIsLoaded = false; + /** + * This field is transient on purpose + * + * @var mixed + */ + public static $fileSystem; + public function __construct(Directory $parentDir = null) { $this->parentDirectory = $parentDir; diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php index 4a7882862..a6aafce96 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php @@ -1188,6 +1188,12 @@ class ClassMetadataTest extends \Doctrine\Tests\OrmTestCase ], $propertyNames ); + + $this->assertNotContains( + 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::fileSystem', + $propertyNames, + 'Abstract properties should not be part of class metadata information' + ); } } From 21995a8b10746d35d95e70296be01d0ec7caa9a4 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 15:40:36 +0100 Subject: [PATCH 12/20] DDC-2704 - more explicit value checking --- tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php index d4929a1af..28e89c360 100644 --- a/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/MergeSharedEntitiesTest.php @@ -107,7 +107,7 @@ class MergeSharedEntitiesTest extends OrmFunctionalTestCase $admin2->setSession('zeh current session data'); $this->assertSame($admin1, $this->_em->merge($admin2)); - $this->assertSame($admin2->getSession(), $admin1->getSession()); + $this->assertSame('zeh current session data', $admin1->getSession()); } } From 91f4ed8b921fcc5fd5bf94a0fe470364c9548411 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 15:48:00 +0100 Subject: [PATCH 13/20] DDC-2704 - data should be merged only into initialized proxies --- .../Tests/ORM/Functional/MergeProxiesTest.php | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/MergeProxiesTest.php b/tests/Doctrine/Tests/ORM/Functional/MergeProxiesTest.php index daed36652..3b334d5d5 100644 --- a/tests/Doctrine/Tests/ORM/Functional/MergeProxiesTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/MergeProxiesTest.php @@ -83,6 +83,35 @@ class MergeProxiesTest extends OrmFunctionalTestCase $this->assertFalse($managed->__isInitialized()); } + /** + * @group DDC-1392 + * @group DDC-1734 + * @group DDC-3368 + * @group #1172 + * + * Bug discovered while working on DDC-2704 - merging towards un-initialized proxies does not initialize them, + * causing merged data to be lost when they are actually initialized + */ + public function testMergeWithExistingUninitializedManagedProxy() + { + $date = new DateTimeModel(); + + $this->_em->persist($date); + $this->_em->flush($date); + $this->_em->clear(); + + $managed = $this->_em->getReference(DateTimeModel::CLASSNAME, $date->id); + + $this->assertInstanceOf('Doctrine\Common\Proxy\Proxy', $managed); + $this->assertFalse($managed->__isInitialized()); + + $date->date = $dateTime = new \DateTime(); + + $this->assertSame($managed, $this->_em->merge($date)); + $this->assertTrue($managed->__isInitialized()); + $this->assertSame($dateTime, $managed->date, 'Data was merged into the proxy after initialization'); + } + /** * @group DDC-1392 * @group DDC-1734 From 8910c2c482e8780eda18806790c6453e1a4b27db Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 20 Jan 2015 15:48:58 +0100 Subject: [PATCH 14/20] DDC-2704 - data should be merged only into initialized proxies --- lib/Doctrine/ORM/UnitOfWork.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 6e1d5428b..a09bf1cc1 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -1868,6 +1868,10 @@ class UnitOfWork implements PropertyChangedListener $visited[$oid] = $managedCopy; // mark visited if (!($entity instanceof Proxy && ! $entity->__isInitialized())) { + if ($managedCopy instanceof Proxy && ! $managedCopy->__isInitialized()) { + $managedCopy->__load(); + } + $this->mergeEntityStateIntoManagedCopy($entity, $managedCopy); } From 1b0a5e38d93970619f6a38ade8fb2550cf790b80 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 24 Jan 2015 14:28:54 +0100 Subject: [PATCH 15/20] #1272 DDC-2704 - specification for a property getter utility --- .../ReflectionPropertiesGetterTest.php | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 tests/Doctrine/Tests/ORM/Mapping/Reflection/ReflectionPropertiesGetterTest.php diff --git a/tests/Doctrine/Tests/ORM/Mapping/Reflection/ReflectionPropertiesGetterTest.php b/tests/Doctrine/Tests/ORM/Mapping/Reflection/ReflectionPropertiesGetterTest.php new file mode 100644 index 000000000..7bc83df2e --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Mapping/Reflection/ReflectionPropertiesGetterTest.php @@ -0,0 +1,129 @@ +getProperties(ClassWithMixedProperties::CLASSNAME); + + $this->assertCount(5, $properties); + + foreach ($properties as $property) { + $this->assertInstanceOf('ReflectionProperty', $property); + } + } + + public function testRetrievedInstancesAreNotStatic() + { + $properties = (new ReflectionPropertiesGetter(new RuntimeReflectionService())) + ->getProperties(ClassWithMixedProperties::CLASSNAME); + + foreach ($properties as $property) { + $this->assertFalse($property->isStatic()); + } + } + + public function testExpectedKeys() + { + $properties = (new ReflectionPropertiesGetter(new RuntimeReflectionService())) + ->getProperties(ClassWithMixedProperties::CLASSNAME); + + $this->assertArrayHasKey( + "\0" . ClassWithMixedProperties::CLASSNAME . "\0" . 'privateProperty', + $properties + ); + $this->assertArrayHasKey( + "\0" . ClassWithMixedProperties::CLASSNAME . "\0" . 'privatePropertyOverride', + $properties + ); + $this->assertArrayHasKey( + "\0" . ParentClass::CLASSNAME . "\0" . 'privatePropertyOverride', + $properties + ); + $this->assertArrayHasKey( + "\0*\0protectedProperty", + $properties + ); + $this->assertArrayHasKey( + "publicProperty", + $properties + ); + } + + public function testPropertiesAreAccessible() + { + $object = new ClassWithMixedProperties(); + $properties = (new ReflectionPropertiesGetter(new RuntimeReflectionService())) + ->getProperties(ClassWithMixedProperties::CLASSNAME); + + foreach ($properties as $property) { + $this->assertSame($property->getName(), $property->getValue($object)); + } + } + + public function testPropertyGetterIsIdempotent() + { + $getter = (new ReflectionPropertiesGetter(new RuntimeReflectionService())); + + $this->assertSame( + $getter->getProperties(ClassWithMixedProperties::CLASSNAME), + $getter->getProperties(ClassWithMixedProperties::CLASSNAME) + ); + } + + public function testPropertyGetterWillSkipPropertiesNotRetrievedByTheRuntimeReflectionService() + { + /* @var $reflectionService ReflectionService|\PHPUnit_Framework_MockObject_MockObject */ + $reflectionService = $this->getMock('Doctrine\Common\Persistence\Mapping\ReflectionService'); + + $reflectionService + ->expects($this->exactly(2)) + ->method('getClass') + ->with($this->logicalOr(ClassWithMixedProperties::CLASSNAME, ParentClass::CLASSNAME)) + ->will($this->returnValueMap([ + [ClassWithMixedProperties::CLASSNAME, new ReflectionClass(ClassWithMixedProperties::CLASSNAME)], + [ParentClass::CLASSNAME, new ReflectionClass(ParentClass::CLASSNAME)], + ])); + + $reflectionService + ->expects($this->atLeastOnce()) + ->method('getAccessibleProperty'); + + $getter = (new ReflectionPropertiesGetter($reflectionService)); + + $this->assertEmpty($getter->getProperties(ClassWithMixedProperties::CLASSNAME)); + } + + public function testPropertyGetterWillSkipClassesNotRetrievedByTheRuntimeReflectionService() + { + /* @var $reflectionService ReflectionService|\PHPUnit_Framework_MockObject_MockObject */ + $reflectionService = $this->getMock('Doctrine\Common\Persistence\Mapping\ReflectionService'); + + $reflectionService + ->expects($this->once()) + ->method('getClass') + ->with(ClassWithMixedProperties::CLASSNAME); + + $reflectionService->expects($this->never())->method('getAccessibleProperty'); + + $getter = (new ReflectionPropertiesGetter($reflectionService)); + + $this->assertEmpty($getter->getProperties(ClassWithMixedProperties::CLASSNAME)); + } +} From 5ec300452a79ef81f992a2471fac214ef5136965 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 24 Jan 2015 14:29:09 +0100 Subject: [PATCH 16/20] #1272 DDC-2704 - implementation for a property getter utility --- .../Reflection/ReflectionPropertiesGetter.php | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php diff --git a/lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php b/lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php new file mode 100644 index 000000000..5da1e9a54 --- /dev/null +++ b/lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php @@ -0,0 +1,161 @@ +. + */ + +namespace Doctrine\ORM\Mapping\Reflection; + +use Doctrine\Common\Persistence\Mapping\ReflectionService; +use ReflectionClass; +use ReflectionProperty; + +/** + * Utility class to retrieve all reflection instance properties of a given class, including + * private inherited properties and transient properties. + * + * @author Marco Pivetta + */ +final class ReflectionPropertiesGetter +{ + /** + * @var ReflectionProperty[][] indexed by class name and property internal name + */ + private $properties = []; + + /** + * @var ReflectionService + */ + private $reflectionService; + + /** + * @param ReflectionService $reflectionService + */ + public function __construct(ReflectionService $reflectionService) + { + $this->reflectionService = $reflectionService; + } + + /** + * @param $className + * + * @return ReflectionProperty[] indexed by property internal name + */ + public function getProperties($className) + { + if (isset($this->properties[$className])) { + return $this->properties[$className]; + } + + return $this->properties[$className] = call_user_func_array( + 'array_merge', + // first merge because `array_merge` expects >= 1 params + array_merge( + [[]], + array_map( + [$this, 'getClassProperties'], + $this->getHierarchyClasses($className) + ) + ) + ); + } + + /** + * @param string $className + * + * @return ReflectionClass[] + */ + private function getHierarchyClasses($className) + { + $classes = []; + $parentClassName = $className; + + while ($parentClassName && $currentClass = $this->reflectionService->getClass($parentClassName)) { + $classes[] = $currentClass; + $parentClassName = null; + + if ($parentClass = $currentClass->getParentClass()) { + $parentClassName = $parentClass->getName(); + } + } + + return $classes; + } + + /** + * @param ReflectionClass $reflectionClass + * + * @return ReflectionProperty[] + */ + private function getClassProperties(ReflectionClass $reflectionClass) + { + $properties = $reflectionClass->getProperties(); + + return array_filter( + array_filter(array_map( + [$this, 'getAccessibleProperty'], + array_combine( + array_map([$this, 'getLogicalName'], $properties), + $properties + ) + )), + [$this, 'isInstanceProperty'] + ); + } + + /** + * @param ReflectionProperty $reflectionProperty + * + * @return bool + */ + private function isInstanceProperty(ReflectionProperty $reflectionProperty) + { + return ! $reflectionProperty->isStatic(); + } + + /** + * @param ReflectionProperty $property + * + * @return null|ReflectionProperty + */ + private function getAccessibleProperty(ReflectionProperty $property) + { + return $this->reflectionService->getAccessibleProperty( + $property->getDeclaringClass()->getName(), + $property->getName() + ); + } + + /** + * @param ReflectionProperty $property + * + * @return string + */ + private function getLogicalName(ReflectionProperty $property) + { + $propertyName = $property->getName(); + + if ($property->isPublic()) { + return $propertyName; + } + + if ($property->isProtected()) { + return "\0*\0" . $propertyName; + } + + return "\0" . $property->getDeclaringClass()->getName() . "\0" . $propertyName; + } +} From 1aa453d493ca84989fc9cf736e349023de49da18 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 24 Jan 2015 14:29:40 +0100 Subject: [PATCH 17/20] #1272 DDC-2704 - property getter utility is package private --- .../ORM/Mapping/Reflection/ReflectionPropertiesGetter.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php b/lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php index 5da1e9a54..6ac7f11c3 100644 --- a/lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php +++ b/lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php @@ -27,6 +27,8 @@ use ReflectionProperty; * Utility class to retrieve all reflection instance properties of a given class, including * private inherited properties and transient properties. * + * @private This API is for internal use only + * * @author Marco Pivetta */ final class ReflectionPropertiesGetter From 05a8e1c77d8d24a89b5ad55b21637fe363c37775 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 24 Jan 2015 14:30:07 +0100 Subject: [PATCH 18/20] #1272 DDC-2704 - test assets for the property getter utility tests --- .../Reflection/ClassWithMixedProperties.php | 18 ++++++++++++++++++ .../Tests/Models/Reflection/ParentClass.php | 10 ++++++++++ 2 files changed, 28 insertions(+) create mode 100644 tests/Doctrine/Tests/Models/Reflection/ClassWithMixedProperties.php create mode 100644 tests/Doctrine/Tests/Models/Reflection/ParentClass.php diff --git a/tests/Doctrine/Tests/Models/Reflection/ClassWithMixedProperties.php b/tests/Doctrine/Tests/Models/Reflection/ClassWithMixedProperties.php new file mode 100644 index 000000000..72899959a --- /dev/null +++ b/tests/Doctrine/Tests/Models/Reflection/ClassWithMixedProperties.php @@ -0,0 +1,18 @@ + Date: Sat, 24 Jan 2015 14:30:40 +0100 Subject: [PATCH 19/20] #1272 DDC-2704 - using the property getter utility rather than metadata API when fetching reflection properties for a class --- lib/Doctrine/ORM/UnitOfWork.php | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index a09bf1cc1..8f82aa96f 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -19,8 +19,10 @@ namespace Doctrine\ORM; +use Doctrine\Common\Persistence\Mapping\RuntimeReflectionService; use Doctrine\DBAL\LockMode; use Doctrine\ORM\Internal\HydrationCompleteHandler; +use Doctrine\ORM\Mapping\Reflection\ReflectionPropertiesGetter; use Exception; use InvalidArgumentException; use UnexpectedValueException; @@ -283,6 +285,11 @@ class UnitOfWork implements PropertyChangedListener */ private $hydrationCompleteHandler; + /** + * @var ReflectionPropertiesGetter + */ + private $reflectionPropertiesGetter; + /** * Initializes a new UnitOfWork instance, bound to the given EntityManager. * @@ -290,12 +297,13 @@ class UnitOfWork implements PropertyChangedListener */ public function __construct(EntityManagerInterface $em) { - $this->em = $em; - $this->evm = $em->getEventManager(); - $this->listenersInvoker = new ListenersInvoker($em); - $this->hasCache = $em->getConfiguration()->isSecondLevelCacheEnabled(); - $this->identifierFlattener = new IdentifierFlattener($this, $em->getMetadataFactory()); - $this->hydrationCompleteHandler = new HydrationCompleteHandler($this->listenersInvoker, $em); + $this->em = $em; + $this->evm = $em->getEventManager(); + $this->listenersInvoker = new ListenersInvoker($em); + $this->hasCache = $em->getConfiguration()->isSecondLevelCacheEnabled(); + $this->identifierFlattener = new IdentifierFlattener($this, $em->getMetadataFactory()); + $this->hydrationCompleteHandler = new HydrationCompleteHandler($this->listenersInvoker, $em); + $this->reflectionPropertiesGetter = new ReflectionPropertiesGetter(new RuntimeReflectionService()); } /** @@ -3352,7 +3360,7 @@ class UnitOfWork implements PropertyChangedListener { $class = $this->em->getClassMetadata(get_class($entity)); - foreach ($class->getAllReflectionProperties() as $prop) { + foreach ($this->reflectionPropertiesGetter->getProperties($class->name) as $prop) { $name = $prop->name; $prop->setAccessible(true); From 28e0da43213b39d9e169fd7c3206f0ebbfe4895d Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Sat, 24 Jan 2015 14:32:23 +0100 Subject: [PATCH 20/20] #1272 DDC-2704 - reverting classmetadata API changes (moved all to reflection property getter API) --- .../ORM/Mapping/ClassMetadataInfo.php | 82 +------------------ .../Tests/ORM/Mapping/ClassMetadataTest.php | 70 ---------------- 2 files changed, 1 insertion(+), 151 deletions(-) diff --git a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php index 6d96576c0..c3c8d8c83 100644 --- a/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php +++ b/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php @@ -20,7 +20,6 @@ namespace Doctrine\ORM\Mapping; use BadMethodCallException; -use Doctrine\Common\Persistence\Mapping\ReflectionService; use Doctrine\Instantiator\Instantiator; use InvalidArgumentException; use RuntimeException; @@ -650,12 +649,6 @@ class ClassMetadataInfo implements ClassMetadata */ private $instantiator; - /** - * @var \ReflectionProperty[]|null (null if not yet initialized) - all instance properties of the class, - * transient or not, in accessible form. - */ - private $reflectionProperties; - /** * Initializes a new ClassMetadata instance that will hold the object-relational mapping * metadata of the class with the given name. @@ -674,30 +667,13 @@ class ClassMetadataInfo implements ClassMetadata /** * Gets the ReflectionProperties of the mapped class. * - * @return \ReflectionProperty[] An array of ReflectionProperty instances. + * @return array An array of ReflectionProperty instances. */ public function getReflectionProperties() { return $this->reflFields; } - /** - * Retrieves all ReflectionProperties of this class, considering inherited and transient ones - * - * Note that this method should only be used after `wakeupReflection` - */ - public function getAllReflectionProperties() - { - if (null === $this->reflectionProperties) { - throw new \RuntimeException(sprintf( - 'You cannot read the reflection properties before calling %s#wakeupReflection() first', - get_class($this) - )); - } - - return $this->reflectionProperties; - } - /** * Gets a ReflectionProperty for a specific field of the mapped class. * @@ -987,8 +963,6 @@ class ClassMetadataInfo implements ClassMetadata ? $reflService->getAccessibleProperty($mapping['declared'], $field) : $reflService->getAccessibleProperty($this->name, $field); } - - $this->initializeAllReflectionProperties($reflService); } /** @@ -1009,8 +983,6 @@ class ClassMetadataInfo implements ClassMetadata } $this->table['name'] = $this->namingStrategy->classToTableName($this->name); - - $this->initializeAllReflectionProperties($reflService); } /** @@ -3341,56 +3313,4 @@ class ClassMetadataInfo implements ClassMetadata return $sequencePrefix; } - - /** - * Initializes the internal `reflectionProperties` property - * - * @param ReflectionService $reflectionService - */ - private function initializeAllReflectionProperties(ReflectionService $reflectionService) - { - if (! $this->reflClass) { - return; - } - - $currentClass = $this->reflClass; - $properties = array(); - - do { - $className = $currentClass->getName(); - - foreach ($currentClass->getProperties() as $property) { - // static properties are not instance properties - if ($property->isStatic()) { - continue; - } - - // indexing by logical name to avoid duplicates - $logicalName = $property->getDeclaringClass()->getName() . '::' . $property->getName(); - $propertyName = $property->getName(); - $existingField = isset($this->reflFields[$propertyName]) ? $this->reflFields[$propertyName] : null; - - if (! $existingField) { - $properties[$logicalName] = $reflectionService->getAccessibleProperty($className, $propertyName); - - continue; - } - - // private properties are not inherited: need to handle them separately and precisely - if ($property->isPrivate() - && $existingField->getDeclaringClass()->getName() !== $property->getDeclaringClass()->getName() - ) { - $properties[$logicalName] = $reflectionService->getAccessibleProperty($className, $propertyName); - - continue; - } - - $properties[$logicalName] = $existingField; - } - - $parentClass = $currentClass->getParentClass(); - } while ($parentClass && $currentClass = $reflectionService->getClass($parentClass->getName())); - - $this->reflectionProperties = array_values($properties); - } } diff --git a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php index a6aafce96..0ffb41c18 100644 --- a/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php +++ b/tests/Doctrine/Tests/ORM/Mapping/ClassMetadataTest.php @@ -3,12 +3,9 @@ namespace Doctrine\Tests\ORM\Mapping; use Doctrine\Common\Persistence\Mapping\RuntimeReflectionService; -use Doctrine\Common\Persistence\Mapping\StaticReflectionService; use Doctrine\ORM\Mapping\ClassMetadata; use Doctrine\ORM\Events; use Doctrine\ORM\Mapping\DefaultNamingStrategy; -use Doctrine\Tests\Models\DirectoryTree\AbstractContentItem; -use Doctrine\Tests\Models\DirectoryTree\File; require_once __DIR__ . '/../../Models/Global/GlobalNamespaceModel.php'; @@ -1128,73 +1125,6 @@ class ClassMetadataTest extends \Doctrine\Tests\OrmTestCase $this->assertInstanceOf(__NAMESPACE__ . '\\MyArrayObjectEntity', $classMetadata->newInstance()); } - - /** - * @group DDC-2704 - */ - public function testGetAllReflectionPropertiesFailsOnNonInitializedMetadata() - { - $classMetadata = new ClassMetadata(__NAMESPACE__ . '\\MyArrayObjectEntity'); - - $this->setExpectedException('RuntimeException'); - - $classMetadata->getAllReflectionProperties(); - } - - /** - * @group DDC-2704 - */ - public function testGetAllReflectionPropertiesFailsOnPartiallyInitializedMetadata() - { - $classMetadata = new ClassMetadata(__NAMESPACE__ . '\\MyArrayObjectEntity'); - - $classMetadata->initializeReflection(new StaticReflectionService()); - - $this->setExpectedException('RuntimeException'); - - $classMetadata->getAllReflectionProperties(); - } - - /** - * @group DDC-2704 - */ - public function testGetAllReflectionPropertiesRetrievesCollidingPrivateProperties() - { - $classMetadata = new ClassMetadata(File::CLASSNAME); - - $classMetadata->initializeReflection(new RuntimeReflectionService()); - - $properties = $classMetadata->getAllReflectionProperties(); - - $this->assertInternalType('array', $properties); - $this->assertCount(5, $properties); - - $propertyNames = array_map( - function (\ReflectionProperty $property) { - return $property->getDeclaringClass()->getName() . '::' . $property->getName(); - }, - $properties - ); - - sort($propertyNames); - - $this->assertEquals( - [ - 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::id', - 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::name', - 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::nodeIsLoaded', - 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::parentDirectory', - 'Doctrine\Tests\Models\DirectoryTree\File::extension', - ], - $propertyNames - ); - - $this->assertNotContains( - 'Doctrine\Tests\Models\DirectoryTree\AbstractContentItem::fileSystem', - $propertyNames, - 'Abstract properties should not be part of class metadata information' - ); - } } /**