From 629cdb63c6bd98cd1ffc827b51950ec42ea3c69e Mon Sep 17 00:00:00 2001 From: Zan Baldwin Date: Fri, 11 Feb 2022 19:49:36 +0100 Subject: [PATCH 1/5] Don't Generate Set or Unset on Proxy of Readonly Public Properties --- lib/Doctrine/Common/Proxy/ProxyGenerator.php | 31 ++++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/lib/Doctrine/Common/Proxy/ProxyGenerator.php b/lib/Doctrine/Common/Proxy/ProxyGenerator.php index 89a945b53..9ae43b044 100644 --- a/lib/Doctrine/Common/Proxy/ProxyGenerator.php +++ b/lib/Doctrine/Common/Proxy/ProxyGenerator.php @@ -474,7 +474,7 @@ public function __construct(?\Closure $initializer = null, ?\Closure $cloner = n $toUnset = array_map(static function (string $name): string { return '$this->' . $name; - }, $this->getLazyLoadedPublicPropertiesNames($class)); + }, $this->getWriteableLazyLoadedPublicPropertiesNames($class)); return $constructorImpl . ($toUnset === [] ? '' : ' unset(' . implode(', ', $toUnset) . ");\n") . <<<'EOT' @@ -591,7 +591,7 @@ public function {$returnReference}__get($parametersString)$returnTypeHint */ private function generateMagicSet(ClassMetadata $class) { - $lazyPublicProperties = $this->getLazyLoadedPublicPropertiesNames($class); + $lazyPublicProperties = $this->getWriteableLazyLoadedPublicPropertiesNames($class); $reflectionClass = $class->getReflectionClass(); $hasParentSet = false; $inheritDoc = ''; @@ -808,7 +808,7 @@ private function generateWakeupImpl(ClassMetadata $class) $hasParentWakeup = $reflectionClass->hasMethod('__wakeup'); $unsetPublicProperties = []; - foreach ($this->getLazyLoadedPublicPropertiesNames($class) as $lazyPublicProperty) { + foreach ($this->getWriteableLazyLoadedPublicPropertiesNames($class) as $lazyPublicProperty) { $unsetPublicProperties[] = '$this->' . $lazyPublicProperty; } @@ -1005,6 +1005,31 @@ private function isShortIdentifierGetter($method, ClassMetadata $class) return false; } + /** + * Generates the list of public properties to be lazy loaded, that are writable. + * + * @return array + */ + public function getWriteableLazyLoadedPublicPropertiesNames(ClassMetadata $class): array + { + $properties = []; + + foreach ($class->getReflectionClass()->getProperties(ReflectionProperty::IS_PUBLIC) as $property) { + $name = $property->getName(); + + if ((! $class->hasField($name) && ! $class->hasAssociation($name)) + || $class->isIdentifier($name) + || (method_exists($property, 'isReadOnly') && $property->isReadOnly()) + ) { + continue; + } + + $properties[] = $name; + } + + return $properties; + } + /** * Generates the list of public properties to be lazy loaded. * From 830786ede0dcef742b812c3f742978180558c1a7 Mon Sep 17 00:00:00 2001 From: Zan Baldwin Date: Fri, 11 Feb 2022 20:37:54 +0100 Subject: [PATCH 2/5] Add Unit Test for Readonly Properties on PHP 8.1 --- lib/Doctrine/Common/Proxy/ProxyGenerator.php | 2 +- .../Proxy/Php81ReadonlyPublicPropertyType.php | 13 +++++++ .../Tests/Common/Proxy/ProxyGeneratorTest.php | 35 +++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php diff --git a/lib/Doctrine/Common/Proxy/ProxyGenerator.php b/lib/Doctrine/Common/Proxy/ProxyGenerator.php index 9ae43b044..ae10f90c0 100644 --- a/lib/Doctrine/Common/Proxy/ProxyGenerator.php +++ b/lib/Doctrine/Common/Proxy/ProxyGenerator.php @@ -1008,7 +1008,7 @@ private function isShortIdentifierGetter($method, ClassMetadata $class) /** * Generates the list of public properties to be lazy loaded, that are writable. * - * @return array + * @return list */ public function getWriteableLazyLoadedPublicPropertiesNames(ClassMetadata $class): array { diff --git a/tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php b/tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php new file mode 100644 index 000000000..011cbfa11 --- /dev/null +++ b/tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php @@ -0,0 +1,13 @@ += 8.1.0 + */ + public function testPhp81ReadonlyPublicProperties() + { + $className = Php81ReadonlyPublicPropertyType::class; + $proxyClassName = 'Doctrine\Tests\Common\ProxyProxy\__CG__\Php81ReadonlyPublicPropertyType'; + + if ( ! class_exists($proxyClassName, false)) { + $metadata = $this->createClassMetadata($className, ['id']); + + $metadata + ->expects($this->any()) + ->method('hasField') + ->will($this->returnCallback(static function ($fieldName) { + return in_array($fieldName, ['id', 'readable', 'writeable']); + })); + + $proxyGenerator = new ProxyGenerator(__DIR__ . '/generated', __NAMESPACE__ . 'Proxy'); + $this->generateAndRequire($proxyGenerator, $metadata); + } + + // Readonly properties are removed from unset. + self::assertStringContainsString( + 'unset($this->writeable);', + file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyPhp81ReadonlyPublicPropertyType.php') + ); + + // But remain in property listings. + self::assertStringContainsString( + "'readable' => NULL", + file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyPhp81ReadonlyPublicPropertyType.php') + ); + } + /** * @requires PHP >= 8.1.0 */ From d189500ac11a30f20715495edd3a22d793b3118d Mon Sep 17 00:00:00 2001 From: Zan Baldwin Date: Fri, 11 Feb 2022 20:43:04 +0100 Subject: [PATCH 3/5] Fix CI Pipeline Errors - Code styling: multi-line control structure - Static analysis: uninitialized readonly property --- lib/Doctrine/Common/Proxy/ProxyGenerator.php | 3 ++- .../Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/Doctrine/Common/Proxy/ProxyGenerator.php b/lib/Doctrine/Common/Proxy/ProxyGenerator.php index ae10f90c0..35a6dbc96 100644 --- a/lib/Doctrine/Common/Proxy/ProxyGenerator.php +++ b/lib/Doctrine/Common/Proxy/ProxyGenerator.php @@ -1017,7 +1017,8 @@ public function getWriteableLazyLoadedPublicPropertiesNames(ClassMetadata $class foreach ($class->getReflectionClass()->getProperties(ReflectionProperty::IS_PUBLIC) as $property) { $name = $property->getName(); - if ((! $class->hasField($name) && ! $class->hasAssociation($name)) + if ( + (! $class->hasField($name) && ! $class->hasAssociation($name)) || $class->isIdentifier($name) || (method_exists($property, 'isReadOnly') && $property->isReadOnly()) ) { diff --git a/tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php b/tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php index 011cbfa11..666824c16 100644 --- a/tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php +++ b/tests/Doctrine/Tests/Common/Proxy/Php81ReadonlyPublicPropertyType.php @@ -9,5 +9,8 @@ class Php81ReadonlyPublicPropertyType public function __construct( public readonly string $id, - ) {} + string $readable = 'readable-default' + ) { + $this->readable = $readable; + } } From 1521aa8c0701a38410058f5d3e2f6605bde7d792 Mon Sep 17 00:00:00 2001 From: Zan Baldwin Date: Sat, 12 Feb 2022 16:27:18 +0100 Subject: [PATCH 4/5] Test Accessing Readonly Properties after Constructing Proxy --- .../Tests/Common/Proxy/ProxyGeneratorTest.php | 48 +++++++++++++++---- 1 file changed, 40 insertions(+), 8 deletions(-) diff --git a/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php b/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php index 6118ccfc2..82968d372 100644 --- a/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php +++ b/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php @@ -4,6 +4,7 @@ use Doctrine\Common\Proxy\Exception\InvalidArgumentException; use Doctrine\Common\Proxy\Exception\UnexpectedValueException; +use Doctrine\Common\Proxy\Proxy; use Doctrine\Common\Proxy\ProxyGenerator; use Doctrine\Persistence\Mapping\ClassMetadata; use PHPUnit\Framework\MockObject\MockObject; @@ -531,7 +532,12 @@ public function testPhp81NeverType() public function testPhp81ReadonlyPublicProperties() { $className = Php81ReadonlyPublicPropertyType::class; - $proxyClassName = 'Doctrine\Tests\Common\ProxyProxy\__CG__\Php81ReadonlyPublicPropertyType'; + $proxyClassName = 'Doctrine\Tests\Common\ProxyProxy\__CG__\\' . $className; + $initializationData = [ + 'id' => 'c0b5cb93-f01b-43f8-bc66-bc943b1ebcfd', + 'readable' => 'This field is read-only.', + 'writeable' => 'This field is writeable.', + ]; if ( ! class_exists($proxyClassName, false)) { $metadata = $this->createClassMetadata($className, ['id']); @@ -539,8 +545,8 @@ public function testPhp81ReadonlyPublicProperties() $metadata ->expects($this->any()) ->method('hasField') - ->will($this->returnCallback(static function ($fieldName) { - return in_array($fieldName, ['id', 'readable', 'writeable']); + ->will($this->returnCallback(static function ($fieldName) use ($initializationData) { + return in_array($fieldName, array_keys($initializationData)); })); $proxyGenerator = new ProxyGenerator(__DIR__ . '/generated', __NAMESPACE__ . 'Proxy'); @@ -553,11 +559,37 @@ public function testPhp81ReadonlyPublicProperties() file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyPhp81ReadonlyPublicPropertyType.php') ); - // But remain in property listings. - self::assertStringContainsString( - "'readable' => NULL", - file_get_contents(__DIR__ . '/generated/__CG__DoctrineTestsCommonProxyPhp81ReadonlyPublicPropertyType.php') - ); + $proxy = new $proxyClassName(static function (Proxy $proxy, $method, $params) use (&$counter, $initializationData) { + if (!in_array($params[0], array_keys($initializationData))) { + throw new InvalidArgumentException( + sprintf('Should not be initialized when checking isset("%s")', $params[0]) + ); + } + $initializer = $proxy->__getInitializer(); + $proxy->__setInitializer(null); + isset($this->{$params[0]}) || $this->{$params[0]} = $initializationData[$params[0]]; + $counter += 1; + $proxy->__setInitializer($initializer); + }); + + var_dump($proxy->id); + + self::assertTrue(isset($proxy->id)); + self::assertTrue(isset($proxy->readable)); + self::assertTrue(isset($proxy->writeable)); + self::assertFalse(isset($proxy->nonExisting)); + + self::assertSame('This field is writeable.', $proxy->writeable); + $proxy->writeable = 'Updated string contents.'; + self::assertSame('Updated string contents.', $proxy->writeable); + + try { + $proxy->readable = 'Invalid'; + self::fail('Should not be able to update readonly property.'); + } catch (\Error) { + } + + self::assertSame(3, $counter); } /** From 306a5ad353959090e65a9e6d3c7e9aaab7779659 Mon Sep 17 00:00:00 2001 From: Zan Baldwin Date: Sat, 12 Feb 2022 17:20:22 +0100 Subject: [PATCH 5/5] Readonly Properties Unit Test: Update Assertions Better unit testing suggestions by @greg0ire --- .../Tests/Common/Proxy/ProxyGeneratorTest.php | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php b/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php index 82968d372..06bd0360a 100644 --- a/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php +++ b/tests/Doctrine/Tests/Common/Proxy/ProxyGeneratorTest.php @@ -543,9 +543,8 @@ public function testPhp81ReadonlyPublicProperties() $metadata = $this->createClassMetadata($className, ['id']); $metadata - ->expects($this->any()) ->method('hasField') - ->will($this->returnCallback(static function ($fieldName) use ($initializationData) { + ->will($this->returnCallback(static function (string $fieldName) use ($initializationData): bool { return in_array($fieldName, array_keys($initializationData)); })); @@ -572,24 +571,20 @@ public function testPhp81ReadonlyPublicProperties() $proxy->__setInitializer($initializer); }); - var_dump($proxy->id); - self::assertTrue(isset($proxy->id)); self::assertTrue(isset($proxy->readable)); self::assertTrue(isset($proxy->writeable)); self::assertFalse(isset($proxy->nonExisting)); + self::assertSame('c0b5cb93-f01b-43f8-bc66-bc943b1ebcfd', $proxy->id); self::assertSame('This field is writeable.', $proxy->writeable); $proxy->writeable = 'Updated string contents.'; self::assertSame('Updated string contents.', $proxy->writeable); - try { - $proxy->readable = 'Invalid'; - self::fail('Should not be able to update readonly property.'); - } catch (\Error) { - } - self::assertSame(3, $counter); + + self::expectError(); + $proxy->readable = 'Invalid'; } /**