Feature: ReflectionBasedAbstractFactory #153
Conversation
|
Ping @GeeH — I think you may be interested in this. Also pinging @zendframework/community-review-team for review. |
|
It is so nice to see documentation with PRs again… |
|
Great to see this in I actually come up with the same idea earlier: click. May way of caching is pretty dumb (making it more elegant would require changes in SM core), but it works fine - I will start using it in production in coming month. Also, I don't like the fact that it comes with code specific to |
|
@mtymek That's a good point. In essence, the zend-mvc I'll update the PR momentarily to remove the initial definitions, as well as to document that aspect of both instantiation and extension. Thanks for the feedback! |
|
@mtymek In terms of caching, that's an idea I discussed with @GeeH with regards to the |
|
It occurs to me that part of the difference in performance may be due to how the benchmark is structured; it has to go through two other abstract factories before this one is hit. I'll see if I can find an improved way to test performance, though I'm not necessarily sure it it needs to be part of this PR. |
|
I'm not convinced this is of value in production without some way to cache the reflection so it's not performed in production. This is not sour grapes because you are diluting the praise I received for the ConfigAbstractFactory, oh no, no siree. It's just that I worry that this will become the "norm" and people will start to moan about how bad performance of ZF is (again). I definitely think this is worth exploring however. |
|
I'm making this as a WIP, with the following goals:
@GeeH is correct - DX is great, but we need an end-to-end solution that also provides production performance. |
|
@weierophinney FWIW I am working on a CLI tool right now for ConfigAbstractFactory, can you ping me on IRC when you have a minute to discuss how I can do both. |
|
Benchmarks:
|
|
Summary of the benchmarks:
The takeaways are:
#154 solves for the performance aspect, as well as the configuration aspect of the My take is that an application will evolve over time:
Ready for review, @zendframework/community-review-team ! |
weierophinney
force-pushed
the
feature/reflection-factory
branch
from
c2a20b8
to
01c285a
Compare
| return function (ReflectionParameter $parameter) use ($container, $requestedName, $hasConfig) { | ||
| if ($parameter->isArray() | ||
| && $parameter->getName() === 'config' | ||
| && $hasConfig |
There was a problem hiding this comment.
What about branching on $hasConfig outside this closure and return a specialized closure for each case?
There was a problem hiding this comment.
I'll give it a try and see if it makes any difference on performance.
| } | ||
|
|
||
| if ($parameter->isArray()) { | ||
| return []; |
There was a problem hiding this comment.
What is the point of passing an empty array here? I'd expect it to throw an exception if the parameter could not be resolved.
There was a problem hiding this comment.
Since it's intended as a rapid application development tool, the idea here is to not error for an array, as we can supply one, even if it's empty. The class itself can raise an exception if the empty array will not work.
|
|
||
| if (! $container->has($type)) { | ||
| throw new ServiceNotFoundException(sprintf( | ||
| 'Unable to create controller "%s"; unable to resolve parameter "%s" using type hint "%s"', |
There was a problem hiding this comment.
Good catch; remnant from the LazyControllerAbstractFactory; will update shortly.
| } | ||
|
|
||
| if (! $parameter->getClass()) { | ||
| return; |
There was a problem hiding this comment.
Here I'd expect it to throw an exception as well instead of failing silently.
There was a problem hiding this comment.
That's a good point; I'll make that change shortly.
This patch ports zend-mvc's `LazyControllerAbstractFactory` to a more generic `ReflectionBasedAbstractFactory` that can be used anywhere. The principal change is that it does not restrict type generation to only dispatchables. One additional change was made: the zend-mvc version hard-codes in a number of "well known" services that use short names, mapping them to the actual classes that handle them. This patch keeps them, but allows you to override them via the constructor.
Includes a comparison with the `ConfigAbstractFactory`.
Looks like it is 100% slower than the ConfigAbstractFactory, which verifies the assumptions made in the documentation.
Ensures de-coupling, and allows the user to configure which services may be necessary to map.
This patch provides more benchmarking for the new abstract factories, pulling them each into separate benchmark classes to ensure they operate in isolation to other abstract factories, and to allow seeing baseline timings. Additionally, each has two benchmarking suites: one demonstrating usage as an abstract factory, another demonstrating usage as a mapped factory. The basic results are that the `ConfigAbstractFactory` operates at roughly the same speed as a normal abstract factory that contains logic for creating an instance. The `ReflectionBasedAbstractFactory` is about 100% slower when at least one dependency is present (and ever-so-slightly faster when no dependencies are present).
weierophinney
force-pushed
the
feature/reflection-factory
branch
from
01c285a
to
15712f9
Compare
- Split the parameter resolution into three:
- A method that handles resolving arrays, detecting typehints, etc. common
across all invocations.
- A method that delegates to the above when the `config` service is not
present.
- A method that will check if the provided parameter name is `config` and hits
against `array`, returning the `config` service; if not, it delegates to the
first.
- The main logic now has a ternary condition to determine which of the latter
two to use, based on whether or not the `config` service is present.
- Raise an exception when detecting a scalar argument (instead of passing null).
- `s/controller/service/` in exception messages.
|
@danizord Incorporated your feedback; thanks! |
| if (! $parameter->getClass()) { | ||
| return; | ||
| } | ||
| if (! $parameter->getClass()) { |
There was a problem hiding this comment.
If $parameter->isDefaultValueAvailable(), we could return $parameter->getDefaultValue() here before falling to exception.
There was a problem hiding this comment.
Okay; will push that momentarily.
|
After making the changes suggested by @danizord , the benchmarks improved; the |
| private function resolveParameter(ReflectionParameter $parameter, ContainerInterface $container, $requestedName) | ||
| { | ||
| if ($parameter->isArray()) { | ||
| return []; |
There was a problem hiding this comment.
Could use default value if available here as well?
There was a problem hiding this comment.
Default can only be an empty array or null; not sure the overhead of retrieving the value via reflection is worth it.
There was a problem hiding this comment.
@weierophinney If a parameter has a default value as null it throws an exception.
I just bumped into it using Zend Expressive Skeleton with default configuration I get Unable to create service "Zend\Expressive\Middleware\ImplicitHeadMiddleware"; unable to resolve parameter "response" using type hint "Psr\Http\Message\ResponseInterface".
As solution can be change this:
if (! $container->has($type)) {
throw new ServiceNotFoundException(sprintf(
'Unable to create service "%s"; unable to resolve parameter "%s" using type hint "%s"',
$requestedName,
$parameter->getName(),
$type
));
}
return $container->get($type);
to
if ($container->has($type)) {
return $container->get($type);
} elseif ($parameter->isOptional()) {
return $parameter->getDefaultValue();
}
throw new ServiceNotFoundException(sprintf(
'Unable to create service "%s"; unable to resolve parameter "%s" using type hint "%s"',
$requestedName,
$parameter->getName(),
$type
));
There was a problem hiding this comment.
@popovserhii
Please open a new issue report for this problem. Thanks!
| */ | ||
| public function __construct(array $aliases = []) | ||
| { | ||
| if (! empty($aliases)) { |
There was a problem hiding this comment.
Is it actually worth doing the empty() check, instead of just assigning directly? (In that case, there'd be no need for the direct property initialization.
There was a problem hiding this comment.
An extending class may define defaults, in which case casting to an empty array would be problematic.
There was a problem hiding this comment.
In that case though, shouldn't the aliases get appended, not override when not empty?
$this->aliases += $aliases;
Although in that case again, the not empty checked is not needed.
Ports zend-mvc LazyControllerAbstractFactory to zend-servicemanager
This patch ports zend-mvc's
LazyControllerAbstractFactoryto a more genericReflectionBasedAbstractFactorythat can be used anywhere. The principal change is that it does not restrict type generation to only dispatchables.One additional change was made: the zend-mvc version hard-codes in a number of "well known" services that use short names, mapping them to the actual classes that handle them. This patch keeps them, but allows you to override them via the constructor.
Documentation is provided, which contrasts it to the ConfigAbstractFactory; benchmarks verify that one principal difference is performance (the ConfigAbstractFactory is 2x faster, but requires more configuration).
I think between this and the ConfigAbstractFactory, we'll have a nice story to tell around developer experience for the 3.2 release.