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

[BUG] Service Manager Not Shared Duplicate new Instance with multiple Abstract Factories #3237

Closed
wants to merge 7 commits into from

Conversation

richardjennings
Copy link
Contributor

Added test: ZendTest\ServiceManager\ServiceManagerTest::testDuplicateNewInstanceMultipleAbstractFactories

Test fails with 1 error.

If an Abstract Factory cannot create an instance, a pendingAbstractFactoryRequests key is left set.

This pendingAbstractFactoryRequests key forces canCreateFromAbstractFactory to return false.

Fix suggestion supplied:
set $this->pendingAbstractFactoryRequests[get_class($abstractFactory)] = $requestedName;

only when $abstractFactory->canCreateServiceWithName($this, $canonicalName, $requestedName) returns true.

All Service Manager Tests pass with supplied fix.

if ($abstractFactory->canCreateServiceWithName($this, $canonicalName, $requestedName)) {
$this->pendingAbstractFactoryRequests[get_class($abstractFactory)] = $requestedName;
Copy link
Member

Choose a reason for hiding this comment

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

Please fix CS.

@@ -654,4 +654,17 @@ public function testCanGetAliasedServicesFromPeeringServiceManagers()

$this->assertSame($service, $this->serviceManager->get('alias-name'));
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

Same here (CS) ;)

weierophinney added a commit that referenced this pull request Dec 18, 2012
@ghost ghost assigned weierophinney Dec 18, 2012
weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-servicemanager that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants