From a848eec6afde751b6a1e2df8708991aaefa60a77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maximilian=20B=C3=B6sing?= Date: Fri, 27 Oct 2017 08:42:10 +0200 Subject: [PATCH 1/4] `AbstractPluginManagerTest` is failing if an own factory is being used instead of `InvokableFactory` with `creationOptions` --- test/AbstractPluginManagerTest.php | 15 ++++++++++++++ test/TestAsset/BazFactory.php | 33 ++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 test/TestAsset/BazFactory.php diff --git a/test/AbstractPluginManagerTest.php b/test/AbstractPluginManagerTest.php index 6bafeb15..62ab3f96 100644 --- a/test/AbstractPluginManagerTest.php +++ b/test/AbstractPluginManagerTest.php @@ -19,6 +19,7 @@ use Zend\ServiceManager\Factory\InvokableFactory; use Zend\ServiceManager\ServiceManager; use ZendTest\ServiceManager\TestAsset\Baz; +use ZendTest\ServiceManager\TestAsset\BazFactory; use ZendTest\ServiceManager\TestAsset\FooPluginManager; use ZendTest\ServiceManager\TestAsset\InvokableObject; use ZendTest\ServiceManager\TestAsset\MockSelfReturningDelegatorFactory; @@ -167,6 +168,20 @@ public function testInvokableFactoryNoOptionsResetsCreationOptions() $this->assertNull($plugin2->options); } + public function testAnyFactoryNoOptionsResetsCreationOptions() + { + /** @var $pluginManager AbstractPluginManager */ + $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager'); + $pluginManager->setFactory(Baz::class, BazFactory::class); + $pluginManager->setShared(Baz::class, false); + $creationOptions = ['key1' => 'value1']; + $plugin1 = $pluginManager->get(Baz::class, $creationOptions); + $plugin2 = $pluginManager->get(Baz::class); + + $this->assertSame($creationOptions, $plugin1->options); + $this->assertNull($plugin2->options); + } + public function testValidatePluginIsCalledWithDelegatorFactoryIfItsAService() { $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager'); diff --git a/test/TestAsset/BazFactory.php b/test/TestAsset/BazFactory.php new file mode 100644 index 00000000..f9ce3093 --- /dev/null +++ b/test/TestAsset/BazFactory.php @@ -0,0 +1,33 @@ + + */ +class BazFactory implements FactoryInterface +{ + + /** + * @var array + */ + private $creationOptions; + + public function createService(ServiceLocatorInterface $serviceLocator) + { + return new Baz($this->creationOptions); + } + + /** + * @param array $creationOptions + * + * @return void + */ + public function setCreationOptions(array $creationOptions) + { + $this->creationOptions = $creationOptions; + } +} From 0c11fa0e8b11a0cdb73e519ae3c86c82d8c21be2 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Mon, 27 Nov 2017 16:01:13 -0600 Subject: [PATCH 2/4] Ensure instances created with options are never shared The issue revealed in #205 unveiled a few issues: - creation options were not being reset in factories when the same service was requested again without options. - services created with options were being cached, when they should not be. I added several tests to cover the different aberrant behaviors, and then fixed the issue. The fix required a few changes: - If non-empty options are passed to `get()`, we check to see if a shared instance already exists. If so, we cache the shared instance and remove its instance from the container state in order to force creation of a new instance. Once done, we reset the container state to store the original shared instance. - If non-empty options are passed, the instance generated will now never be shared. - When `createServiceViaCallback()` is called, we do not check for non-empty creation options if the `setCreationOptions()` method exists; we always pass them. Further, we always cast non-array creation options to an empty array. --- src/AbstractPluginManager.php | 42 +++++++++++- test/AbstractPluginManagerTest.php | 64 +++++++++++++++++-- ...ry.php => FactoryUsingCreationOptions.php} | 11 ++-- 3 files changed, 102 insertions(+), 15 deletions(-) rename test/TestAsset/{BazFactory.php => FactoryUsingCreationOptions.php} (61%) diff --git a/src/AbstractPluginManager.php b/src/AbstractPluginManager.php index c3a5663f..89cf4dd9 100644 --- a/src/AbstractPluginManager.php +++ b/src/AbstractPluginManager.php @@ -147,6 +147,8 @@ abstract public function validatePlugin($plugin); public function get($name, $options = [], $usePeeringServiceManagers = true) { $isAutoInvokable = false; + $cName = null; + $sharedInstance = null; // Allow specifying a class name directly; registers as an invokable class if (!$this->has($name) && $this->autoAddInvokableClass && class_exists($name)) { @@ -157,22 +159,58 @@ public function get($name, $options = [], $usePeeringServiceManagers = true) $this->creationOptions = $options; + // If creation options were provided, we want to force creation of a + // new instance. + if (! empty($this->creationOptions)) { + $cName = isset($this->canonicalNames[$name]) + ? $this->canonicalNames[$name] + : $this->canonicalizeName($name); + + if (isset($this->instances[$cName])) { + $sharedInstance = $this->instances[$cName]; + unset($this->instances[$cName]); + } + } + try { $instance = parent::get($name, $usePeeringServiceManagers); } catch (Exception\ServiceNotFoundException $exception) { + if ($sharedInstance) { + $this->instances[$cName] = $sharedInstance; + } + $this->creationOptions = null; $this->tryThrowingServiceLocatorUsageException($name, $isAutoInvokable, $exception); } catch (Exception\ServiceNotCreatedException $exception) { + if ($sharedInstance) { + $this->instances[$cName] = $sharedInstance; + } + $this->creationOptions = null; $this->tryThrowingServiceLocatorUsageException($name, $isAutoInvokable, $exception); } $this->creationOptions = null; + // If we had a previously shared instance, restore it. + if ($sharedInstance) { + $this->instances[$cName] = $sharedInstance; + } + try { $this->validatePlugin($instance); } catch (Exception\RuntimeException $exception) { $this->tryThrowingServiceLocatorUsageException($name, $isAutoInvokable, $exception); } + // If we created a new instance using creation options, and it was + // marked to share, we remove the shared instance + // (options === cannot share) + if ($cName + && isset($this->instances[$cName]) + && $this->instances[$cName] === $instance + ) { + unset($this->instances[$cName]); + } + return $instance; } @@ -321,10 +359,8 @@ protected function createServiceViaCallback($callable, $cName, $rName) // duck-type MutableCreationOptionsInterface for forward compatibility if (isset($factory) && method_exists($factory, 'setCreationOptions') - && is_array($this->creationOptions) - && !empty($this->creationOptions) ) { - $factory->setCreationOptions($this->creationOptions); + $factory->setCreationOptions(is_array($this->creationOptions) ? $this->creationOptions : []); } elseif ($factory instanceof Factory\InvokableFactory) { $factory->setCreationOptions(null); } diff --git a/test/AbstractPluginManagerTest.php b/test/AbstractPluginManagerTest.php index 62ab3f96..c8b145f8 100644 --- a/test/AbstractPluginManagerTest.php +++ b/test/AbstractPluginManagerTest.php @@ -19,7 +19,7 @@ use Zend\ServiceManager\Factory\InvokableFactory; use Zend\ServiceManager\ServiceManager; use ZendTest\ServiceManager\TestAsset\Baz; -use ZendTest\ServiceManager\TestAsset\BazFactory; +use ZendTest\ServiceManager\TestAsset\FactoryUsingCreationOptions; use ZendTest\ServiceManager\TestAsset\FooPluginManager; use ZendTest\ServiceManager\TestAsset\InvokableObject; use ZendTest\ServiceManager\TestAsset\MockSelfReturningDelegatorFactory; @@ -111,8 +111,9 @@ public function testMutableMethodNeverCalledWithoutCreationOptions() { $mock = 'ZendTest\ServiceManager\TestAsset\CallableWithMutableCreationOptions'; $callable = $this->getMock($mock, ['setCreationOptions']); - $callable->expects($this->never()) - ->method('setCreationOptions'); + $callable->expects($this->once()) + ->method('setCreationOptions') + ->with([]); $ref = new ReflectionObject($this->pluginManager); @@ -165,21 +166,70 @@ public function testInvokableFactoryNoOptionsResetsCreationOptions() $plugin2 = $pluginManager->get(Baz::class); $this->assertSame($creationOptions, $plugin1->options); - $this->assertNull($plugin2->options); + $this->assertEmpty($plugin2->options); + } + + /** + * @group 205 + */ + public function testRetrievingServicesViaFactoryThatUsesCreationOptionsShouldNotRememberServiceOnSubsequentRetrievals() + { + /** @var $pluginManager AbstractPluginManager */ + $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager'); + $pluginManager->setFactory(Baz::class, FactoryUsingCreationOptions::class); + $pluginManager->setShared(Baz::class, false); + $creationOptions = ['key1' => 'value1']; + $plugin1 = $pluginManager->get(Baz::class, $creationOptions); + $plugin2 = $pluginManager->get(Baz::class); + + $this->assertNotSame($plugin1, $plugin2); + + $this->assertSame($creationOptions, $plugin1->options); + $this->assertEmpty($plugin2->options); } - public function testAnyFactoryNoOptionsResetsCreationOptions() + /** + * @group 205 + */ + public function testRetrievingServicesViaFactoryThatUsesCreationOptionsShouldReturnNewInstanceIfOptionsAreProvided() { /** @var $pluginManager AbstractPluginManager */ $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager'); - $pluginManager->setFactory(Baz::class, BazFactory::class); + $pluginManager->setFactory(Baz::class, FactoryUsingCreationOptions::class); + $pluginManager->setShared(Baz::class, false); + $creationOptions = ['key1' => 'value1']; + $plugin1 = $pluginManager->get(Baz::class); + $plugin2 = $pluginManager->get(Baz::class, $creationOptions); + + $this->assertNotSame($plugin1, $plugin2); + + $this->assertEmpty($plugin1->options); + $this->assertSame($creationOptions, $plugin2->options); + } + + /** + * @group 205 + */ + // @codingStandardsIgnoreStart + public function testRetrievingServicesViaFactoryThatUsesCreationOptionsShouldReturnNewInstanceEveryTimeOptionsAreProvided() + { + // @codingStandardsIgnoreEnd + /** @var $pluginManager AbstractPluginManager */ + $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager'); + $pluginManager->setFactory(Baz::class, FactoryUsingCreationOptions::class); $pluginManager->setShared(Baz::class, false); $creationOptions = ['key1' => 'value1']; $plugin1 = $pluginManager->get(Baz::class, $creationOptions); $plugin2 = $pluginManager->get(Baz::class); + $plugin3 = $pluginManager->get(Baz::class, $creationOptions); + + $this->assertNotSame($plugin1, $plugin2); + $this->assertNotSame($plugin1, $plugin3); + $this->assertNotSame($plugin2, $plugin3); $this->assertSame($creationOptions, $plugin1->options); - $this->assertNull($plugin2->options); + $this->assertEmpty($plugin2->options); + $this->assertSame($creationOptions, $plugin3->options); } public function testValidatePluginIsCalledWithDelegatorFactoryIfItsAService() diff --git a/test/TestAsset/BazFactory.php b/test/TestAsset/FactoryUsingCreationOptions.php similarity index 61% rename from test/TestAsset/BazFactory.php rename to test/TestAsset/FactoryUsingCreationOptions.php index f9ce3093..2a935da0 100644 --- a/test/TestAsset/BazFactory.php +++ b/test/TestAsset/FactoryUsingCreationOptions.php @@ -1,16 +1,17 @@ - */ -class BazFactory implements FactoryInterface +class FactoryUsingCreationOptions implements FactoryInterface { - /** * @var array */ From 93c47600704161699d9927bef2696c78069ea495 Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Mon, 27 Nov 2017 16:14:35 -0600 Subject: [PATCH 3/4] Do not ignore platform requirements during installation - zendframework/zend-code 3.0 is PHP 7.1+ only, and makes tests on this release branch fail due to syntax issues. - doctrine/instantiator 1.1.0 is PHP 7.1+ only, and makes tests on this release branch fail due to syntax issues. --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 152da12c..82b69a7b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -35,7 +35,7 @@ before_install: - if [[ $EXECUTE_TEST_COVERALLS == 'true' ]]; then composer require --dev --no-update satooshi/php-coveralls ; fi install: - - COMPOSER_ROOT_VERSION=2.7.99 travis_retry composer install --no-interaction --ignore-platform-reqs + - COMPOSER_ROOT_VERSION=2.7.99 travis_retry composer install --no-interaction script: - if [[ $EXECUTE_TEST_COVERALLS == 'true' ]]; then ./vendor/bin/phpunit --coverage-clover clover.xml ; fi From 8ec53caee5e3eb4b99f0e23deb2c0d27ea82454e Mon Sep 17 00:00:00 2001 From: Matthew Weier O'Phinney Date: Mon, 27 Nov 2017 16:34:07 -0600 Subject: [PATCH 4/4] Adds CHANGELOG entry for #205 --- CHANGELOG.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a867c45..8910e6c2 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.9 - TBD +## 2.7.9 - 2017-11-27 ### Added @@ -18,7 +18,12 @@ All notable changes to this project will be documented in this file, in reverse ### Fixed -- Nothing. +- [#205](https://github.com/zendframework/zend-servicemanager/pull/205) fixes + how the `AbstractPluginManager` handles repeated retrievals of the same + service when instance options are provided and the service is marked as + "shared". Previously, it incorrectly would return the first instance + retrieved; with this release, no instance created with instance options is + ever shared. ## 2.7.8 - 2016-12-19