Skip to content
This repository has been archived by the owner on Feb 6, 2020. It is now read-only.

Hotfix #279 #280

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 29 additions & 2 deletions src/ServiceManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use Zend\ServiceManager\Exception\InvalidArgumentException;
use Zend\ServiceManager\Exception\ServiceNotCreatedException;
use Zend\ServiceManager\Exception\ServiceNotFoundException;
use Zend\Stdlib\ArrayUtils;
thomasvargiu marked this conversation as resolved.
Show resolved Hide resolved

/**
* Service Manager.
Expand Down Expand Up @@ -337,7 +338,7 @@ public function configure(array $config)
}

if (isset($config['delegators'])) {
$this->delegators = array_merge_recursive($this->delegators, $config['delegators']);
$this->mergeDelegators($config['delegators']);
}

if (isset($config['shared'])) {
Expand All @@ -357,7 +358,7 @@ public function configure(array $config)
// If lazy service configuration was provided, reset the lazy services
// delegator factory.
if (isset($config['lazy_services']) && ! empty($config['lazy_services'])) {
$this->lazyServices = array_merge_recursive($this->lazyServices, $config['lazy_services']);
$this->lazyServices = ArrayUtils::merge($this->lazyServices, $config['lazy_services']);
$this->lazyServicesDelegator = null;
}

Expand Down Expand Up @@ -830,6 +831,32 @@ private function createLazyServiceDelegatorFactory()
return $this->lazyServicesDelegator;
}

/**
* Merge delegators avoiding multiple same delegators for the same service.
* It works with strings and class instances.
* It's not possible to de-duple anonymous functions
*
* @param string[][]|Factory\DelegatorFactoryInterface[][] $config
* @return string[][]|Factory\DelegatorFactoryInterface[][]
*/
private function mergeDelegators(array $config)
{
foreach ($config as $key => $delegators) {
if (! isset($this->delegators[$key])) {
$this->delegators[$key] = $delegators;
continue;
}

foreach ($delegators as $delegator) {
if (! in_array($delegator, $this->delegators[$key], true)) {
$this->delegators[$key][] = $delegator;
}
}
}

return $this->delegators;
}

/**
* Create aliases for invokable classes.
*
Expand Down
48 changes: 48 additions & 0 deletions test/ServiceManagerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
use stdClass;
use Zend\ServiceManager\Factory\FactoryInterface;
use Zend\ServiceManager\Factory\InvokableFactory;
use Zend\ServiceManager\Proxy\LazyServiceFactory;
use Zend\ServiceManager\ServiceManager;
use ZendTest\ServiceManager\TestAsset\InvokableObject;
use ZendTest\ServiceManager\TestAsset\SimpleServiceManager;
Expand Down Expand Up @@ -283,4 +284,51 @@ public function testFactoryMayBeStaticMethodDescribedByCallableString()
$serviceManager = new SimpleServiceManager($config);
$this->assertEquals(stdClass::class, get_class($serviceManager->get(stdClass::class)));
}

/**
* Hotfix #279
* @see https://github.com/zendframework/zend-servicemanager/issues/279
*/
public function testConfigureMultipleTimes()
{
$delegatorFactory = function (
ContainerInterface $container,
$name,
callable $callback
) {
/** @var InvokableObject $instance */
$instance = $callback();
$options = $instance->getOptions();
$inc = ! empty($options['inc']) ? $options['inc'] : 0;
return new InvokableObject(['inc' => ++$inc]);
};

$config = [
'factories' => [
'Foo' => function () {
return new InvokableObject();
},
],
'delegators' => [
'Foo' => [
$delegatorFactory,
LazyServiceFactory::class,
],
],
'lazy_services' => [
'class_map' => [
'Foo' => InvokableObject::class,
],
],
];

$serviceManager = new ServiceManager($config);
$serviceManager->configure($config);

/** @var InvokableObject $instance */
$instance = $serviceManager->get('Foo');

self::assertInstanceOf(InvokableObject::class, $instance);
self::assertSame(1, $instance->getOptions()['inc']);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a single test should have only one assertion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, I think it can be a valid assertion. Instead to throw an error in case of failure I'm asserting that the second assertion depends on the first assertion. If it's enough the phpunit error catch for you, I can remove the first assertion. Different point of view :)
Someonelse opinion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to have multiple assertions as long as they are connected, imho. Kinda "one logic assertion". I was checking what people say about it, and I see some approach to write test with single assertion. I see the advantages of it, but I think this approach would be more messy. You can see that in other tests usually we have more than one assertion, but these are connected - as you are saying, @thomasvargiu.

See: https://softwareengineering.stackexchange.com/questions/7823/is-it-ok-to-have-multiple-asserts-in-a-single-unit-test

any my favourite comment there is:

A single assert per unit test is a great way to test the reader's ability to scroll up and down.

🤣

}
}