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

Commit

Permalink
Alias resolution fixes
Browse files Browse the repository at this point in the history 
This patch fixes alias resolution for version 2 to ensure (a) forwards
compatibility with version 3, and (b) proper resolution of aliases with regards
to services resolved by abstract factories.

The original report in #66 was that doing the following did not work correctly:

```php
$sm->setAlias('foo', InvokableObject::class);
$sm->setFactory(InvokableObject::class, InvokableFactory::class);
$result = $sm->get('foo');
```

When the above code was executed, `InvokableFactory::createService()` was
receiving arguments that it could not use, and thus raising an exception. On
inspection, what I discovered was that the `$cName` provided as a normalized
version of the resolved alias — and due to normalization, would not resolve to a
fully qualified class name. This was due to the fact that the SM was storing a
normalized version of the alias target.

As such, the patch does the following:

- Alias targets are no longer canonicalized in the internal `$aliases` array.
  This means that circular detection must canonicalize the targets for purposes
  of comparison when iterating aliases.

  It also means that when an alias is resolved, the name returned is the name as
  registered with the alias — which becomes important when considering fully
  qualified class names.

- `get()` now resets the `$name` to the *resolved alias target* if an alias was
  detected. Combined with the above change, this means that factories and
  abstract factories now receive the resolved alias target as the requested
  name, and its canonicalized version as the `$cName`.

One test broke with this change: `testGetAbstractFactoryWithAlias()`.

Previously, it read as follows:

```php
    public function testGetAbstractFactoryWithAlias()
    {
        $this->serviceManager->addAbstractFactory('ZendTest\ServiceManager\TestAsset\FooAbstractFactory');
        $this->serviceManager->setAlias('foo', 'ZendTest\ServiceManager\TestAsset\FooAbstractFactory');
        $this->assertInstanceOf('ZendTest\ServiceManager\TestAsset\Foo', $this->serviceManager->get('foo'));
    }
```

My assertion is that the test expectation is incorrect. The resolved alias of
'foo' is `ZendTest\ServiceManager\TestAsset\FooAbstractFactory`, *which that
same abstract factory cannot resolve!* The behavior when retrieving a service
via alias should be identical to fetching the alias target (which is the case
with version 3).

As such, this test rewrites the test to demonstrate what I consider the intended
expectation:

```php
    public function testGetAbstractFactoryWithAlias()
    {
        $expected = new TestAsset\Foo;
        $abstractFactory = $this->prophesize(AbstractFactoryInterface::class);
        $abstractFactory->canCreateServiceWithName(
            $this->serviceManager,
            'foo',
            'Foo'
        )->willReturn(true);
        $abstractFactory->createServiceWithName(
            $this->serviceManager,
            'foo',
            'Foo'
        )->willReturn($expected);
        $this->serviceManager->addAbstractFactory($abstractFactory->reveal());
        $this->serviceManager->setAlias('bar', 'Foo');
        $this->assertSame($expected, $this->serviceManager->get('bar'));
    }
```
  • Loading branch information
@weierophinney
weierophinney committed Jan 13, 2016
1 parent b9fc91b commit a7bc0d1
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 15 deletions.
17 changes: 7 additions & 10 deletions src/ServiceManager.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -418,8 +418,7 @@ public function setShared($name, $isShared)
{
$cName = $this->canonicalizeName($name);

if (
!isset($this->invokableClasses[$cName])
if (!isset($this->invokableClasses[$cName])
&& !isset($this->factories[$cName])
&& !$this->canCreateFromAbstractFactory($cName, $name)
) {
Expand Down Expand Up @@ -505,7 +504,8 @@ public function get($name, $usePeeringServiceManagers = true)

if ($this->hasAlias($cName)) {
$isAlias = true;
$cName = $this->resolveAlias($cName);
$name = $this->resolveAlias($cName);
$cName = $this->canonicalizeName($name);
}

$instance = null;
Expand All @@ -524,8 +524,7 @@ public function get($name, $usePeeringServiceManagers = true)

if (!$instance) {
$this->checkNestedContextStart($cName);
if (
isset($this->invokableClasses[$cName])
if (isset($this->invokableClasses[$cName])
|| isset($this->factories[$cName])
|| isset($this->aliases[$cName])
|| $this->canCreateFromAbstractFactory($cName, $name)
Expand Down Expand Up @@ -562,8 +561,7 @@ public function get($name, $usePeeringServiceManagers = true)
));
}

if (
($this->shareByDefault && !isset($this->shared[$cName]))
if (($this->shareByDefault && !isset($this->shared[$cName]))
|| (isset($this->shared[$cName]) && $this->shared[$cName] === true)
) {
$this->instances[$cName] = $instance;
Expand Down Expand Up @@ -793,7 +791,7 @@ public function canCreateFromAbstractFactory($cName, $rName)
protected function checkForCircularAliasReference($alias, $nameOrAlias)
{
$aliases = $this->aliases;
$aliases[$alias] = $nameOrAlias;
$aliases[$alias] = $this->canonicalizeName($nameOrAlias);
$stack = [];

while (isset($aliases[$alias])) {
Expand All @@ -808,7 +806,7 @@ protected function checkForCircularAliasReference($alias, $nameOrAlias)
}

$stack[$alias] = $alias;
$alias = $aliases[$alias];
$alias = $this->canonicalizeName($aliases[$alias]);
}

return $this;
Expand All @@ -828,7 +826,6 @@ public function setAlias($alias, $nameOrAlias)
}

$cAlias = $this->canonicalizeName($alias);
$nameOrAlias = $this->canonicalizeName($nameOrAlias);

if ($alias == '' || $nameOrAlias == '') {
throw new Exception\InvalidServiceNameException('Invalid service name alias');
Expand Down
33 changes: 28 additions & 5 deletions test/ServiceManagerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
use PHPUnit_Framework_TestCase as TestCase;
use Zend\Di\Di;
use Zend\Mvc\Service\DiFactory;
use Zend\ServiceManager\AbstractFactoryInterface;
use Zend\ServiceManager\Di\DiAbstractServiceFactory;
use Zend\ServiceManager\Exception;
use Zend\ServiceManager\Factory\InvokableFactory;
use Zend\ServiceManager\ServiceManager;
use Zend\ServiceManager\Config;
use ZendTest\ServiceManager\TestAsset\FooCounterAbstractFactory;
Expand Down Expand Up @@ -248,9 +250,21 @@ public function testGetWithAlias()
*/
public function testGetAbstractFactoryWithAlias()
{
$this->serviceManager->addAbstractFactory('ZendTest\ServiceManager\TestAsset\FooAbstractFactory');
$this->serviceManager->setAlias('foo', 'ZendTest\ServiceManager\TestAsset\FooAbstractFactory');
$this->assertInstanceOf('ZendTest\ServiceManager\TestAsset\Foo', $this->serviceManager->get('foo'));
$expected = new TestAsset\Foo;
$abstractFactory = $this->prophesize(AbstractFactoryInterface::class);
$abstractFactory->canCreateServiceWithName(
$this->serviceManager,
'foo',
'Foo'
)->willReturn(true);
$abstractFactory->createServiceWithName(
$this->serviceManager,
'foo',
'Foo'
)->willReturn($expected);
$this->serviceManager->addAbstractFactory($abstractFactory->reveal());
$this->serviceManager->setAlias('bar', 'Foo');
$this->assertSame($expected, $this->serviceManager->get('bar'));
}

/**
Expand Down Expand Up @@ -938,8 +952,6 @@ public function testUsesMultipleDelegates()
*/
public function testSetCircularAliasReferenceThrowsException()
{
$this->setExpectedException('Zend\ServiceManager\Exception\CircularReferenceException');

// Only affects service managers that allow overwriting definitions
$this->serviceManager->setAllowOverride(true);
$this->serviceManager->setInvokableClass('foo-service', 'stdClass');
Expand All @@ -948,6 +960,7 @@ public function testSetCircularAliasReferenceThrowsException()
$this->serviceManager->setAlias('baz-alias', 'bar-alias');

// This will now cause a cyclic reference and should throw an exception
$this->setExpectedException('Zend\ServiceManager\Exception\CircularReferenceException');
$this->serviceManager->setAlias('foo-alias', 'bar-alias');
}

Expand Down Expand Up @@ -1226,4 +1239,14 @@ public function testServiceCanBeFoundFromPeeringServicesManagers()
$this->assertSame($expectedService, $peeredServiceManager->get('peered_service'));
$this->assertSame($expectedService, $secondParentServiceManager->get('peered_service'));
}

public function testCanGetServiceUsingAliasAndBuildByInvokableFactory()
{
$this->serviceManager->setAlias('foo', TestAsset\InvokableObject::class);
$this->serviceManager->setFactory(TestAsset\InvokableObject::class, InvokableFactory::class);

$result = $this->serviceManager->get('foo');

$this->assertInstanceOf(TestAsset\InvokableObject::class, $result);
}
}

0 comments on commit a7bc0d1

Please sign in to comment.