-
Notifications
You must be signed in to change notification settings - Fork 89
BUGFIXES: Processing of preconfigured settings and invokable resolution #242
BUGFIXES: Processing of preconfigured settings and invokable resolution #242
Conversation
requested name 'foo'.
aliases and factories. Added test to show that invokables override delegators and delegators. Added a test to show that preconfigured invokables are ignored. Fixed that problems.
This is ready for review. |
…igured settings are processed. Resolves bug which could cause invokables to overwrite delegators and factories. Removed tests which explicitly tested that invokables were tranformed to aliases and factories (they are now stored in a member array).
d2b4709
to
5aa73cc
Compare
…igured settings are processed. Resolves bug which could cause invokables to overwrite delegators and factories. Removed tests which explicitly tested that invokables were tranformed to aliases and factories (they are now stored in a member array).
Heres the benchmark for ServiceManagerCreation. Done with --revs=500 --iterations=50.
|
Here are the comparison reports for the rest of the benchmarks. Tests which feature the creation of invokables profit from the fix.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks sane. My one hesitation is the change to how invokables are handled internally, as it results in test deletions. That said, the public behavior is well-tested, and the behavior changes have good tests.
@Ocramius — thoughts?
(Marking as requiring changes, as a commented-out line needs removal.)
@@ -640,7 +640,7 @@ public function testCanInjectInvokables() | |||
$container = $this->createContainer(); | |||
$container->setInvokableClass('foo', stdClass::class); | |||
$this->assertTrue($container->has('foo')); | |||
$this->assertTrue($container->has(stdClass::class)); | |||
// $this->assertTrue($container->has(stdClass::class)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the line entirely, instead of commenting it out.
The tests deleted checked the particular implementation of invokables (i.e. the transformation to alias and factory). Kind of white box testing. As the implementation changed, these tests got obsolete. To be complete, I could have introduced a white box test verifying that an invokable really gets stored in the invokables array. I believe, that would be overdone a bit. |
combination of alias and factory, which has changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more change needed, as indicated, but I'll do that during merge.
Thanks!
*/ | ||
public function __invoke(ContainerInterface $container, $name, callable $callback, array $options = null) | ||
{ | ||
return $callback($options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct.
$callback()
should be called without any arguments; it essentially executes any previous delegators and/or the factory that produces the original service and returns the service.
$options
is present for abstract plugin managers, to indicate what $options
were provided to build()
. Delegators will typically ignore them.
@fhein I've merged locally. However, this patch is wildly different than what is on develop currently, with regards to invokables, which is making merging there quite difficult. Should I leave the code on develop as-is, or attempt a merge? |
Is there a way you could leave the pain up to me? I think, the bug fix is most important. I offer to take care of develop by rebasing if that's possible. The main difference of this to develop is that alias handling was introduced to develop which is not contained in this PR. But I will have a closer look on the diffs. |
Puh. Aliases and setter optimization and minor adjustments. Would a process like this work? 1, Merge this with master. |
* | ||
* @var string[]|callable[] | ||
*/ | ||
protected $invokables = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a new property, we should use private
to avoid introducing even more BC issues.
benchmarks/SetNewServicesBench.php
Outdated
]; | ||
|
||
for ($i = 0; $i <= self::NUM_SERVICES; $i++) { | ||
$config['factories']["factory_$i"] = BenchAsset\FactoryFoo::class; | ||
$config['aliases']["alias_$i"] = "service_$i"; | ||
$config['abstract_factories'][] = BenchAsset\AbstractFactoryFoo::class; | ||
$config['invokables']['invokable_$i'] = BenchAsset\Foo::class; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$i
is not being interpolated here
benchmarks/SetNewServicesBench.php
Outdated
]; | ||
|
||
for ($i = 0; $i <= self::NUM_SERVICES; $i++) { | ||
$config['factories']["factory_$i"] = BenchAsset\FactoryFoo::class; | ||
$config['aliases']["alias_$i"] = "service_$i"; | ||
$config['abstract_factories'][] = BenchAsset\AbstractFactoryFoo::class; | ||
$config['invokables']['invokable_$i'] = BenchAsset\Foo::class; | ||
$config['delegators']['delegator_$i'] = [ DelegatorFactoryFoo::class ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$i
is not being interpolated here
src/ServiceManager.php
Outdated
$this->configure($config); | ||
$this->configured = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved back into configure()
to avoid BC issues - calling just the parent constructor may still lead a child class to change the value to false
afterwards.
@@ -425,7 +434,7 @@ public function setAlias($alias, $target) | |||
*/ | |||
public function setInvokableClass($name, $class = null) | |||
{ | |||
$this->configure(['invokables' => [$name => $class ?: $name]]); | |||
$this->configure(['invokables' => [ $name => (isset($class) ? $class : $name)]]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be replaced by ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?? is PHP 7. Am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package has PHP 7.1 as minimum supported PHP version, so you can use all the fancy stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a PR against master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, totally missed that =_=
@@ -151,40 +155,6 @@ public function testShareability($sharedByDefault, $serviceShared, $serviceDefin | |||
$this->assertEquals($shouldBeSameInstance, $a === $b); | |||
} | |||
|
|||
public function testMapsOneToOneInvokablesAsInvokableFactoriesInternally() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for the removal of this test? While it is true that we should always treat the SUT as a black box, this test protected us against BC issues with subclassing (factories
property is protected
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for removal is that the implementation does not map invokables to factories at configuration time any more. See my comment above your review.
], 'factories', $serviceManager, 'Invokable object factory not found'); | ||
} | ||
|
||
public function testMapsNonSymmetricInvokablesAsAliasPlusInvokableFactory() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here - while the test was indeed ugly (accessing object state) it exposes the BC boundary around protected $aliases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason.
test/ServiceManagerTest.php
Outdated
@@ -283,4 +253,92 @@ public function testFactoryMayBeStaticMethodDescribedByCallableString() | |||
$serviceManager = new SimpleServiceManager($config); | |||
$this->assertEquals(stdClass::class, get_class($serviceManager->get(stdClass::class))); | |||
} | |||
|
|||
public function testMemberBasedConfigurationGetsApplied() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to split this test into multiple ones? Ideally, the comments you put above each block should be the test method name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On command. :)
test/ServiceManagerTest.php
Outdated
|
||
$object1 = $sm->build('factory1'); | ||
// assert delegated object is produced by delegator factory | ||
$this->assertTrue(isset($object1->delegatorTag)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertPropertyExist
should probably be used here
test/ServiceManagerTest.php
Outdated
$this->assertFalse(isset($object2->delegatorTag)); | ||
$this->assertInstanceOf(InvokableObject::class, $object2); | ||
|
||
$sm->setInvokableClass('factory1', stdClass::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should raise an exception due to already set service
No, this doesn't work. It creates a merge conflict when we're ready to merge develop back to master in order to prep the next minor release, which means having to resolve it all again anyways. (Learned this the hard way.) Looks like there are other changes requested, so I'll wait for now. |
assertObjectHasProperty/assertObjectNotHasProperty
preconfiguration by child class.
reference parameter. getFactory was replaced by createObjectThroughFactory. Adjusted delegator creation accordingly.
…e technique there.
…igured settings are processed. Resolves bug which could cause invokables to overwrite delegators and factories. Removed tests which explicitly tested that invokables were tranformed to aliases and factories (they are now stored in a member array).
…e technique there.
Something more I should do? |
…zend-servicemanager/master Applied PR zendframework#242 to master
Apply PR zendframework#242 to zend-master
I noticed that is quite a lot of work to rebase develop to master after #242 was applied to master. With PR #251 I provide a branch which represents a rebase of current develop onto (current master with PR #242 applied). This can obviously not get automatically be merged with develop, but it delivers a reference, how develop should look like after integration of PR #242. I hope this helps. If I can do more, please let me know. |
This was replaced by #253. Please see documentation there. |
This PR adds tests and fixes for several issues around configuration and resolution of Invokables.
name => InvokableClass
can override delegators and factories.A performance gain more than 100% in FetchNewServiceManagerBench.php is a welcome side effect.