Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

ServiceManager::canCreateFromAbstractFactory() missing foreach break after valid abstract factory found #5795

Merged
merged 2 commits into from Mar 3, 2014

Conversation

Projects
None yet
4 participants
Contributor

bacinsky commented Feb 7, 2014

It seems that in the ServiceManager::canCreateFromAbstractFactoryMethod() is missing foreach break after an abstract factory is found and assigned to the nestedContext.

Missing break after https://github.com/zendframework/zf2/blob/develop/library/Zend/ServiceManager/ServiceManager.php#L732

Now the issue is, that the first abstract factory if can create a service is always overridden with the last abstract factory that can create the same service, e.g. DiAbstractServiceFactory.

For example, I add an abstract factory to the service manager config AdapterAbstractServiceFactory and I configure the db adapter. When I want to inject that adapter with the Di, I have to configure the di instance alias. But, when the service manager is creating that db adapter new instance it continues over the AdapterAbstractServiceFactory to the DiAbstractServiceFactory, because of that missing foreach break. Thus db adapter instance can't be created,

Member

Ocramius commented Feb 6, 2014

@bacinsky the issue seems serious - can you create a failing test case for it?

Contributor

bacinsky commented Feb 7, 2014

Yes it's serious enough, it took me a while to figure out, why I can't inject my multiple db adapters via dependency injection. Added tests + fix.

Member

Ocramius commented Feb 7, 2014

Looking good!

This could also be backported to dev-master IMO.

Contributor

bacinsky commented Feb 7, 2014

Do you mean tests? Because there is no such issue in the master.
The bug was introduced by this merge zendframework#5237

@weierophinney weierophinney added this to the 2.2.6 milestone Mar 3, 2014

@weierophinney weierophinney self-assigned this Mar 3, 2014

@weierophinney weierophinney modified the milestones: 2.3.0, 2.2.6 Mar 3, 2014

weierophinney added a commit that referenced this pull request Mar 3, 2014

Merge pull request #5795 from bacinsky/fix/service-manager-missing-br…
…eak-after-abstract-factory-found

ServiceManager::canCreateFromAbstractFactory() missing foreach break after valid abstract factory found

weierophinney added a commit that referenced this pull request Mar 3, 2014

@weierophinney weierophinney merged commit 6f89e93 into zendframework:develop Mar 3, 2014

1 check passed

default The Travis CI build passed
Details

@bacinsky bacinsky deleted the bacinsky:fix/service-manager-missing-break-after-abstract-factory-found branch Mar 3, 2014

Contributor

primitive-type commented Mar 6, 2014

I am probably in the wrong here, but this PR broke the following code in my Apigility application:

<?php

return array(
    'service_manager' => array(
        'abstract_factories' => array(
            'ZF\Apigility\DbConnectedResourceAbstractFactory' => 'MyOrganization\MyProject\MyDbConnectedResourceAbstractFactory',
        ),
    ),
);

Previously I was able to override an abstract factory using the above code, but after this PR was merged, it's no longer possible. I am guessing that this was an intended effect (perhaps due to the above snippet not being the correct way of doing things), but just in case, I wanted to bring the issue to light.

Owner

weierophinney commented Mar 7, 2014

@primitive-type This PR has to do with how abstract factories are invoked, ensuring that the SM returns as soon as any abstract factory indicates it can create the requested service. The abstract factory indicated by this operation will be the one used when a call to get() or createFromAbstractFactory() is made.

The problem you are detailing is how abstract factories are registered. What's interesting is that the notation you're using has never been supported. Zend\ServiceManager\Config:configureServiceManager() loops through the values only of a an abstract_factories key, and does not do anything with the keys at all; in other words, it will not replace a given abstract factory based on the key. It passes the value then to Zend\ServiceManager\ServiceManager::addAbstractFactory(), which takes two arguments: the abstract factory (or its class name), and an optional flag, $topOfStack, which is, by default, true -- meaning that the abstract factory registered will be at the top of the stack when iterating through abstract factories. This means that if you register your abstract factories after Apigility does, yours will take precedence.

Try removing the key from your configuration; I'm curious if the key is causing it to bubble to the top of the abstract_factories configuration. Normally, since that is expected to be an array, values will be appended -- which will cause later values to be pushed to the top of the stack.

Contributor

primitive-type commented Mar 7, 2014

@weierophinney Removing the key didn't make a difference, but registering the abstract factory by attaching to MvcEvent::EVENT_DISPATCH in the onBootsrap() method of my Module.php did the trick:

public function onBootstrap(\Zend\Mvc\MvcEvent $e)
{
    $app = $e->getApplication();
    $events = $app->getEventManager();
    $sharedEvents = $events->getSharedManager();
    $this->sm = $app->getServiceManager();
    $events->attach(\Zend\Mvc\MvcEvent::EVENT_DISPATCH, array($this, 'registerCustomAbstractFactory'), 100);
}

public function registerCustomAbstractFactory()
{
    $customAbstractFactory = new \MyOrganization\MyProject\DbConnectedResourceAbstractFactory();
    $this->sm->addAbstractFactory($customAbstractFactory);
}

However, what I ended up actually doing in my application is just transferring the logic I previously had in my custom abstract factory to an initializer, and performed the logic there. This removed the need for a custom abstract factory entirely, so it turns out my issue was just a non-issue in this case.

Thanks for the thorough explanation! It really helped my understanding of how ZF2 handles abstract factories a lot.

gianarb pushed a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015

Fix for zendframework/zendframework#5795 - updated ServiceManager, ad…
…ded break after valid abstract factory found

weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5795 from bacinsky/fix…
…/service-manager-missing-break-after-abstract-factory-found

ServiceManager::canCreateFromAbstractFactory() missing foreach break after valid abstract factory found

weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015

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