fix delegators to allow usage in plugin managers #5175

Closed
wants to merge 11 commits into
from

Projects

None yet

3 participants

@stefanotorresi

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
@Ocramius Ocramius commented on the diff Sep 26, 2013
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)
@Ocramius
Ocramius Sep 26, 2013

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

@stefanotorresi
stefanotorresi Sep 27, 2013

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?

@Ocramius
Ocramius Sep 30, 2013

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

@Ocramius Ocramius commented on an outdated diff Sep 26, 2013
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');
@Ocramius
Ocramius Sep 26, 2013

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

@Ocramius Ocramius commented on an outdated diff Sep 26, 2013
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');
@Ocramius
Ocramius Sep 26, 2013

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

@Ocramius Ocramius commented on an outdated diff Sep 26, 2013
library/Zend/ServiceManager/ServiceManager.php
+ * @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)) {
@Ocramius
Ocramius Sep 26, 2013

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.

@Ocramius
Ocramius Sep 26, 2013

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

@Ocramius
Zend Framework member

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);
@stefanotorresi

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

@weierophinney weierophinney added a commit that closed this pull request Oct 23, 2013
@weierophinney weierophinney Merge branch 'hotfix/5175'
Close #5175
Fixes #5172
a7a4a8e
@weierophinney weierophinney added a commit that referenced this pull request Oct 23, 2013
@weierophinney weierophinney Merge branch 'hotfix/5175' into develop
Forward port #5175
ec33e0f
@weierophinney weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#5175 from stefanotorre…
…si/fix/pm-delegators

fix delegators to allow usage in plugin managers

Conflicts:
	tests/ZendTest/ServiceManager/ServiceManagerTest.php
e27390d
@weierophinney weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/5175' 9650994
@weierophinney weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'hotfix/5175' into develop f8d5651
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment