From 0f00cb90562ae434b3b2c62db9568bf945cca5a9 Mon Sep 17 00:00:00 2001 From: Andreas Braun Date: Thu, 28 May 2020 21:07:15 +0200 Subject: [PATCH] Fix proxies with return types in magic methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Jan Barášek Co-authored-by: Miloš Havlíček --- lib/Doctrine/Common/Proxy/ProxyGenerator.php | 109 +++++++---- phpstan.neon.dist | 3 + .../Proxy/MagicGetClassWithScalarType.php | 39 ++++ ...ClassWithScalarTypeAndRenamedParameter.php | 39 ++++ .../Common/Proxy/MagicGetClassWithVoid.php | 21 +++ .../Proxy/MagicIssetClassWithBoolean.php | 39 ++++ .../Proxy/MagicIssetClassWithInteger.php | 39 ++++ .../Proxy/MagicSetClassWithScalarType.php | 44 +++++ .../Common/Proxy/ProxyMagicMethodsTest.php | 173 ++++++++++++++++++ 9 files changed, 474 insertions(+), 32 deletions(-) create mode 100644 tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithScalarType.php create mode 100644 tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithScalarTypeAndRenamedParameter.php create mode 100644 tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithVoid.php create mode 100644 tests/Doctrine/Tests/Common/Proxy/MagicIssetClassWithBoolean.php create mode 100644 tests/Doctrine/Tests/Common/Proxy/MagicIssetClassWithInteger.php create mode 100644 tests/Doctrine/Tests/Common/Proxy/MagicSetClassWithScalarType.php diff --git a/lib/Doctrine/Common/Proxy/ProxyGenerator.php b/lib/Doctrine/Common/Proxy/ProxyGenerator.php index 7b07c54e6..5180782e8 100644 --- a/lib/Doctrine/Common/Proxy/ProxyGenerator.php +++ b/lib/Doctrine/Common/Proxy/ProxyGenerator.php @@ -1,10 +1,10 @@ hasMethod('__get')) { - $hasParentGet = true; - $inheritDoc = '{@inheritDoc}'; + $hasParentGet = true; + $inheritDoc = '{@inheritDoc}'; + $methodReflection = $reflectionClass->getMethod('__get'); - if ($reflectionClass->getMethod('__get')->returnsReference()) { + if ($methodReflection->returnsReference()) { $returnReference = '& '; } + + $methodParameters = $methodReflection->getParameters(); + $name = '$' . $methodParameters[0]->getName(); + + $parametersString = $this->buildParametersString($methodReflection->getParameters()); + $returnTypeHint = $this->getMethodReturnType($methodReflection); } if (empty($lazyPublicProperties) && ! $hasParentGet) { @@ -450,19 +460,28 @@ private function generateMagicGet(ClassMetadata $class) $magicGet = <<__initializer__ && $this->__initializer__->__invoke($this, '__get', [%s]); +EOT + , $name, $name); + + if ($returnTypeHint === ': void') { + $magicGet .= "\n return;"; + } else { + $magicGet .= "\n return \$this->$name;"; + } + $magicGet .= <<<'EOT' - if (\array_key_exists($name, self::$lazyPropertiesNames)) { - $this->__initializer__ && $this->__initializer__->__invoke($this, '__get', [$name]); - return $this->$name; } @@ -470,22 +489,34 @@ public function {$returnReference}__get(\$name) } if ($hasParentGet) { - $magicGet .= <<<'EOT' - $this->__initializer__ && $this->__initializer__->__invoke($this, '__get', [$name]); - - return parent::__get($name); - -EOT; + $magicGet .= sprintf(<<<'EOT' + $this->__initializer__ && $this->__initializer__->__invoke($this, '__get', [%s]); +EOT + , $name); + + if ($returnTypeHint === ': void') { + $magicGet .= sprintf(<<<'EOT' + + parent::__get(%s); + return; +EOT + , $name); + } else { + $magicGet .= sprintf(<<<'EOT' + + return parent::__get(%s); +EOT + , $name); + } } else { - $magicGet .= <<<'EOT' - trigger_error(sprintf('Undefined property: %s::$%s', __CLASS__, $name), E_USER_NOTICE); + $magicGet .= sprintf(<<getLazyLoadedPublicPropertiesNames($class); $hasParentSet = $class->getReflectionClass()->hasMethod('__set'); + $parametersString = '$name, $value'; + $returnTypeHint = null; + + if ($hasParentSet) { + $methodReflection = $class->getReflectionClass()->getMethod('__set'); + $parametersString = $this->buildParametersString($methodReflection->getParameters()); + $returnTypeHint = $this->getMethodReturnType($methodReflection); + } if (empty($lazyPublicProperties) && ! $hasParentSet) { return ''; } $inheritDoc = $hasParentSet ? '{@inheritDoc}' : ''; - $magicSet = <<getLazyLoadedPublicPropertiesNames($class); $hasParentIsset = $class->getReflectionClass()->hasMethod('__isset'); + $parametersString = '$name'; + $returnTypeHint = null; + + if ($hasParentIsset) { + $methodReflection = $class->getReflectionClass()->getMethod('__isset'); + $parametersString = $this->buildParametersString($methodReflection->getParameters()); + $returnTypeHint = $this->getMethodReturnType($methodReflection); + } if (empty($lazyPublicProperties) && ! $hasParentIsset) { return ''; @@ -568,7 +614,7 @@ private function generateMagicIsset(ClassMetadata $class) * @param string \$name * @return boolean */ - public function __isset(\$name) + public function __isset($parametersString)$returnTypeHint { EOT; @@ -590,7 +636,6 @@ public function __isset(\$name) $this->__initializer__ && $this->__initializer__->__invoke($this, '__isset', [$name]); return parent::__isset($name); - EOT; } else { $magicIsset .= " return false;"; diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 5ce87e0fb..56ba1e6bc 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -37,6 +37,9 @@ parameters: - message: '#^Property Doctrine\\Tests\\Common\\Proxy\\ProxyLogicVoidReturnTypeTest::\$initializerCallbackMock \(callable&PHPUnit\\Framework\\MockObject\\MockObject\) does not accept PHPUnit\\Framework\\MockObject\\MockObject&stdClass\.$#' path: '%currentWorkingDirectory%/tests/Doctrine/Tests/Common/Proxy/ProxyLogicVoidReturnTypeTest.php' + - + message: '#^Method Doctrine\\Tests\\Common\\Proxy\\MagicIssetClassWithInteger::__isset\(\) should return bool but returns int\.$#' + path: '%currentWorkingDirectory%/tests/Doctrine/Tests/Common/Proxy/MagicIssetClassWithInteger.php' includes: - vendor/phpstan/phpstan-phpunit/extension.neon diff --git a/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithScalarType.php b/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithScalarType.php new file mode 100644 index 000000000..c48ffee65 --- /dev/null +++ b/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithScalarType.php @@ -0,0 +1,39 @@ + + */ +class MagicGetClassWithScalarType +{ + /** + * @var string + */ + public $id = 'id'; + + /** + * @var string + */ + public $publicField = 'publicField'; + + /** + * @param string $name + * + * @return string + * @throws \BadMethodCallException + */ + public function __get(string $name): string + { + if ($name === 'test') { + return 'test'; + } + + if ($name === 'publicField' || $name === 'id') { + throw new \BadMethodCallException('Should never be called for "publicField" or "id"'); + } + + return 'not defined'; + } +} diff --git a/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithScalarTypeAndRenamedParameter.php b/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithScalarTypeAndRenamedParameter.php new file mode 100644 index 000000000..8818e79bc --- /dev/null +++ b/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithScalarTypeAndRenamedParameter.php @@ -0,0 +1,39 @@ + + */ +class MagicGetClassWithScalarTypeAndRenamedParameter +{ + /** + * @var string + */ + public $id = 'id'; + + /** + * @var string + */ + public $publicField = 'publicField'; + + /** + * @param string $n + * + * @return string + * @throws \BadMethodCallException + */ + public function __get(string $n): string + { + if ($n === 'test') { + return 'test'; + } + + if ($n === 'publicField' || $n === 'id') { + throw new \BadMethodCallException('Should never be called for "publicField" or "id"'); + } + + return 'not defined'; + } +} diff --git a/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithVoid.php b/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithVoid.php new file mode 100644 index 000000000..67197047b --- /dev/null +++ b/tests/Doctrine/Tests/Common/Proxy/MagicGetClassWithVoid.php @@ -0,0 +1,21 @@ + + */ +class MagicIssetClassWithBoolean +{ + /** + * @var string + */ + public $id = 'id'; + + /** + * @var string + */ + public $publicField = 'publicField'; + + /** + * @param string $name + * + * @return bool + * @throws \BadMethodCallException + */ + public function __isset(string $name): bool + { + if ('test' === $name) { + return true; + } + + if ('publicField' === $name || 'id' === $name) { + throw new \BadMethodCallException('Should never be called for "publicField" or "id"'); + } + + return false; + } +} diff --git a/tests/Doctrine/Tests/Common/Proxy/MagicIssetClassWithInteger.php b/tests/Doctrine/Tests/Common/Proxy/MagicIssetClassWithInteger.php new file mode 100644 index 000000000..4f76e8739 --- /dev/null +++ b/tests/Doctrine/Tests/Common/Proxy/MagicIssetClassWithInteger.php @@ -0,0 +1,39 @@ + + */ +class MagicIssetClassWithInteger +{ + /** + * @var string + */ + public $id = 'id'; + + /** + * @var string + */ + public $publicField = 'publicField'; + + /** + * @param string $name + * + * @return int + * @throws \BadMethodCallException + */ + public function __isset(string $name): int + { + if ('test' === $name) { + return 1; + } + + if ('publicField' === $name || 'id' === $name) { + throw new \BadMethodCallException('Should never be called for "publicField" or "id"'); + } + + return 0; + } +} diff --git a/tests/Doctrine/Tests/Common/Proxy/MagicSetClassWithScalarType.php b/tests/Doctrine/Tests/Common/Proxy/MagicSetClassWithScalarType.php new file mode 100644 index 000000000..74eaff1cf --- /dev/null +++ b/tests/Doctrine/Tests/Common/Proxy/MagicSetClassWithScalarType.php @@ -0,0 +1,44 @@ + + */ +class MagicSetClassWithScalarType +{ + /** + * @var string + */ + public $id = 'id'; + + /** + * @var string + */ + public $publicField = 'publicField'; + + /** + * @var string|null + */ + public $testAttribute; + + /** + * @param string $name + * @param mixed $value + * + * @throws \BadMethodCallException + */ + public function __set(string $name, $value) + { + if ($name === 'test') { + $this->testAttribute = $value; + } + + if ($name === 'publicField' || $name === 'id') { + throw new \BadMethodCallException('Should never be called for "publicField" or "id"'); + } + + $this->testAttribute = $value; + } +} diff --git a/tests/Doctrine/Tests/Common/Proxy/ProxyMagicMethodsTest.php b/tests/Doctrine/Tests/Common/Proxy/ProxyMagicMethodsTest.php index 78d9f7077..3d3ddf31b 100644 --- a/tests/Doctrine/Tests/Common/Proxy/ProxyMagicMethodsTest.php +++ b/tests/Doctrine/Tests/Common/Proxy/ProxyMagicMethodsTest.php @@ -74,6 +74,90 @@ function (Proxy $proxy, $method, $params) use (&$counter) { self::assertSame(3, $counter); } + public function testInheritedMagicGetWithScalarType() + { + $proxyClassName = $this->generateProxyClass(MagicGetClassWithScalarType::class); + $proxy = new $proxyClassName( + function (Proxy $proxy, $method, $params) use (&$counter) { + if ( ! in_array($params[0], ['publicField', 'test', 'notDefined'])) { + throw new InvalidArgumentException('Unexpected access to field "' . $params[0] . '"'); + } + + $initializer = $proxy->__getInitializer(); + + $proxy->__setInitializer(null); + + $proxy->publicField = 'modifiedPublicField'; + $counter += 1; + + $proxy->__setInitializer($initializer); + } + ); + + self::assertSame('id', $proxy->id); + self::assertSame('modifiedPublicField', $proxy->publicField); + self::assertSame('test', $proxy->test); + self::assertSame('not defined', $proxy->notDefined); + + self::assertSame(3, $counter); + } + + public function testInheritedMagicGetWithScalarTypeAndRenamedParameter() + { + $proxyClassName = $this->generateProxyClass(MagicGetClassWithScalarTypeAndRenamedParameter::class); + $proxy = new $proxyClassName( + function (Proxy $proxy, $method, $params) use (&$counter) { + if ( ! in_array($params[0], ['publicField', 'test', 'notDefined'])) { + throw new InvalidArgumentException('Unexpected access to field "' . $params[0] . '"'); + } + + $initializer = $proxy->__getInitializer(); + + $proxy->__setInitializer(null); + + $proxy->publicField = 'modifiedPublicField'; + $counter += 1; + + $proxy->__setInitializer($initializer); + } + ); + + self::assertSame('id', $proxy->id); + self::assertSame('modifiedPublicField', $proxy->publicField); + self::assertSame('test', $proxy->test); + self::assertSame('not defined', $proxy->notDefined); + + self::assertSame(3, $counter); + } + + public function testInheritedMagicGetWithVoid() + { + $proxyClassName = $this->generateProxyClass(MagicGetClassWithVoid::class); + $proxy = new $proxyClassName(function (Proxy $proxy, $method, $params) use (&$counter) { + if (in_array($params[0], ['publicField', 'test'])) { + $initializer = $proxy->__getInitializer(); + + $proxy->__setInitializer(null); + + $proxy->publicField = 'modifiedPublicField'; + $counter += 1; + + $proxy->__setInitializer($initializer); + + return; + } + + throw new InvalidArgumentException( + sprintf('Should not be initialized when checking isset("%s")', $params[0]) + ); + }); + + self::assertNull($proxy->publicField); + self::assertNull($proxy->test); + + self::assertSame(2, $counter); + } + /** * @group DCOM-194 */ @@ -125,6 +209,35 @@ function (Proxy $proxy, $method, $params) use (&$counter) { self::assertSame(3, $counter); } + public function testInheritedMagicSetWithScalarType() + { + $proxyClassName = $this->generateProxyClass(MagicSetClassWithScalarType::class); + $proxy = new $proxyClassName( + function (Proxy $proxy, $method, $params) use (&$counter) { + if ( ! in_array($params[0], ['publicField', 'test', 'notDefined'])) { + throw new InvalidArgumentException('Unexpected access to field "' . $params[0] . '"'); + } + + $counter += 1; + } + ); + + self::assertSame('id', $proxy->id); + + $proxy->publicField = 'publicFieldValue'; + + self::assertSame('publicFieldValue', $proxy->publicField); + + $proxy->test = 'testValue'; + + self::assertSame('testValue', $proxy->testAttribute); + + $proxy->notDefined = 'not defined'; + + self::assertSame('not defined', $proxy->testAttribute); + self::assertSame(3, $counter); + } + public function testInheritedMagicSleep() { $proxyClassName = $this->generateProxyClass(MagicSleepClass::class); @@ -193,6 +306,66 @@ public function testInheritedMagicIsset() self::assertSame(3, $counter); } + public function testInheritedMagicIssetWithBoolean() + { + $proxyClassName = $this->generateProxyClass(MagicIssetClassWithBoolean::class); + $proxy = new $proxyClassName(function (Proxy $proxy, $method, $params) use (&$counter) { + if (in_array($params[0], ['publicField', 'test', 'nonExisting'])) { + $initializer = $proxy->__getInitializer(); + + $proxy->__setInitializer(null); + + $proxy->publicField = 'modifiedPublicField'; + $counter += 1; + + $proxy->__setInitializer($initializer); + + return; + } + + throw new InvalidArgumentException( + sprintf('Should not be initialized when checking isset("%s")', $params[0]) + ); + }); + + self::assertTrue(isset($proxy->id)); + self::assertTrue(isset($proxy->publicField)); + self::assertTrue(isset($proxy->test)); + self::assertFalse(isset($proxy->nonExisting)); + + self::assertSame(3, $counter); + } + + public function testInheritedMagicIssetWithInteger() + { + $proxyClassName = $this->generateProxyClass(MagicIssetClassWithInteger::class); + $proxy = new $proxyClassName(function (Proxy $proxy, $method, $params) use (&$counter) { + if (in_array($params[0], ['publicField', 'test', 'nonExisting'])) { + $initializer = $proxy->__getInitializer(); + + $proxy->__setInitializer(null); + + $proxy->publicField = 'modifiedPublicField'; + $counter += 1; + + $proxy->__setInitializer($initializer); + + return; + } + + throw new InvalidArgumentException( + sprintf('Should not be initialized when checking isset("%s")', $params[0]) + ); + }); + + self::assertTrue(isset($proxy->id)); + self::assertTrue(isset($proxy->publicField)); + self::assertTrue(isset($proxy->test)); + self::assertFalse(isset($proxy->nonExisting)); + + self::assertSame(3, $counter); + } + public function testInheritedMagicClone() { $proxyClassName = $this->generateProxyClass(MagicCloneClass::class);