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

Fixed creationContext not being passed to abstract factory canCreate() #79

Closed
wants to merge 1 commit into from
Closed

Fixed creationContext not being passed to abstract factory canCreate() #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions src/ServiceManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ public function has($name)

// Check abstract factories
foreach ($this->abstractFactories as $abstractFactory) {
if ($abstractFactory->canCreate($this, $name)) {
if ($abstractFactory->canCreate($this->creationContext, $name)) {
return true;
}
}
Expand Down Expand Up @@ -608,7 +608,7 @@ private function getFactory($name)

// Check abstract factories
foreach ($this->abstractFactories as $abstractFactory) {
if ($abstractFactory->canCreate($this, $name)) {
if ($abstractFactory->canCreate($this->creationContext, $name)) {
return $abstractFactory;
}
}
Expand Down
14 changes: 14 additions & 0 deletions test/AbstractPluginManagerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
use Zend\ServiceManager\Exception\InvalidArgumentException;
use Zend\ServiceManager\Exception\InvalidServiceException;
use Zend\ServiceManager\Exception\ServiceNotFoundException;
use Zend\ServiceManager\Factory\AbstractFactoryInterface;
use Zend\ServiceManager\Factory\FactoryInterface;
use Zend\ServiceManager\Factory\InvokableFactory;
use Zend\ServiceManager\ServiceManager;
Expand Down Expand Up @@ -349,4 +350,17 @@ public function testPassingServiceInstanceViaConfigureShouldRaiseExceptionForInv
stdClass::class => new stdClass(),
]]);
}

public function testAbstractFactoryGetsCreationContext()
{
$serviceManager = new ServiceManager();
$pluginManager = new TestAsset\SimplePluginManager($serviceManager);
$abstractFactory = $this->prophesize(AbstractFactoryInterface::class);
$abstractFactory->canCreate($serviceManager, 'foo')
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldBeCalled() should be added (as far as I remember, without shouldBeCalled this assertion does nothing actually, and changing foo to something completely different does not throw any error... but I may be wrong. Mind testing @Ocramius ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. But the intention was to catch the situation where canCreate() was called with the $pluginManager instead of the $serviceManager, which it does (in a rather ugly fashion ;)

->willReturn(true);
$abstractFactory->__invoke($serviceManager, 'foo', null)
->willReturn(new InvokableObject());
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, ->shouldBeCalled() is necessary.

Copy link
Member

Choose a reason for hiding this comment

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

shouldBeCalled is not necessary, as the purpose of the test is not to verify that the factory was called, but that the InvokableObject was returned (and the factory received those parameters in between).

shouldBeCalled is self::atLeastOnce(), IIRC.

Copy link
Member

Choose a reason for hiding this comment

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

Also: every time I read prophecy code I cringe >.<

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha @danizord absolutely wanted we used Prophecy and I actually quite like it now :). I've tested, you're right. I don't know what I thought that :/.

Copy link

Choose a reason for hiding this comment

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

shouldBeCalled() is not mandatory here, but it is useful for debugging. If the method does not get called, then I'd prefer the test to fail early telling that it was not called instead instad of failing on assertInstanceOf() where I can't see why it failed.

Copy link

Choose a reason for hiding this comment

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

Dynamic methods are bad, mmkay? :-P

cough, I've learned from GandalPHP that magic is good when it works :)

Copy link
Member

Choose a reason for hiding this comment

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

@danizord that's why I wrote a 2 Mb library to avoid having magic methods ;-)

Copy link

Choose a reason for hiding this comment

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

I see, but they don't hurt in this context (tests) IMHO. It just makes the test a lot more readable than:

$this->getMock(MyStuff::class, [], [], '', false)->expects($this->once())->method('doSomething')->with($this->equalTo('something'));

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like (and read) that syntax better, heh.
On Jan 25, 2016 04:52, "Daniel Gimenes" notifications@github.com wrote:

In test/AbstractPluginManagerTest.php
#79 (comment)
:

@@ -349,4 +350,17 @@ public function testPassingServiceInstanceViaConfigureShouldRaiseExceptionForInv
stdClass::class => new stdClass(),
]]);
}
+

  • public function testAbstractFactoryGetsCreationContext()
  • {
  •    $serviceManager = new ServiceManager();

  •    $pluginManager = new TestAsset\SimplePluginManager($serviceManager);

  •    $abstractFactory = $this->prophesize(AbstractFactoryInterface::class);

  •    $abstractFactory->canCreate($serviceManager, 'foo')

  •        ->willReturn(true);

  •    $abstractFactory->__invoke($serviceManager, 'foo', null)

  •        ->willReturn(new InvokableObject());

I see, but they don't hurt in this context (tests) IMHO. It just makes the
test a lot more readable than:

$this->getMock(MyStuff::class, [], [], '', false)->expects($this->once())->method('doSomething')->with($this->equalTo('something'));

—
Reply to this email directly or view it on GitHub
https://github.com/zendframework/zend-servicemanager/pull/79/files#r50652305
.

Copy link

Choose a reason for hiding this comment

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

Rly? Damn, that was my unique argument. 😂

$pluginManager->addAbstractFactory($abstractFactory->reveal());
$this->assertInstanceOf(InvokableObject::class, $pluginManager->get('foo'));
}
}