From c1a6bf35f7f913bdab8e224f9a66623db3ab93ab Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 5 Dec 2017 10:11:15 -0600 Subject: [PATCH 1/2] Ensure a `null` value is passed to `setCreationOptions` instead of empty array Per https://github.com/zendframework/zend-servicemanager/issues/159#issuecomment-349339218, the updates in #205 broke a previously fixed edge case whereby `AbstractPluginManager::createServiceViaCallback()` would inject a `null` value to a factory's `setCreationOptions()` method if no instance options were provided; it instead passed an empty array, which broke a number of plugins that relied on a null value being present. This patch does the following: - Adds a regression test for the behavior. - Modifies `AbstractPluginManager::createServiceViaCallback()` to have three distinct behaviors surrounding creation options, in the following order: - if the factory is an `InvokableFactory`, it tests if the creation options are an array or non-empty; if not, it passes a `null` value to `setCreationOptions()`. - if the factory is an `MutableCreationOptionsInterface`, it tests if the creation options are an array or non-empty; if not, it passes an empty array to `setCreationOptions()`. - if the factory implements the `setCreationOptions()` and the creation options are a non-array or non-empty, it reflects the method to determine if the options argument has a default value, using that if present, or an empty array if not. --- src/AbstractPluginManager.php | 36 ++++++++++++++++--- test/AbstractPluginManagerTest.php | 14 +++++++- .../TestAsset/FactoryUsingCreationOptions.php | 4 +-- 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/AbstractPluginManager.php b/src/AbstractPluginManager.php index 89cf4dd9..7e40a2d0 100644 --- a/src/AbstractPluginManager.php +++ b/src/AbstractPluginManager.php @@ -11,6 +11,7 @@ use Interop\Container\ContainerInterface; use Exception as BaseException; +use ReflectionMethod; /** * ServiceManager implementation for managing plugins @@ -356,13 +357,38 @@ protected function createServiceViaCallback($callable, $cName, $rName) $factory = reset($callable); } - // duck-type MutableCreationOptionsInterface for forward compatibility - if (isset($factory) + if ($factory instanceof Factory\InvokableFactory) { + // InvokableFactory::setCreationOptions has a different signature than + // MutableCreationOptionsInterface; allows null value. + $options = is_array($this->creationOptions) && ! empty($this->creationOptions) + ? $this->creationOptions + : null; + $factory->setCreationOptions($options); + } elseif ($factory instanceof MutableCreationOptionsInterface) { + // MutableCreationOptionsInterface expects an array, always; pass an + // empty array for lack of creation options. + $options = is_array($this->creationOptions) && ! empty($this->creationOptions) + ? $this->creationOptions + : []; + $factory->setCreationOptions($options); + } elseif (isset($factory) && method_exists($factory, 'setCreationOptions') ) { - $factory->setCreationOptions(is_array($this->creationOptions) ? $this->creationOptions : []); - } elseif ($factory instanceof Factory\InvokableFactory) { - $factory->setCreationOptions(null); + // duck-type MutableCreationOptionsInterface for forward compatibility + + $options = $this->creationOptions; + + // If we have empty creation options, we have to find out if a default + // value is present and use that; otherwise, we should use an empty + // array, as that's the standard type-hint. + if (! is_array($options) || empty($options)) { + $r = new ReflectionMethod($factory, 'setCreationOptions'); + $params = $r->getParameters(); + $optionsParam = array_shift($params); + $options = $optionsParam->isDefaultValueAvailable() ? $optionsParam->getDefaultValue() : []; + } + + $factory->setCreationOptions($options); } return parent::createServiceViaCallback($callable, $cName, $rName); diff --git a/test/AbstractPluginManagerTest.php b/test/AbstractPluginManagerTest.php index c8b145f8..45ba1b48 100644 --- a/test/AbstractPluginManagerTest.php +++ b/test/AbstractPluginManagerTest.php @@ -209,8 +209,8 @@ public function testRetrievingServicesViaFactoryThatUsesCreationOptionsShouldRet /** * @group 205 + * @codingStandardsIgnoreStart */ - // @codingStandardsIgnoreStart public function testRetrievingServicesViaFactoryThatUsesCreationOptionsShouldReturnNewInstanceEveryTimeOptionsAreProvided() { // @codingStandardsIgnoreEnd @@ -451,4 +451,16 @@ public function testInvokableFactoryHasMutableOptions() $object = $pluginManager->get('foo', $options); $this->assertEquals($options, $object->getOptions()); } + + public function testIfCreationOptionsAreEmptySetThemThemToNullWhenCreatingAService() + { + // @codingStandardsIgnoreEnd + /** @var $pluginManager AbstractPluginManager */ + $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager'); + $pluginManager->setFactory(Baz::class, FactoryUsingCreationOptions::class); + $pluginManager->setShared(Baz::class, false); + $plugin = $pluginManager->get(Baz::class); + + $this->assertNull($plugin->options); + } } diff --git a/test/TestAsset/FactoryUsingCreationOptions.php b/test/TestAsset/FactoryUsingCreationOptions.php index 2a935da0..9838c389 100644 --- a/test/TestAsset/FactoryUsingCreationOptions.php +++ b/test/TestAsset/FactoryUsingCreationOptions.php @@ -23,11 +23,11 @@ public function createService(ServiceLocatorInterface $serviceLocator) } /** - * @param array $creationOptions + * @param array|null $creationOptions * * @return void */ - public function setCreationOptions(array $creationOptions) + public function setCreationOptions(array $creationOptions = null) { $this->creationOptions = $creationOptions; } From f217651d85877af82e11ac2498bf4a1749420667 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Tue, 5 Dec 2017 10:27:26 -0600 Subject: [PATCH 2/2] Adds 2.7.10 CHANGELOG --- CHANGELOG.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bf55712f..1cf142c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ All notable changes to this project will be documented in this file, in reverse chronological order by release. -## 2.7.10 - TBD +## 2.7.10 - 2017-12-05 ### Added @@ -22,7 +22,10 @@ All notable changes to this project will be documented in this file, in reverse ### Fixed -- Nothing. +- [#210](https://github.com/zendframework/zend-servicemanager/pull/210) fixes a + regression whereby factories accepting creation options were receiving an + empty array versus a `null` value when no options were present for a + particular invocation; they now correctly receive a `null` value. ## 2.7.9 - 2017-11-27