Reordered property type resolution to resolve typed properties before…

… untyped ones Due to how PHP 7.4 works, if you have a typed public property that does not support null values, whenever Doctrine tries to delete an entity with such properties, it raises a TypeError. This happens because of the new default "uninitialized" value for typed properties. In previous versions of PHP, the default uninitialized value was null. My fist idea was to just reverse the order, and while that worked, it introduced a BC break. Now, as per @guilliamxavier suggestion, I've moved the methods from the class TypedNoDefaultRuntimePublicReflectionProperty to a trait, then I proceeded to create a new class extending RuntimePublicReflectionProperty, that uses the new trait. With this, whenever Doctrine tries to access a public property with no default value set, it will properly unset the property instead of setting it as null getting a PHP TypeError. Reordered property type resolution so typed properties get resolved first, added tests for covering multiple scenarios Fixes #102 Fixes doctrine/orm#7999 Co-authored-by: Guilliam Xavier <guilliamxavier@users.noreply.github.com>
doctrine · Jun 15, 2021 · dc3d9eb · dc3d9eb
1 parent 79e97bc
commit dc3d9eb
Show file tree

Hide file tree

Showing 5 changed files with 117 additions and 52 deletions.
diff --git a/lib/Doctrine/Persistence/Mapping/RuntimeReflectionService.php b/lib/Doctrine/Persistence/Mapping/RuntimeReflectionService.php
@@ -4,6 +4,7 @@
 
 use Doctrine\Persistence\Reflection\RuntimePublicReflectionProperty;
 use Doctrine\Persistence\Reflection\TypedNoDefaultReflectionProperty;
+use Doctrine\Persistence\Reflection\TypedNoDefaultRuntimePublicReflectionProperty;
 use ReflectionClass;
 use ReflectionException;
 use ReflectionMethod;
@@ -86,10 +87,14 @@ public function getAccessibleProperty($class, $property)
     {
         $reflectionProperty = new ReflectionProperty($class, $property);
 
-        if ($reflectionProperty->isPublic()) {
+        if ($this->supportsTypedPropertiesWorkaround && ! array_key_exists($property, $this->getClass($class)->getDefaultProperties())) {
+            if ($reflectionProperty->isPublic()) {
+                $reflectionProperty = new TypedNoDefaultRuntimePublicReflectionProperty($class, $property);
+            } else {
+                $reflectionProperty = new TypedNoDefaultReflectionProperty($class, $property);
+            }
+        } elseif ($reflectionProperty->isPublic()) {
             $reflectionProperty = new RuntimePublicReflectionProperty($class, $property);
-        } elseif ($this->supportsTypedPropertiesWorkaround && ! array_key_exists($property, $this->getClass($class)->getDefaultProperties())) {
-            $reflectionProperty = new TypedNoDefaultReflectionProperty($class, $property);
         }
 
         $reflectionProperty->setAccessible(true);

diff --git a/lib/Doctrine/Persistence/Reflection/TypedNoDefaultReflectionProperty.php b/lib/Doctrine/Persistence/Reflection/TypedNoDefaultReflectionProperty.php
@@ -2,60 +2,12 @@
 
 namespace Doctrine\Persistence\Reflection;
 
-use Closure;
 use ReflectionProperty;
-use ReturnTypeWillChange;
-
-use function assert;
 
 /**
  * PHP Typed No Default Reflection Property - special override for typed properties without a default value.
  */
 class TypedNoDefaultReflectionProperty extends ReflectionProperty
 {
-    /**
-     * {@inheritDoc}
-     *
-     * Checks that a typed property is initialized before accessing its value.
-     * This is necessary to avoid PHP error "Error: Typed property must not be accessed before initialization".
-     * Should be used only for reflecting typed properties without a default value.
-     *
-     * @param object $object
-     */
-    #[ReturnTypeWillChange]
-    public function getValue($object = null)
-    {
-        return $object !== null && $this->isInitialized($object) ? parent::getValue($object) : null;
-    }
-
-    /**
-     * {@inheritDoc}
-     *
-     * Works around the problem with setting typed no default properties to
-     * NULL which is not supported, instead unset() to uninitialize.
-     *
-     * @link https://github.com/doctrine/orm/issues/7999
-     *
-     * @param object $object
-     */
-    #[ReturnTypeWillChange]
-    public function setValue($object, $value = null)
-    {
-        if ($value === null && $this->hasType() && ! $this->getType()->allowsNull()) {
-            $propertyName = $this->getName();
-
-            $unsetter = function () use ($propertyName): void {
-                unset($this->$propertyName);
-            };
-            $unsetter = $unsetter->bindTo($object, $this->getDeclaringClass()->getName());
-
-            assert($unsetter instanceof Closure);
-
-            $unsetter();
-
-            return;
-        }
-
-        parent::setValue($object, $value);
-    }
+    use TypedNoDefaultReflectionPropertyBase;
 }
diff --git a/lib/Doctrine/Persistence/Reflection/TypedNoDefaultReflectionPropertyBase.php b/lib/Doctrine/Persistence/Reflection/TypedNoDefaultReflectionPropertyBase.php
@@ -0,0 +1,60 @@
+<?php
+
+namespace Doctrine\Persistence\Reflection;
+
+use Closure;
+use ReturnTypeWillChange;
+
+use function assert;
+
+/**
+ * PHP Typed No Default Reflection Property Base - special override for typed properties without a default value.
+ */
+trait TypedNoDefaultReflectionPropertyBase
+{
+    /**
+     * {@inheritDoc}
+     *
+     * Checks that a typed property is initialized before accessing its value.
+     * This is necessary to avoid PHP error "Error: Typed property must not be accessed before initialization".
+     * Should be used only for reflecting typed properties without a default value.
+     *
+     * @param object $object
+     */
+    #[ReturnTypeWillChange]
+    public function getValue($object = null)
+    {
+        return $object !== null && $this->isInitialized($object) ? parent::getValue($object) : null;
+    }
+
+    /**
+     * {@inheritDoc}
+     *
+     * Works around the problem with setting typed no default properties to
+     * NULL which is not supported, instead unset() to uninitialize.
+     *
+     * @link https://github.com/doctrine/orm/issues/7999
+     *
+     * @param object $object
+     */
+    #[ReturnTypeWillChange]
+    public function setValue($object, $value = null)
+    {
+        if ($value === null && $this->hasType() && ! $this->getType()->allowsNull()) {
+            $propertyName = $this->getName();
+
+            $unsetter = function () use ($propertyName): void {
+                unset($this->$propertyName);
+            };
+            $unsetter = $unsetter->bindTo($object, $this->getDeclaringClass()->getName());
+
+            assert($unsetter instanceof Closure);
+
+            $unsetter();
+
+            return;
+        }
+
+        parent::setValue($object, $value);
+    }
+}
diff --git a/lib/Doctrine/Persistence/Reflection/TypedNoDefaultRuntimePublicReflectionProperty.php b/lib/Doctrine/Persistence/Reflection/TypedNoDefaultRuntimePublicReflectionProperty.php
@@ -0,0 +1,11 @@
+<?php
+
+namespace Doctrine\Persistence\Reflection;
+
+/**
+ * PHP Typed No Default Runtime Public Reflection Property - special override for public typed properties without a default value.
+ */
+class TypedNoDefaultRuntimePublicReflectionProperty extends RuntimePublicReflectionProperty
+{
+    use TypedNoDefaultReflectionPropertyBase;
+}
diff --git a/tests/Doctrine/Tests_PHP74/Persistence/Mapping/RuntimeReflectionServiceTest.php b/tests/Doctrine/Tests_PHP74/Persistence/Mapping/RuntimeReflectionServiceTest.php
@@ -7,6 +7,7 @@
 use Doctrine\Persistence\Mapping\RuntimeReflectionService;
 use Doctrine\Persistence\Reflection\RuntimePublicReflectionProperty;
 use Doctrine\Persistence\Reflection\TypedNoDefaultReflectionProperty;
+use Doctrine\Persistence\Reflection\TypedNoDefaultRuntimePublicReflectionProperty;
 use PHPUnit\Framework\TestCase;
 use ReflectionProperty;
 
@@ -20,8 +21,17 @@ class RuntimeReflectionServiceTest extends TestCase
 
     private string $typedNoDefaultProperty;
     private string $typedDefaultProperty = '';
+    /** @var string */
+    private $nonTypedNoDefaultProperty; // phpcs:ignore SlevomatCodingStandard.Classes.UnusedPrivateElements.UnusedProperty
+    /** @var string */
+    private $nonTypedDefaultProperty = ''; // phpcs:ignore SlevomatCodingStandard.Classes.UnusedPrivateElements.UnusedProperty
 
     public string $typedNoDefaultPublicProperty;
+    public string $typedDefaultPublicProperty = '';
+    /** @var string */
+    public $nonTypedNoDefaultPublicProperty;
+    /** @var string */
+    public $nonTypedDefaultPublicProperty = '';
 
     protected function setUp(): void
     {
@@ -45,6 +55,33 @@ public function testGetTypedPublicNoDefaultPropertyWorksWithGetValue(): void
     {
         $reflProp = $this->reflectionService->getAccessibleProperty(self::class, 'typedNoDefaultPublicProperty');
         self::assertInstanceOf(RuntimePublicReflectionProperty::class, $reflProp);
+        self::assertInstanceOf(TypedNoDefaultRuntimePublicReflectionProperty::class, $reflProp);
         self::assertNull($reflProp->getValue($this));
     }
+
+    public function testGetNonTypedNoDefaultReflectionProperty(): void
+    {
+        $reflProp = $this->reflectionService->getAccessibleProperty(self::class, 'nonTypedNoDefaultProperty');
+        self::assertInstanceOf(ReflectionProperty::class, $reflProp);
+    }
+
+    public function testGetNonTypedDefaultReflectionProperty(): void
+    {
+        $reflProp = $this->reflectionService->getAccessibleProperty(self::class, 'nonTypedDefaultProperty');
+        self::assertInstanceOf(ReflectionProperty::class, $reflProp);
+        self::assertNotInstanceOf(TypedNoDefaultReflectionProperty::class, $reflProp);
+    }
+
+    public function testGetTypedPublicDefaultPropertyWorksWithGetValue(): void
+    {
+        $reflProp = $this->reflectionService->getAccessibleProperty(self::class, 'typedDefaultPublicProperty');
+        self::assertInstanceOf(ReflectionProperty::class, $reflProp);
+        self::assertNotInstanceOf(TypedNoDefaultReflectionProperty::class, $reflProp);
+    }
+
+    public function testGetNonTypedPublicDefaultPropertyWorksWithGetValue(): void
+    {
+        $reflProp = $this->reflectionService->getAccessibleProperty(self::class, 'nonTypedDefaultPublicProperty');
+        self::assertInstanceOf(RuntimePublicReflectionProperty::class, $reflProp);
+    }
 }