Fix return type to generic in ReflectionBasedAbstractFactory

[snapshotpl]

Q	A
Documentation	no
Bugfix	yes
BC Break	no
New Feature	no
RFC	no
QA	yes

Description

ReflectionBasedAbstractFactory has invalid return type when invoking. This PR adds also generics to return type, because it always create class directly from $requestedName

[Ocramius]

LGTM, but the baseline got reduced by too much 🤔

[snapshotpl]

Same generic can be applied to InvokableFactory and ConfigAbstractFactory. Do it in this or separate PR?

[boesing]

Works for me. Its only sugar for this repository only so feel free to add the same logic to the other factories as well, no need to have dedicated PRs for this.

I also don't think that we are fixing anything here. We just adjust the type inference which actually does not fix anything but provides more precise type inference which is only needed in this repository (i.e. in tests, etc.). In actual applications, these factories are never called from code inside of these projects, its all within ServiceManager#doCreate and thus nothing upstream users will benefit from. I don't say that this is useless, it helps us reducing our baseline, but thats it.

So I'd rephrase the PRs title to something like "Adjusting type inference for pre-defined factories", but up to you.

[snapshotpl]

I have a case where I use ReflectionBasedAbstractFactory in my factory to create new instance. Typically I use shared instance across system, but in this case I need new one. Using Factory reduce boilerplate code.

$container->get(ReflectionBasedAbstractFactory::class)($container, MyClass::class);

[boesing]

Why not registering MyClass properly to your config and use $container->get(MyClass::class) instead?
That is actually how its meant to be used.

return [
    'factories' => [
         MyClass::class => ReflectionBasedAbstractFactory::class,
    ],
];

That would also enable v4 to actually AoT generate a real factory. With your example, thats not possible.

So yes, your code works, but that will always use reflection at runtime to determine what services need to be injected into MyClass::__construct. That can be improved for production code by creating AoT factories in CI/CD which will basically replace ReflectionBasedAbstractFactory with an actual (generated) factory instead. That will most probably improve production code, especially if you have a bunch of those reflection related factories.

[snapshotpl]

I have already registered this service (and share it in multiple places), but I need - in some case - new instance.

I can create new factory and pass by hand, I know, but now for me development time is more important than performance.

[froschdesign]

@snapshotpl

…but I need - in some case - new instance.

The build method is not an option here?

[boesing]

Yeah, I would also use build method instead of the current implementation but whatever floats your boat.
This is still a good PR, thats why I approved it in the first place.
No need to discuss this more than necessary. There are other ways to achieve your actual goal but I am still +1 on this.

Only problem with the build method is that it does only work if one asserts that the container is ServiceManager or ServiceLocatorInterface, so it might not be the best solution to use build here as it also requires an assertion.

[snapshotpl]

I forgot about build! That should works!