Merge pull request #370 from carnage/fix-126

#126: Fixes issues reading/writing typed properties which are uninitialised
Ocramius · Jan 9, 2024 · 1679eaa · 1679eaa
2 parents a02cd04 + 4b6d35b
commit 1679eaa
Show file tree

Hide file tree

Showing 3 changed files with 51 additions and 16 deletions.
diff --git a/src/GeneratedHydrator/CodeGenerator/Visitor/HydratorMethodsVisitor.php b/src/GeneratedHydrator/CodeGenerator/Visitor/HydratorMethodsVisitor.php
@@ -21,6 +21,7 @@
 use function array_merge;
 use function implode;
 use function reset;
+use function sprintf;
 use function var_export;
 
 /**
@@ -107,18 +108,38 @@ private function generatePropertyHydrateCall(ObjectProperty $property, string $i
         $escapedName  = var_export($propertyName, true);
 
         if ($property->allowsNull && ! $property->hasDefault) {
-            return ['$object->' . $propertyName . ' = ' . $inputArrayName . '[' . $escapedName . '] ?? null;'];
+            return [sprintf('$object->%s = %s[%s] ?? null;', $propertyName, $inputArrayName, $escapedName)];
         }
 
         return [
-            'if (isset(' . $inputArrayName . '[' . $escapedName . '])',
-            '    || $object->' . $propertyName . ' !== null && \\array_key_exists(' . $escapedName . ', ' . $inputArrayName . ')',
+            sprintf('if (isset(%s[%s]) || isset($object->%s) && \\array_key_exists(%2$s, %1$s)', $inputArrayName, $escapedName, $propertyName),
             ') {',
-            '    $object->' . $propertyName . ' = ' . $inputArrayName . '[' . $escapedName . '];',
+            sprintf('    $object->%s = %s[%s];', $propertyName, $inputArrayName, $escapedName),
             '}',
         ];
     }
 
+    /**
+     * @return string[]
+     * @psalm-return list<string>
+     */
+    private function generatePropertyExtractCall(ObjectProperty $property): array
+    {
+        $propertyName        = $property->name;
+        $assignmentStatement = sprintf('    $values[\'%s\'] = $object->%1$s;', $propertyName);
+        $requiresGuard       = $property->hasType && ! ($property->hasDefault || $property->allowsNull);
+
+        if (! $requiresGuard) {
+            return [$assignmentStatement];
+        }
+
+        return [
+            sprintf('    if (isset($object->%s)) {', $propertyName),
+            $assignmentStatement,
+            '    }',
+        ];
+    }
+
     private function replaceConstructor(ClassMethod $method): void
     {
         $method->params = [];
@@ -135,16 +156,15 @@ private function replaceConstructor(ClassMethod $method): void
                 $bodyParts = array_merge($bodyParts, $this->generatePropertyHydrateCall($property, '$values'));
             }
 
-            $bodyParts[] = '}, null, ' . var_export($className, true) . ');' . "\n";
+            $bodyParts[] = sprintf("}, null, %s);\n", var_export($className, true));
 
             // Extract closures
             $bodyParts[] = '$this->extractCallbacks[] = \\Closure::bind(static function ($object, &$values) {';
             foreach ($properties as $property) {
-                $propertyName = $property->name;
-                $bodyParts[]  = "    \$values['" . $propertyName . "'] = \$object->" . $propertyName . ';';
+                $bodyParts = array_merge($bodyParts, $this->generatePropertyExtractCall($property));
             }
 
-            $bodyParts[] = '}, null, ' . var_export($className, true) . ');' . "\n";
+            $bodyParts[] = sprintf("}, null, %s);\n", var_export($className, true));
         }
 
         $method->stmts = (new ParserFactory())

diff --git a/src/GeneratedHydrator/CodeGenerator/Visitor/ObjectProperty.php b/src/GeneratedHydrator/CodeGenerator/Visitor/ObjectProperty.php
@@ -15,13 +15,9 @@
  */
 final class ObjectProperty
 {
-    /** @psalm-var non-empty-string */
-    public string $name;
-
     /** @psalm-param non-empty-string $name */
-    private function __construct(string $name, public bool $hasType, public bool $allowsNull, public bool $hasDefault)
+    private function __construct(public string $name, public bool $hasType, public bool $allowsNull, public bool $hasDefault)
     {
-        $this->name = $name;
     }
 
     public static function fromReflection(ReflectionProperty $property): self

diff --git a/tests/GeneratedHydratorTest/Functional/HydratorFunctionalTest.php b/tests/GeneratedHydratorTest/Functional/HydratorFunctionalTest.php
@@ -79,8 +79,6 @@ public function testHydratingNull(): void
      * Ensures that the hydrator will not attempt to read unitialized PHP >= 7.4
      * typed property, which would cause "Uncaught Error: Typed property Foo::$a
      * must not be accessed before initialization" PHP engine errors.
-     *
-     * @requires PHP >= 7.4
      */
     public function testHydratorWillNotRaisedUnitiliazedTypedPropertyAccessError(): void
     {
@@ -100,7 +98,28 @@ public function testHydratorWillNotRaisedUnitiliazedTypedPropertyAccessError():
         ], $hydrator->extract($instance));
     }
 
-    /** @requires PHP >= 7.4 */
+    /**
+     * Ensures that the hydrator will not attempt to read unitialized PHP >= 7.4
+     * typed property, which would cause "Uncaught Error: Typed property Foo::$a
+     * must not be accessed before initialization" PHP engine errors.
+     */
+    public function testHydratorWillNotRaisedUnitiliazedTypedPropertyAccessErrorIfPropertyIsntHydrated(): void
+    {
+        $instance = new ClassWithTypedProperties();
+        $hydrator = $this->generateHydrator($instance);
+
+        $hydrator->hydrate(['untyped0' => 3], $instance);
+
+        self::assertSame([
+            'property0' => 1, // 'property0' has a default value, it should keep it.
+            'property1' => 2, // 'property1' has a default value, it should keep it.
+            'property3' => null, // 'property3' is not required, it should remain null.
+            'property4' => null, // 'property4' default value is null, it should remain null.
+            'untyped0' => 3, // 'untyped0' is null by default
+            'untyped1' => null, // 'untyped1' is null by default
+        ], $hydrator->extract($instance));
+    }
+
     public function testHydratorWillSetAllTypedProperties(): void
     {
         $instance = new ClassWithTypedProperties();