Merge branch 'hotfix/205-creation-options' into release-2.7

Close #205

Author: weierophinney

.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

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
 

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);
         }

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()

test/TestAsset/FactoryUsingCreationOptions.php

@@ -0,0 +1,34 @@
+<?php
+/**
+ * @see       https://github.com/zendframework/zend-servicemanager for the canonical source repository
+ * @copyright Copyright (c) 2017 Zend Technologies USA Inc. (https://www.zend.com)
+ * @license   https://github.com/zendframework/zend-servicemanager/blob/master/LICENSE.md New BSD License
+ */
+
+namespace ZendTest\ServiceManager\TestAsset;
+
+use Zend\ServiceManager\FactoryInterface;
+use Zend\ServiceManager\ServiceLocatorInterface;
+
+class FactoryUsingCreationOptions 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;
+    }
+}