Merge pull request #1272 from Ocramius/hotfix/DDC-2704-merge-inherited-transient-properties

[DDC-2704] - merge inherited transient properties - merge properties into uninitialized proxies

lib/Doctrine/ORM/Mapping/Reflection/ReflectionPropertiesGetter.php

@@ -0,0 +1,163 @@
+<?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\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.
+ *
+ * @private This API is for internal use only
+ *
+ * @author Marco Pivetta <ocramius@gmail.com>
+ */
+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;
+    }
+}

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->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());
+        $this->identifierFlattener        = new IdentifierFlattener($this, $em->getMetadataFactory());
-        $this->hydrationCompleteHandler = new HydrationCompleteHandler($this->listenersInvoker, $em);
+        $this->hydrationCompleteHandler   = new HydrationCompleteHandler($this->listenersInvoker, $em);
+        $this->reflectionPropertiesGetter = new ReflectionPropertiesGetter(new RuntimeReflectionService());
     }
 
     /**
@@ -1868,6 +1876,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);
             }
 
@@ -3348,7 +3360,7 @@ class UnitOfWork implements PropertyChangedListener
     {
         $class = $this->em->getClassMetadata(get_class($entity));
 
-        foreach ($class->reflClass->getProperties() as $prop) {
+        foreach ($this->reflectionPropertiesGetter->getProperties($class->name) as $prop) {
             $name = $prop->name;
 
             $prop->setAccessible(true);

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,20 @@ abstract class AbstractContentItem
     /** @column(type="string") */
     protected $name;
 
+    /**
+     * This field is transient and private on purpose
+     *
+     * @var bool
+     */
+    private $nodeIsLoaded = false;
+
+    /**
+     * This field is transient on purpose
+     *
+     * @var mixed
+     */
+    public static $fileSystem;
+
     public function __construct(Directory $parentDir = null)
     {
         $this->parentDirectory = $parentDir;
@@ -61,4 +77,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;
+    }
 }

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";
 

tests/Doctrine/Tests/Models/Reflection/ClassWithMixedProperties.php

@@ -0,0 +1,18 @@
+<?php
+
+namespace Doctrine\Tests\Models\Reflection;
+
+class ClassWithMixedProperties extends ParentClass
+{
+    const CLASSNAME = __CLASS__;
+
+    public static $staticProperty = 'staticProperty';
+
+    public $publicProperty = 'publicProperty';
+
+    protected $protectedProperty = 'protectedProperty';
+
+    private $privateProperty = 'privateProperty';
+
+    private $privatePropertyOverride = 'privatePropertyOverride';
+}

tests/Doctrine/Tests/Models/Reflection/ParentClass.php

@@ -0,0 +1,10 @@
+<?php
+
+namespace Doctrine\Tests\Models\Reflection;
+
+class ParentClass
+{
+    const CLASSNAME = __CLASS__;
+
+    private $privatePropertyOverride = 'privatePropertyOverride';
+}

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

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

@@ -90,6 +90,25 @@ class MergeSharedEntitiesTest extends OrmFunctionalTestCase
 
         $this->assertEquals($picture->file, $picture->otherFile, 'Identical entities must remain identical');
     }
+
+    /**
+     * @group DDC-2704
+     */
+    public function testMergeInheritedTransientPrivateProperties()
+    {
+        $admin1 = new MSEAdmin();
+        $admin2 = new MSEAdmin();
+
+        $admin1->id = 123;
+        $admin2->id = 123;
+
+        $this->_em->persist($admin1);
+
+        $admin2->setSession('zeh current session data');
+
+        $this->assertSame($admin1, $this->_em->merge($admin2));
+        $this->assertSame('zeh current session data', $admin1->getSession());
+    }
 }
 
 /** @Entity */
@@ -111,3 +130,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;
+}

tests/Doctrine/Tests/ORM/Mapping/Reflection/ReflectionPropertiesGetterTest.php

@@ -0,0 +1,129 @@
+<?php
+
+namespace Doctrine\Tests\ORM\Mapping\Reflection;
+
+use Doctrine\Common\Persistence\Mapping\ReflectionService;
+use Doctrine\Common\Persistence\Mapping\RuntimeReflectionService;
+use Doctrine\ORM\Mapping\Reflection\ReflectionPropertiesGetter;
+use Doctrine\Tests\Models\Reflection\ClassWithMixedProperties;
+use Doctrine\Tests\Models\Reflection\ParentClass;
+use PHPUnit_Framework_TestCase;
+use ReflectionClass;
+
+/**
+ * Tests for {@see \Doctrine\ORM\Mapping\Reflection\ReflectionPropertiesGetter}
+ *
+ * @covers \Doctrine\ORM\Mapping\Reflection\ReflectionPropertiesGetter
+ */
+class ReflectionPropertiesGetterTest extends PHPUnit_Framework_TestCase
+{
+    public function testRetrievesProperties()
+    {
+        $properties = (new ReflectionPropertiesGetter(new RuntimeReflectionService()))
+            ->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));
+    }
+}