Bugfixes as of #242 applied to develop + more #231
Conversation
There was a problem hiding this comment.
There are quite a number of changes that are unnecessary, and revert changes made previously for internal consistency. Most of these are returning from within a conditional and then raising an exception as the main method body, which is contrary to how we typically raise exceptions. For consistency, I'm requesting that you revert those changes, and I can review again once they are so I can truly understand what changes this pull request creates.
| <?php | ||
| /** | ||
| * @link http://github.com/zendframework/zend-servicemanager for the canonical source repository | ||
| * @copyright Copyright (c) 2016 Zend Technologies USA Inc. (http://www.zend.com) |
There was a problem hiding this comment.
Please change the year to 2018, and the link to use https vs http.
| /** | ||
| * @link http://github.com/zendframework/zend-servicemanager for the canonical source repository | ||
| * @copyright Copyright (c) 2016 Zend Technologies USA Inc. (http://www.zend.com) | ||
| * @license http://framework.zend.com/license/new-bsd New BSD License |
benchmarks/SetNewServicesBench.php
Outdated
| public function benchSetInvokableClass() | ||
| { | ||
|
|
||
| // @todo @link https://github.com/phpbench/phpbench/issues/304 |
There was a problem hiding this comment.
Here and in the remaining new method additions, please:
- Move the
@todoannotation to a method-level docblock - Remove the leading line at the start of the method
src/ServiceManager.php
Outdated
| } | ||
|
|
||
| $this->mapAliasToTarget($alias, $target); | ||
| throw ContainerModificationsNotAllowedException::fromExistingService($alias); |
There was a problem hiding this comment.
Please revert this change.
Our convention is to throw exceptions from within conditionals, and do the actual method work within the method body:
if (isset($this->services[$alias]) && ! $this->allowOverride) {
throw ContainerModificationNotAllowedException::fromExistingService($alias);
}
$this->mapAliasToTarget($alias, $target);
src/ServiceManager.php
Outdated
| } | ||
|
|
||
| $this->createAliasesAndFactoriesForInvokables([$name => $class ?? $name]); | ||
| throw ContainerModificationsNotAllowedException::fromExistingService($name); |
There was a problem hiding this comment.
Same comment here; please revert this change.
src/ServiceManager.php
Outdated
| } | ||
| $this->services[$name] = $service; | ||
| throw ContainerModificationsNotAllowedException::fromExistingService($name); |
There was a problem hiding this comment.
Revert this change, please.
src/ServiceManager.php
Outdated
| } | ||
|
|
||
| $this->shared[$name] = (bool) $flag; | ||
| throw ContainerModificationsNotAllowedException::fromExistingService($name); |
There was a problem hiding this comment.
Revert this change, please.
src/ServiceManager.php
Outdated
| // Cached string factory name | ||
| if (! isset($this->cachedAbstractFactories[$abstractFactory])) { | ||
| $this->cachedAbstractFactories[$abstractFactory] = new $abstractFactory(); | ||
| foreach ($abstractFactories as $_ => $abstractFactory) { |
There was a problem hiding this comment.
Remove the $_ variable. It's not used, and is confusing otherwise.
src/ServiceManager.php
Outdated
| if ($abstractFactory instanceof Factory\AbstractFactoryInterface) { | ||
| $abstractFactoryObjHash = spl_object_hash($abstractFactory); | ||
| $this->abstractFactories[$abstractFactoryObjHash] = $abstractFactory; | ||
| return; |
There was a problem hiding this comment.
Shouldn't this be a continue statement?
test/ServiceManagerTest.php
Outdated
| // ]); | ||
| // $this->assertSame($sm->get('alias1'), $sm->get('alias2')); | ||
| // $this->assertSame($sm->get(stdClass::class), $sm->get('alias1')); | ||
| // } |
There was a problem hiding this comment.
Why are these commented out? IIRC, these are from #230? Please rebase and re-instate.
There was a problem hiding this comment.
They moved to CommonServiceLocatorBehavioursTrait.php, so that AbstractPluginManager gets tested also. I delete the comments.
|
This PR is now ready for review and merge. I rebased to current develop. This thing comes out of the twilight zone, where @Ocramius and I discussed how modular and distint good PRs would be ideally. I comitted to modularity and stopped work on #221. But #221 was not complete. It was not my best idea to do this cut on an uncomplete PR. Anyway, I did. Please take this PR as what it is. The missing parts of #221. There were bugs in #221, which will cause regression. They are fixed here. Some of the bugs were introduced with #221, which was incomplete. Some of them are evident even in master currently. It is not granted that ServiceManager respects and integrates all (hard coded) configurations. In particular: If a derived class defines $abstractFactories in code, this will be ignored, Same for initializers. This PR finalizes the work of #221, fixes the bugs #221 introduced, and it provides a test case to make sure, that member var based configs of derived classes get processed and included. You may want to apply the introduced test on master. |
|
For your concenience I created PR #242 . Should I continue with PR's like such (there are enough tests missing)? I'd prefer to care about those things. |
| */ | ||
| public function __invoke(ContainerInterface $container, $name, callable $callback, array $options = null) | ||
| { | ||
| $foo = call_user_func($callback); |
There was a problem hiding this comment.
Considering this branch targets PHP 7.1 at this time, we can use the simpler $foo = $callback(); here.
src/ServiceManager.php
Outdated
|
|
||
| if (! empty($this->invokables)) { | ||
| $this->createAliasesAndFactoriesForInvokables($this->invokables); | ||
| } |
There was a problem hiding this comment.
Can you provide some background on the above changes, please? What problems do they solve?
There was a problem hiding this comment.
If a derived class defines $this->aliases or $this->abstractFactories, $this->invokables or $this->initializers those settings were ignored at ServiceManager construction time.
configure() does only look at the supplied $config array, so these preconfigurations were not processed, i.e. resolveAbstractFactories, resolveInitializers, ...
There was a problem hiding this comment.
If a derived class defines $this->aliases or $this->abstractFactories, $this->invokables or $this->initializers those settings were ignored at ServiceManager construction time.
configure() does only look at the supplied $config array, so these preconfigurations were not processed, i.e. resolveAbstractFactories, resolveInitializers, ...
src/ServiceManager.php
Outdated
| @@ -411,13 +399,12 @@ public function configure(array $config) | |||
| * @throws ContainerModificationsNotAllowedException if $alias already | |||
| * exists as a service and overrides are disallowed. | |||
| */ | |||
| public function setAlias($alias, $target) | |||
| public function setAlias($name, $target) | |||
There was a problem hiding this comment.
Why did you change this variable name? $alias makes the intention of the variable more clear.
If you had not changed the name, the diff would not show any changes in this method; as such, please revert.
There was a problem hiding this comment.
This change was meant to have the setAlias paremeters consistent with all the other setter's params.
Not good?
There was a problem hiding this comment.
No; context is different here. The first argument is the alias that will resolve to the target service name.
src/ServiceManager.php
Outdated
| */ | ||
| private function resolveInitializers(array $initializers) | ||
| private function resolveInitializers(array $initializers, $constructing = false) |
There was a problem hiding this comment.
Since this branch targets PHP 7.1, we can use scalar typehints; let's do that here.
Also, I don't understand what $constructing means as a flag. When should I pass it? What behavior is associated with it? Is there perhaps a more descriptive name for the flag that will better indicate its purpose?
There was a problem hiding this comment.
ResolveAbstractFactories and ..Initializers are designed to add abstract factories/initializers from a supplied array which is not the member array $this->abstractFactories or $this->initializers.
If called with the member arrays this inplace resolution will not work. That was not a problem before, because preconfigured abstract factories and initializers were ignored at all before (bug).
$constructing set true should be used only once on initial setup (i.e. in __construct) to indicate, that the supplied array is the member array ($this->abstractFactories and $this->initializers respectively).
This construct is not pretty.
An alternative could be to allow calls to that resolvers with null parameter to indicate that the member arrays should be used. I think that would look a bit nicer. Please advice.
There was a problem hiding this comment.
I'm fine with either of these options; the only caveat is that if you keep the current approach proposed here, $constructing needs to be renamed to something like $calledFromConstructor or $forExisting(Initializers|AbstractFactories) to better indicate its purpose.
There was a problem hiding this comment.
Removed $constructing and call with null argument now.
src/ServiceManager.php
Outdated
| */ | ||
| private function resolveAbstractFactoryInstance($abstractFactory) | ||
| private function resolveAbstractFactories($abstractFactories, $constructing = false) |
There was a problem hiding this comment.
Same comment as before: the name $constructing is unclear here.
| @@ -18,7 +18,6 @@ class DelegatorFactoryFoo implements DelegatorFactoryInterface | |||
| */ | |||
| public function __invoke(ContainerInterface $container, $name, callable $callback, array $options = null) | |||
| { | |||
| $foo = call_user_func($callback); | |||
| return $foo; | |||
| return $callback; | |||
|
I cannot resolve the conflicting file ServiceManager.php, because this PR provides the version to maintain. The single issue is about a docblock. |
|
@fhein — The process to use is as follows: $ git fetch origin # or whatever you call the upstream remote pointing to this repo
$ git rebase origin/develop # again, replace origin if necessary
# if it indicates a merge conflict:
$ git mergetool # make sure you have configured a diff editor!
# once you've resolved conflicts and staged any changes:
$ git rebase --continue
# after all commits are replayed and the rebase is over:
$ git push -f <your remote> <local branch>Note the |
fhein
force-pushed
the
Rest-of-#211
branch
2 times, most recently
from
54f4cb7 to
a418817
Compare
|
@weierophinney: Thank you very much for the detailed process description. Very helpful. 👍 |
|
All tests below with --revs=100000. |
|
I cared about SerivceManager construction and configuration yet. Service resolution and delivery was not a topic for me so far. This will be the next thing for me to do. Expressive or whatever the current thing to promote is called relies on state-of-the-art high performing base components. ServiceManager currently is far awy of being any of these. |
I have zero clue where this comment is coming from or who it is directed to. The last comment I made on this patch, which was the last one made by any maintainers, was instructions to help you rebase, which you appeared to acknowledge with enthusiasm. Why the vitriol? |
|
No vitriol. I am comitted. I did the benchmarks again, I thought about those nasty bugs, I thought about the issues I opened hoping a discussion would start about what to do with Invokables and Aliases in future versions. I was disappointed about the silence I got in response. And I remembered that there seems to be a community of 60+ people. I wanted to reassure myself, that those people are still there. 60+ thumbs down are better than nothing at all. |
|
Heres the benchmark for ServiceManagerCreation. Done with --revs=500 --iterations=50. |
There are 1-2 dozen folks with commit rights, but we also have ~200 repositories under the organizations we maintain, so we have to split our attention between them. As such, please be patient; we are aware of your patches, and we'll get to them. We've already been providing reviews, and will continue to do so as we gradually merge changes. |
|
Ok. I currently can spend a serious amount of time on ServiceManager. |
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.
$i is not replaced here - single quotes were used, so only one invokable is ever defined
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 ]; |
|
|
||
| class InitializerFoo implements InitializerInterface | ||
| { | ||
| protected $options; |
There was a problem hiding this comment.
private and docblock (yeah, I know it's a test class
There was a problem hiding this comment.
Would be the only benchasset featuring a docblock. Do it anyway?
| { | ||
| } | ||
|
|
||
| public function __construct($options = null) |
There was a problem hiding this comment.
can you add the array type? Or was it something untyped?
src/ServiceManager.php
Outdated
| * | ||
| * @var bool | ||
| */ | ||
| protected $configured = false; |
There was a problem hiding this comment.
Removing this field is a BC break - we'll need to keep it, even if we don't rely on it.
| $this->mapAliasesToTargets(); | ||
| } | ||
| if (! empty($config['delegators'])) { | ||
| $this->delegators = array_merge_recursive($this->delegators, $config['delegators']); |
There was a problem hiding this comment.
Why is a recursive merge used here?
There was a problem hiding this comment.
Interesting. That's in master already. I will have a look. See #249 also.
| // If $this->services was not empty, we are here because $this->allowOverride was false, | ||
| // so checking $this->services only is sufficient, also. | ||
| if (isset($this->services[$name])) { | ||
| throw ContainerModificationsNotAllowedException::fromExistingService($name); |
There was a problem hiding this comment.
Can the exceptions be differentiated by type?
There was a problem hiding this comment.
Currently the exact behaviour of master is modelled. What I mainly did was inlining for performance to get rid of the function call overhead.
To change this would not be related to the bug fixes and performance optimizations introduced here. This would be a new feature.
| // If $this->services was not empty, we are here because $this->allowOverride was false, | ||
| // so checking $this->services only is sufficient, also. | ||
| if (isset($this->services[$name])) { | ||
| throw ContainerModificationsNotAllowedException::fromExistingService($name); |
There was a problem hiding this comment.
Can the exceptions be differentiated by type for better messages? Or is it not worth doing?
src/ServiceManager.php
Outdated
| @@ -562,7 +644,7 @@ private function resolveInitializers(array $initializers) | |||
| * @return callable | |||
| * @throws ServiceNotFoundException | |||
| */ | |||
| private function getFactory($name) | |||
| private function getFactory(&$name) | |||
src/ServiceManager.php
Outdated
| @@ -882,39 +877,40 @@ private function mapAliasToTarget($alias, $target) | |||
| */ | |||
| private function mapAliasesToTargets() | |||
| { | |||
| // localize to avoid continous dereferencing | |||
There was a problem hiding this comment.
Continuous dereferencing is how the engine works. Adding references to it just adds extremely big complexity, including possible JIT bailout if that ever lands in master/7.3 :-\
There was a problem hiding this comment.
So I revert. I found that pattern in other zf components, so I adopted it.
sharedByDefault when allowOverride is false, because sharedServices remain shared anyway.
…e technique there.
fhein
changed the title
|
I don't understand what is being asked, to be honest: we just do our best and so should you, that's all. Mistakes happen, forget about #241 (which passes tests btw). |
|
I was happy about the reviews I got. I processed the requests immediately, wanted to go to sleep, but then I recognized, that this thing had become out of sync. I am not good in pushing such issues to the next day. So I rebased like hell being not so happy any more. Sorry. I'll continue to do my best as fast as possible. Anyway, thanks for reminding me. Thanks in exchange |
|
Anything I could do to make this challenging merge somewhat easier to swallow? Is it acceptable at all? Should I explain changes in more detail? Can I do anything to help? |
|
This one @Ocramius obviously forgot when he kicked me out. @Ocramius, @weierophinney; It would be a matter of honour to not only reject my attempts to ignite some discussions in this lazy community but also reject code which is mine. You do not get copyright by applying copyright notices to things you do not own. So please forget about all my ideas and concepts. When I started, I got answers by copy&paste. Flaming was my fast track to get attention at all. It is not much meant with 'attention'. Just to get answers, which have at least some minimal relation to the question. I think, I managed to reach that level at the end. No rationale for me for more flames. But when some idiot recommended me to get out out to the public and talk about the weather when I was discussing an important topic with @Ocramius, and I answered that guy, that I would recommend him to stick a finger into his butt to find out, if he's still alive at all... No go, no go, no go, kick him out. I do not completely understand why it's me and not this guy who gets kicked, Anyway, important guy for some reason. Effects: Important guy remains important, accomplishing what? Important things, I guess. Important questions can be answered with copy&paste essays again. That makes life easier. Good luck. Don't use code you do not own. :) I stay committed, goal is: ServiceManager configuration at about 100 microseconds. my code currently: 290 microseconds, current master 880 microseconds. Forget about my code, please. You start with 880 to the competition, please. Or don't compete at all. PHP-DI is the thing @Ocramius puts his bets on, I learned. |
This PR initially delivered the rest of #221. As several bugs were unrevealed in the process I created according tests and fixes for master with #242.
This PR applies the same fixes to develop recognizing that #221 and #230 were already merged. The issuese fixed are in particular
A performance gain of more than 100% in FetchNewServiceManagerBench.php is a welcome side effect.
Btw, I should have named this branch initially 'Rest-Of-#221'. 211 was wrong right from the beginning. :(