Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

fix delegators to allow usage in plugin managers #5175

Closed
wants to merge 11 commits into from

3 participants

Stefano Torresi Marco Pivetta Matthew Weier O'Phinney
Stefano Torresi

see #5172

  • move delegator logic from create() to separate method
  • remove automatic invokable service for delegator factories
  • add delegator factory class validation
  • add callback support for delegator factories
  • tests
Marco Pivetta Ocramius commented on the diff
library/Zend/ServiceManager/ServiceManager.php
@@ -1046,6 +1025,53 @@ protected function createFromAbstractFactory($canonicalName, $requestedName)
}
/**
+ * @param $canonicalName
+ * @param $requestedName
+ * @return mixed
+ * @throws Exception\ServiceNotCreatedException
+ */
+ protected function createDelegatorFromFactory($canonicalName, $requestedName)
Marco Pivetta Collaborator

While this is indeed clean, consider that I inlined the code for performance reasons

hmmm but it's 40 lines of code now, plus there are separated methods for each service type. this way it's not only cleaner, it's more consistent too.

are you sure the performance gain is worth it? maybe there is even another way to improve that aspect?

Marco Pivetta Collaborator

Hrm... it's probably ok to keep the method separate then, yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/ZendTest/ServiceManager/ServiceManagerTest.php
@@ -798,10 +795,8 @@ public function testUsesMultipleDelegates()
$fooDelegator = new MockSelfReturningDelegatorFactory();
$barDelegator = new MockSelfReturningDelegatorFactory();
- $this->serviceManager->setService('foo-delegate', $fooDelegator);
- $this->serviceManager->setService('bar-delegate', $barDelegator);
- $this->serviceManager->addDelegator('foo-service', 'foo-delegate');
- $this->serviceManager->addDelegator('foo-service', 'bar-delegate');
Marco Pivetta Collaborator

You should add a new test, not modify the existing one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/ZendTest/ServiceManager/ServiceManagerTest.php
@@ -760,8 +760,7 @@ public function testUsesDelegatorWhenAvailable()
{
$delegator = $this->getMock('Zend\\ServiceManager\\DelegatorFactoryInterface');
- $this->serviceManager->setService('foo-delegator', $delegator);
- $this->serviceManager->addDelegator('foo-service', 'foo-delegator');
Marco Pivetta Collaborator

You should add a new test, not modify the existing one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
library/Zend/ServiceManager/ServiceManager.php
((6 lines not shown))
+ * @return mixed
+ * @throws Exception\ServiceNotCreatedException
+ */
+ protected function createDelegatorFromFactory($canonicalName, $requestedName)
+ {
+ $serviceManager = $this;
+ $delegatorsCount = count($this->delegators[$canonicalName]);
+ $creationCallback = function () use ($serviceManager, $requestedName, $canonicalName) {
+ return $serviceManager->doCreate($requestedName, $canonicalName);
+ };
+
+ for ($i = 0; $i < $delegatorsCount; $i += 1) {
+
+ $delegatorFactory = $this->delegators[$canonicalName][$i];
+
+ if (is_string($delegatorFactory) && class_exists($delegatorFactory, true)) {
Marco Pivetta Collaborator

This breaks BC. Delegators that are defined as services won't work anymore. While that's the required behavior, we can't break BC here.

Marco Pivetta Collaborator

You should therefore check $this->has($delegatorFactory) first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Marco Pivetta
Collaborator

on add callback support for DelegatorFactories - are you sure you want callback support there?

Should be trivial though:

  1. change the check on

    if (!$delegatorFactory instanceof DelegatorFactoryInterface) {

    to

    if (!$delegatorFactory instanceof DelegatorFactoryInterface && !is_callable($delegatorFactory)) {
  2. change

    return $delegatorFactory->createDelegatorWithName($serviceManager, $cName, $rName, $creationCallback);

    to

    return $delegatorFactory instanceof DelegatorFactoryInterface 
        ? $delegatorFactory->createDelegatorWithName($serviceManager, $cName, $rName, $creationCallback)
        : $delegatorFactory($serviceManager, $cName, $rName, $creationCallback);
Stefano Torresi

yep, why not, this way we can keep delegator usage in line with the other service types :)

Matthew Weier O'Phinney weierophinney closed this pull request from a commit
Matthew Weier O'Phinney weierophinney Merge branch 'hotfix/5175'
Close #5175
Fixes #5172
a7a4a8e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 18, 2013
  1. Stefano Torresi

    add new method for delegators

    stefanotorresi authored
    done:
    * move delegator logic from create() to separate method
    * remove automatic invokable service for delegator factories
    * add delegator factory class validation
    * update tests
  2. Stefano Torresi
  3. Stefano Torresi

    fix BC

    stefanotorresi authored
  4. Stefano Torresi
  5. Stefano Torresi
  6. Stefano Torresi

    test cleanup

    stefanotorresi authored
  7. Stefano Torresi
  8. Stefano Torresi
  9. Stefano Torresi

    fix callback test

    stefanotorresi authored
  10. Stefano Torresi
  11. Stefano Torresi

    code cleaning

    stefanotorresi authored
This page is out of date. Refresh to see the latest.
83 library/Zend/ServiceManager/ServiceManager.php
View
@@ -532,25 +532,7 @@ public function create($name)
}
if (isset($this->delegators[$cName])) {
- $serviceManager = $this;
- $additionalDelegators = count($this->delegators[$cName]) - 1;
- $creationCallback = function () use ($serviceManager, $rName, $cName) {
- return $serviceManager->doCreate($rName, $cName);
- };
-
- for ($i = 0; $i < $additionalDelegators; $i += 1) {
- $creationCallback = $this->createDelegatorCallback(
- $this->delegators[$cName][$i],
- $rName,
- $cName,
- $creationCallback
- );
- }
-
- /* @var $delegatorFactory DelegatorFactoryInterface */
- $delegatorFactory = $this->get($this->delegators[$cName][$i]);
-
- return $delegatorFactory->createDelegatorWithName($this, $cName, $rName, $creationCallback);
+ return $this->createDelegatorFromFactory($cName, $rName);
}
return $this->doCreate($rName, $cName);
@@ -559,22 +541,21 @@ public function create($name)
/**
* Creates a callback that uses a delegator to create a service
*
- * @param string $delegatorFactoryName name of the delegator factory service
- * @param string $rName requested service name
- * @param string $cName canonical service name
- * @param callable $creationCallback callback that is responsible for instantiating the service
+ * @param DelegatorFactoryInterface|callable $delegatorFactory the delegator factory
+ * @param string $rName requested service name
+ * @param string $cName canonical service name
+ * @param callable $creationCallback callback for instantiating the real service
*
* @return callable
*/
- private function createDelegatorCallback($delegatorFactoryName, $rName, $cName, $creationCallback)
+ private function createDelegatorCallback($delegatorFactory, $rName, $cName, $creationCallback)
{
$serviceManager = $this;
- return function () use ($serviceManager, $delegatorFactoryName, $rName, $cName, $creationCallback) {
- /* @var $delegatorFactory DelegatorFactoryInterface */
- $delegatorFactory = $serviceManager->get($delegatorFactoryName);
-
- return $delegatorFactory->createDelegatorWithName($serviceManager, $cName, $rName, $creationCallback);
+ return function () use ($serviceManager, $delegatorFactory, $rName, $cName, $creationCallback) {
+ return $delegatorFactory instanceof DelegatorFactoryInterface
+ ? $delegatorFactory->createDelegatorWithName($serviceManager, $cName, $rName, $creationCallback)
+ : $delegatorFactory($serviceManager, $cName, $rName, $creationCallback);
};
}
@@ -1046,6 +1027,50 @@ protected function createFromAbstractFactory($canonicalName, $requestedName)
}
/**
+ * @param $canonicalName
+ * @param $requestedName
+ * @return mixed
+ * @throws Exception\ServiceNotCreatedException
+ */
+ protected function createDelegatorFromFactory($canonicalName, $requestedName)
Marco Pivetta Collaborator

While this is indeed clean, consider that I inlined the code for performance reasons

hmmm but it's 40 lines of code now, plus there are separated methods for each service type. this way it's not only cleaner, it's more consistent too.

are you sure the performance gain is worth it? maybe there is even another way to improve that aspect?

Marco Pivetta Collaborator

Hrm... it's probably ok to keep the method separate then, yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ $serviceManager = $this;
+ $delegatorsCount = count($this->delegators[$canonicalName]);
+ $creationCallback = function () use ($serviceManager, $requestedName, $canonicalName) {
+ return $serviceManager->doCreate($requestedName, $canonicalName);
+ };
+
+ for ($i = 0; $i < $delegatorsCount; $i += 1) {
+
+ $delegatorFactory = $this->delegators[$canonicalName][$i];
+
+ if (is_string($delegatorFactory)) {
+ $delegatorFactory = !$this->has($delegatorFactory) && class_exists($delegatorFactory, true) ?
+ new $delegatorFactory
+ : $this->get($delegatorFactory);
+ $this->delegators[$canonicalName][$i] = $delegatorFactory;
+ }
+
+ if (!$delegatorFactory instanceof DelegatorFactoryInterface && !is_callable($delegatorFactory)) {
+ throw new Exception\ServiceNotCreatedException(sprintf(
+ 'While attempting to create %s%s an invalid factory was registered for this instance type.',
+ $canonicalName,
+ ($requestedName ? '(alias: ' . $requestedName . ')' : '')
+ ));
+ }
+
+ $creationCallback = $this->createDelegatorCallback(
+ $delegatorFactory,
+ $requestedName,
+ $canonicalName,
+ $creationCallback
+ );
+ }
+
+ return $creationCallback($serviceManager, $canonicalName, $requestedName, $creationCallback);
+ }
+
+ /**
* Checks if the object has this class as one of its parents
*
* @see https://bugs.php.net/bug.php?id=53727
75 tests/ZendTest/ServiceManager/AbstractPluginManagerTest.php
View
@@ -17,6 +17,7 @@
use ZendTest\ServiceManager\TestAsset\FooCounterAbstractFactory;
use ZendTest\ServiceManager\TestAsset\FooPluginManager;
+use ZendTest\ServiceManager\TestAsset\MockSelfReturningDelegatorFactory;
class AbstractPluginManagerTest extends \PHPUnit_Framework_TestCase
{
@@ -117,4 +118,78 @@ public function testCallableObjectWithMutableCreationOptions()
$method->setAccessible(true);
$method->invoke($this->pluginManager, $callable, 'foo', 'bar');
}
+
+ public function testValidatePluginIsCalledWithDelegatorFactoryIfItsAService()
+ {
+ $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager');
+ $delegatorFactory = $this->getMock('Zend\\ServiceManager\\DelegatorFactoryInterface');
+
+ $pluginManager->setService('delegator-factory', $delegatorFactory);
+ $pluginManager->addDelegator('foo-service', 'delegator-factory');
+
+ $pluginManager->expects($this->once())
+ ->method('validatePlugin')
+ ->with($delegatorFactory);
+
+ $pluginManager->create('foo-service');
+ }
+
+ public function testSingleDelegatorUsage()
+ {
+ $delegatorFactory = $this->getMock('Zend\\ServiceManager\\DelegatorFactoryInterface');
+ $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager');
+ $realService = $this->getMock('stdClass', array(), array(), 'RealService');
+ $delegator = $this->getMock('stdClass', array(), array(), 'Delegator');
+
+ $delegatorFactory
+ ->expects($this->once())
+ ->method('createDelegatorWithName')
+ ->with(
+ $pluginManager,
+ 'fooservice',
+ 'foo-service',
+ $this->callback(function ($callback) use ($realService) {
+ if (!is_callable($callback)) {
+ return false;
+ }
+
+ return call_user_func($callback) === $realService;
+ })
+ )
+ ->will($this->returnValue($delegator));
+
+ $pluginManager->setFactory('foo-service', function() use ($realService) {
+ return $realService;
+ });
+ $pluginManager->addDelegator('foo-service', $delegatorFactory);
+
+ $pluginManager->expects($this->once())
+ ->method('validatePlugin')
+ ->with($delegator);
+
+ $this->assertSame($delegator, $pluginManager->get('foo-service'));
+ }
+
+ public function testMultipleDelegatorsUsage()
+ {
+ $pluginManager = $this->getMockForAbstractClass('Zend\ServiceManager\AbstractPluginManager');
+
+ $fooDelegator = new MockSelfReturningDelegatorFactory();
+ $barDelegator = new MockSelfReturningDelegatorFactory();
+
+ $pluginManager->addDelegator('foo-service', $fooDelegator);
+ $pluginManager->addDelegator('foo-service', $barDelegator);
+ $pluginManager->setInvokableClass('foo-service', 'stdClass');
+
+ $pluginManager->expects($this->once())
+ ->method('validatePlugin')
+ ->with($barDelegator);
+
+ $this->assertSame($barDelegator, $pluginManager->get('foo-service'));
+ $this->assertCount(1, $barDelegator->instances);
+ $this->assertCount(1, $fooDelegator->instances);
+ $this->assertInstanceOf('stdClass', array_shift($fooDelegator->instances));
+ $this->assertSame($fooDelegator, array_shift($barDelegator->instances));
+
+ }
}
106 tests/ZendTest/ServiceManager/ServiceManagerTest.php
View
@@ -753,6 +753,7 @@ public function testRetrieveServiceFromPeeringServiceManagerIfretrieveFromPeerin
/**
* @covers Zend\ServiceManager\ServiceManager::create
+ * @covers Zend\ServiceManager\ServiceManager::createDelegatorFromFactory
* @covers Zend\ServiceManager\ServiceManager::createDelegatorCallback
* @covers Zend\ServiceManager\ServiceManager::addDelegator
*/
@@ -783,13 +784,12 @@ public function testUsesDelegatorWhenAvailable()
)
->will($this->returnValue($delegator));
- //die(var_dump($this->serviceManager));
-
$this->assertSame($delegator, $this->serviceManager->create('foo-service'));
}
/**
* @covers Zend\ServiceManager\ServiceManager::create
+ * @covers Zend\ServiceManager\ServiceManager::createDelegatorFromFactory
* @covers Zend\ServiceManager\ServiceManager::createDelegatorCallback
* @covers Zend\ServiceManager\ServiceManager::addDelegator
*/
@@ -810,4 +810,106 @@ public function testUsesMultipleDelegates()
$this->assertInstanceOf('stdClass', array_shift($fooDelegator->instances));
$this->assertSame($fooDelegator, array_shift($barDelegator->instances));
}
+
+ /**
+ * @covers Zend\ServiceManager\ServiceManager::createDelegatorFromFactory
+ */
+ public function testDelegatorFactoryWhenNotRegisteredAsService()
+ {
+ $delegator = $this->getMock('Zend\\ServiceManager\\DelegatorFactoryInterface');
+
+ $this->serviceManager->addDelegator('foo-service', $delegator);
+ $this->serviceManager->setInvokableClass('foo-service', 'stdClass');
+
+ $delegator
+ ->expects($this->once())
+ ->method('createDelegatorWithName')
+ ->with(
+ $this->serviceManager,
+ 'fooservice',
+ 'foo-service',
+ $this->callback(function ($callback) {
+ if (!is_callable($callback)) {
+ return false;
+ }
+
+ $service = call_user_func($callback);
+
+ return $service instanceof \stdClass;
+ })
+ )
+ ->will($this->returnValue($delegator));
+
+ $this->assertSame($delegator, $this->serviceManager->create('foo-service'));
+ }
+
+ /**
+ * @covers Zend\ServiceManager\ServiceManager::create
+ * @covers Zend\ServiceManager\ServiceManager::createDelegatorFromFactory
+ * @covers Zend\ServiceManager\ServiceManager::createDelegatorCallback
+ * @covers Zend\ServiceManager\ServiceManager::addDelegator
+ */
+ public function testMultipleDelegatorFactoriesWhenNotRegisteredAsServices()
+ {
+ $fooDelegator = new MockSelfReturningDelegatorFactory();
+ $barDelegator = new MockSelfReturningDelegatorFactory();
+
+ $this->serviceManager->addDelegator('foo-service', $fooDelegator);
+ $this->serviceManager->addDelegator('foo-service', $barDelegator);
+ $this->serviceManager->setInvokableClass('foo-service', 'stdClass');
+
+ $this->assertSame($barDelegator, $this->serviceManager->create('foo-service'));
+ $this->assertCount(1, $barDelegator->instances);
+ $this->assertCount(1, $fooDelegator->instances);
+ $this->assertInstanceOf('stdClass', array_shift($fooDelegator->instances));
+ $this->assertSame($fooDelegator, array_shift($barDelegator->instances));
+ }
+
+ public function testInvalidDelegatorFactoryThrowsException()
+ {
+ $delegatorFactory = new \stdClass;
+ $this->serviceManager->addDelegator('foo-service', $delegatorFactory);
+
+ try {
+ $this->serviceManager->create('foo-service');
+ $this->fail('Expected exception was not raised');
+ }catch (Exception\ServiceNotCreatedException $expected) {
+ $this->assertRegExp('/invalid factory/', $expected->getMessage());
+ return;
+ }
+ }
+
+ public function testInvalidDelegatorFactoryAmongMultipleOnesThrowsException()
+ {
+ $this->serviceManager->addDelegator('foo-service', new MockSelfReturningDelegatorFactory());
+ $this->serviceManager->addDelegator('foo-service', new MockSelfReturningDelegatorFactory());
+ $this->serviceManager->addDelegator('foo-service', 'stdClass');
+
+ try {
+ $this->serviceManager->create('foo-service');
+ $this->fail('Expected exception was not raised');
+ }catch (Exception\ServiceNotCreatedException $expected) {
+ $this->assertRegExp('/invalid factory/', $expected->getMessage());
+ return;
+ }
+ }
+
+ public function testDelegatorFromCallback()
+ {
+ $realService = $this->getMock('stdClass', array(), array(), 'RealService');
+ $delegator = $this->getMock('stdClass', array(), array(), 'Delegator');
+
+ $delegatorFactoryCallback = function($serviceManager, $cName, $rName, $callback) use ($delegator) {
+ $delegator->real = call_user_func($callback);
+ return $delegator;
+ };
+
+ $this->serviceManager->setFactory('foo-service', function() use ($realService) { return $realService; } );
+ $this->serviceManager->addDelegator('foo-service', $delegatorFactoryCallback);
+
+ $service = $this->serviceManager->create('foo-service');
+
+ $this->assertSame($delegator, $service);
+ $this->assertSame($realService, $service->real);
+ }
}
Something went wrong with that request. Please try again.