From ff2ce4d86a792ac80d83023660347894657fd136 Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Tue, 8 May 2018 13:03:27 +0100 Subject: [PATCH 1/6] ReflectionBasedAbstractFactory should not be expected to instantiate private constructors --- .../ReflectionBasedAbstractFactory.php | 21 +++++++++++++++++-- .../ReflectionBasedAbstractFactoryTest.php | 18 +++++++++++++++- .../TestAsset/ClassWithPrivateConstructor.php | 15 +++++++++++++ 3 files changed, 51 insertions(+), 3 deletions(-) create mode 100644 test/AbstractFactory/TestAsset/ClassWithPrivateConstructor.php diff --git a/src/AbstractFactory/ReflectionBasedAbstractFactory.php b/src/AbstractFactory/ReflectionBasedAbstractFactory.php index ba8cf9d6..82e71bb1 100644 --- a/src/AbstractFactory/ReflectionBasedAbstractFactory.php +++ b/src/AbstractFactory/ReflectionBasedAbstractFactory.php @@ -1,7 +1,7 @@ canCallConstructor($requestedName); + } + + /** + * @param string $requestedName + * @return bool + * @throws \ReflectionException + */ + private function canCallConstructor(string $requestedName) : bool + { + $constructor = (new ReflectionClass($requestedName))->getConstructor(); + + if ($constructor === null) { + return true; + } + + return $constructor->isPublic(); } /** diff --git a/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php b/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php index 94f4acd6..eea824f5 100644 --- a/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php +++ b/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php @@ -1,7 +1,7 @@ container = $this->prophesize(ContainerInterface::class); @@ -36,6 +38,20 @@ public function testCanCreateReturnsFalseForNonClassRequestedNames($requestedNam $this->assertFalse($factory->canCreate($this->container->reveal(), $requestedName)); } + /** + * @throws \ReflectionException + */ + public function testCanCreateReturnsFalseWhenConstructorIsPrivate() : void + { + self::assertFalse( + (new ReflectionBasedAbstractFactory())->canCreate( + $this->container->reveal(), + TestAsset\ClassWithPrivateConstructor::class + ), + 'ReflectionBasedAbstractFactory should not be able to instantiate a class with a private constructor' + ); + } + public function testFactoryInstantiatesClassDirectlyIfItHasNoConstructor() { $factory = new ReflectionBasedAbstractFactory(); diff --git a/test/AbstractFactory/TestAsset/ClassWithPrivateConstructor.php b/test/AbstractFactory/TestAsset/ClassWithPrivateConstructor.php new file mode 100644 index 00000000..5c140a65 --- /dev/null +++ b/test/AbstractFactory/TestAsset/ClassWithPrivateConstructor.php @@ -0,0 +1,15 @@ + Date: Tue, 8 May 2018 14:23:29 +0100 Subject: [PATCH 2/6] Update annotations and comments for consistency --- src/AbstractFactory/ReflectionBasedAbstractFactory.php | 8 +------- .../ReflectionBasedAbstractFactoryTest.php | 9 ++++----- 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/src/AbstractFactory/ReflectionBasedAbstractFactory.php b/src/AbstractFactory/ReflectionBasedAbstractFactory.php index 82e71bb1..34d45172 100644 --- a/src/AbstractFactory/ReflectionBasedAbstractFactory.php +++ b/src/AbstractFactory/ReflectionBasedAbstractFactory.php @@ -1,7 +1,7 @@ canCallConstructor($requestedName); } - /** - * @param string $requestedName - * @return bool - * @throws \ReflectionException - */ private function canCallConstructor(string $requestedName) : bool { $constructor = (new ReflectionClass($requestedName))->getConstructor(); diff --git a/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php b/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php index eea824f5..45cc8096 100644 --- a/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php +++ b/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php @@ -1,7 +1,7 @@ assertFalse($factory->canCreate($this->container->reveal(), $requestedName)); } - /** - * @throws \ReflectionException - */ - public function testCanCreateReturnsFalseWhenConstructorIsPrivate() : void + public function testCanCreateReturnsFalseWhenConstructorIsPrivate() { self::assertFalse( (new ReflectionBasedAbstractFactory())->canCreate( From 287455d1df78ed2b53cc1c5b3ef31720bc3214ec Mon Sep 17 00:00:00 2001 From: James Titcumb Date: Wed, 9 May 2018 17:55:32 +0100 Subject: [PATCH 3/6] Additional test case for canCreate when no constructor for class --- .../ReflectionBasedAbstractFactory.php | 6 +----- .../ReflectionBasedAbstractFactoryTest.php | 11 +++++++++++ 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/AbstractFactory/ReflectionBasedAbstractFactory.php b/src/AbstractFactory/ReflectionBasedAbstractFactory.php index 34d45172..4fd5c4c8 100644 --- a/src/AbstractFactory/ReflectionBasedAbstractFactory.php +++ b/src/AbstractFactory/ReflectionBasedAbstractFactory.php @@ -141,11 +141,7 @@ private function canCallConstructor(string $requestedName) : bool { $constructor = (new ReflectionClass($requestedName))->getConstructor(); - if ($constructor === null) { - return true; - } - - return $constructor->isPublic(); + return $constructor === null || $constructor->isPublic(); } /** diff --git a/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php b/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php index 45cc8096..52760fe9 100644 --- a/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php +++ b/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php @@ -51,6 +51,17 @@ public function testCanCreateReturnsFalseWhenConstructorIsPrivate() ); } + public function testCanCreateReturnsTrueWhenClassHasNoConstructor() : void + { + self::assertTrue( + (new ReflectionBasedAbstractFactory())->canCreate( + $this->container->reveal(), + TestAsset\ClassWithNoConstructor::class + ), + 'ReflectionBasedAbstractFactory should be able to instantiate a class without a constructor' + ); + } + public function testFactoryInstantiatesClassDirectlyIfItHasNoConstructor() { $factory = new ReflectionBasedAbstractFactory(); From 734eeb11a609e80e02eaf82af18e57baa83fe9f9 Mon Sep 17 00:00:00 2001 From: Aleksei Khudiakov Date: Thu, 20 Dec 2018 18:33:18 +1000 Subject: [PATCH 4/6] Update file level dockblocks for changed files --- src/AbstractFactory/ReflectionBasedAbstractFactory.php | 4 ++-- test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php | 4 ++-- .../AbstractFactory/TestAsset/ClassWithPrivateConstructor.php | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/AbstractFactory/ReflectionBasedAbstractFactory.php b/src/AbstractFactory/ReflectionBasedAbstractFactory.php index 4fd5c4c8..7f4ec413 100644 --- a/src/AbstractFactory/ReflectionBasedAbstractFactory.php +++ b/src/AbstractFactory/ReflectionBasedAbstractFactory.php @@ -1,8 +1,8 @@ Date: Thu, 20 Dec 2018 18:37:15 +1000 Subject: [PATCH 5/6] Add CHANGELOG for #268 --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce090458..7a4c01ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,9 @@ All notable changes to this project will be documented in this file, in reverse ### Fixed -- Nothing. +- [#268](https://github.com/zendframework/zend-servicemanager/pull/268) Fixes + ReflectionBasedAbstractFactory trying to instantiate classes with private + constructors ## 3.3.2 - 2018-01-29 From 8d83c416304d40a6e93c704bed490bcf8644d9fd Mon Sep 17 00:00:00 2001 From: Aleksei Khudiakov Date: Thu, 20 Dec 2018 18:42:31 +1000 Subject: [PATCH 6/6] Drop return typehints for 5.6 --- src/AbstractFactory/ReflectionBasedAbstractFactory.php | 2 +- test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/AbstractFactory/ReflectionBasedAbstractFactory.php b/src/AbstractFactory/ReflectionBasedAbstractFactory.php index 7f4ec413..33fab12f 100644 --- a/src/AbstractFactory/ReflectionBasedAbstractFactory.php +++ b/src/AbstractFactory/ReflectionBasedAbstractFactory.php @@ -137,7 +137,7 @@ public function canCreate(ContainerInterface $container, $requestedName) return class_exists($requestedName) && $this->canCallConstructor($requestedName); } - private function canCallConstructor(string $requestedName) : bool + private function canCallConstructor($requestedName) { $constructor = (new ReflectionClass($requestedName))->getConstructor(); diff --git a/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php b/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php index 77c41faa..381cc166 100644 --- a/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php +++ b/test/AbstractFactory/ReflectionBasedAbstractFactoryTest.php @@ -51,7 +51,7 @@ public function testCanCreateReturnsFalseWhenConstructorIsPrivate() ); } - public function testCanCreateReturnsTrueWhenClassHasNoConstructor() : void + public function testCanCreateReturnsTrueWhenClassHasNoConstructor() { self::assertTrue( (new ReflectionBasedAbstractFactory())->canCreate(