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

ServiceManager returns that service does not exist with has() but can create with get() #50

Closed
svycka opened this issue Nov 6, 2015 · 10 comments

Comments

@svycka
Copy link
Contributor

svycka commented Nov 6, 2015

I created service manager like this:

use Zend\ServiceManager\AbstractPluginManager;
use Zend\ServiceManager\Exception\RuntimeException;

class TypesManager extends AbstractPluginManager
{
    protected $invokableClasses = [
        InArrayType::class => InArrayType::class,
        RegexType::class   => RegexType::class,
    ];
    protected $aliases = [
        'in_array' => InArrayType::class,
        'regex'    => RegexType::class,
    ];

    public function validatePlugin($type)
    {
        if ($type instanceof SettingTypeInterface) {
            return; // we're okay
        }
        throw new RuntimeException(sprintf(
            'SettingType of type %s is invalid; must implement %s',
            (is_object($type) ? get_class($type) : gettype($type)),
            SettingTypeInterface::class
        ));
    }
}

I am using like this:

$manager = new TypesManager();
$manager->has(InArrayType::class); // returns false
$manager->has('in_array'); // returns false(canonicalization problem because of `_` ?)
$manager->has(RegexType::class); // returns false
$manager->has('regex'); // returns true
$manager->get(InArrayType::class); // returns instance off InArrayType::class
$manager->get('in_array'); // // returns instance off InArrayType::class
$manager->get(RegexType::class); // returns instance off RegexType::class
$manager->get('regex'); // // returns instance off RegexType::class

when I sow this I thought maybe I doing it wrong so checked other zend modules like zend-filter and wrote some tests:

    public function testFirstGet()
    {
        $manager = new FilterPluginManager();
        $this->assertInstanceOf('Zend\Filter\Boolean', $manager->get('Zend\Filter\Boolean'));
        $this->assertTrue($manager->has('Zend\Filter\Boolean'));
    }
    public function testFirstHas()
    {
        $manager = new FilterPluginManager();
        $this->assertTrue($manager->has('Zend\Filter\Boolean')); // fails here
        $this->assertInstanceOf('Zend\Filter\Boolean', $manager->get('Zend\Filter\Boolean'));
    }

First pass second fails.
order of has() and get() is important?

@demichl68
Copy link
Contributor

Regarding your tests:

The first one passes because after calling "get", the service is added to your invokable services (autoAddInvokableClass).

Your second case fails, because the service has not been added to your invokables when you call "has"

@svycka
Copy link
Contributor Author

svycka commented Nov 6, 2015

@demichl68 the point is that service manager does not allow to check if it can create instance has() return false but get() can create

@svycka
Copy link
Contributor Author

svycka commented Nov 6, 2015

also if service manager implements container-interop then this https://github.com/container-interop/container-interop/blob/master/src/Interop/Container/ContainerInterface.php#L29 states that has() should return true if it can create instance with given identifier

Returns true if the container can return an entry for the given identifier.
Returns false otherwise.

@marc-mabe
Copy link
Member

This bug also breaks a test of zend-cache:

1) ZendTest\Cache\Service\StorageCacheAbstractServiceFactoryTest::testCanLookupCacheByName

Failed asserting that true is false.

/home/travis/build/zendframework/zend-cache/test/Service/StorageCacheAbstractServiceFactoryTest.php:59
    public function setUp()
    {
        Cache\StorageFactory::resetAdapterPluginManager();
        Cache\StorageFactory::resetPluginManager();
        $this->sm = new ServiceManager([
            'services' => [
                'config' => [
                    'caches' => [
                        'Memory' => [
                            'adapter' => 'Memory',
                            'plugins' => ['Serializer', 'ClearExpiredByFactor'],
                        ],
                        'Foo' => [
                            'adapter' => 'Memory',
                            'plugins' => ['Serializer', 'ClearExpiredByFactor'],
                        ],
                    ]
                ],
            ],
            'abstract_factories' => [
                'Zend\Cache\Service\StorageCacheAbstractServiceFactory'
            ]
        ]);
    }
// ...
    public function testCanLookupCacheByName()
    {
        // Since these are delivered by abstract factory, by default, `has()`
        // should return false, as it doesn't consult abstract factories by
        // default.
        $this->assertFalse($this->sm->has('Memory'));
        $this->assertFalse($this->sm->has('Foo'));

        // Passing the boolean true to the second argument forces a lookup
        // via abstract factory.
        $this->assertTrue($this->sm->has('Memory', true));
        $this->assertTrue($this->sm->has('Foo', true));
    }

@weierophinney
Copy link
Member

@marc-mabe — make sure all adapters that zend-cache provides are registered by their FQCN, with short name aliases to the FQCN. That's the approach we can and should take with all components.

@svycka as noted on #51, this is intended behavior. I note a way to get the behavior you request in a comment on that patch request, but we will not be implementing any changes to the existing behavior at this time.

@svycka
Copy link
Contributor Author

svycka commented Jan 12, 2016

@weierophinney but this is against container-interop no? there was discussion about this here container-interop/container-interop#37

@Ocramius what do you think?

@Ocramius
Copy link
Member

@svycka yes, that is an inconsistency with container-interop, but I think we can't do much about it atm (except documenting it)

@Maks3w
Copy link
Member

Maks3w commented Jan 12, 2016

Note this has been fixed on V3. Exactly since #49

@Ocramius
Copy link
Member

@Maks3w thanks for pointing it out!

@Maks3w
Copy link
Member

Maks3w commented Jan 12, 2016

Of course this won't be fixed on V2.

  1. Because break the backward compatibility.
  2. Because container interop support was added the last year but this component is published from before.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants