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 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 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 6bafeb15..c8b145f8 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\FactoryUsingCreationOptions; use ZendTest\ServiceManager\TestAsset\FooPluginManager; use ZendTest\ServiceManager\TestAsset\InvokableObject; use ZendTest\ServiceManager\TestAsset\MockSelfReturningDelegatorFactory; @@ -110,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); @@ -164,7 +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); + } + + /** + * @group 205 + */ + public function testRetrievingServicesViaFactoryThatUsesCreationOptionsShouldReturnNewInstanceIfOptionsAreProvided() + { + /** @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); + $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->assertEmpty($plugin2->options); + $this->assertSame($creationOptions, $plugin3->options); } public function testValidatePluginIsCalledWithDelegatorFactoryIfItsAService() diff --git a/test/TestAsset/FactoryUsingCreationOptions.php b/test/TestAsset/FactoryUsingCreationOptions.php new file mode 100644 index 00000000..2a935da0 --- /dev/null +++ b/test/TestAsset/FactoryUsingCreationOptions.php @@ -0,0 +1,34 @@ +creationOptions); + } + + /** + * @param array $creationOptions + * + * @return void + */ + public function setCreationOptions(array $creationOptions) + { + $this->creationOptions = $creationOptions; + } +}