Second param of ServiceManager::setFactory doesn't accept a class-string of a callable class

[rieschl]

Bug Report

Q	A
Version(s)	3.17.0

Summary

When calling \Laminas\ServiceManager\ServiceManager::setFactory the @psalm-param declaration of the $factory param requires that the class-string implements the \Laminas\ServiceManager\Factory\FactoryInterface.
The service manager also accepts a plain callable, though.

Current behavior

PHPStan complains that the class-string of the class doesn't implement FactoryInterface.

How to reproduce

Try to pass a callable class to setFactory and run PHPStan.

Expected behavior

The method should accept a plain callable class. Psalm and PHPStan don't accept class-string<callable>, though.

[boesing]

Can you probably create a static analysis test in test/StaticAnalysis?

Another question I'd have is: what is the use-case for a dedicated ServiceManager#setFactory call?
I am asking as I was thinking about deprecating those methods:

• ServiceManager#setFactory
• ServiceManager#setInvokableClass
• ServiceManager#setShared

I can't think about a good reason on why to modify the ServiceManager at runtime rather than via initial configuration 🤔
Besides the fact, that its still possible to use ServiceManager#configure (which is not - yet - overriding existing factories).

[rieschl]

TBH, I can't remember anymore. There was a slack conversation beforehand and it was suggested, that I open an issue here.
I'm pretty sure I discovered that behavior when writing a test or after upgrading phpstan and it complained in one of our tests.
In prod, we don't use setFactory but it's quite handy in high level tests.
It probably has its benefits when registering a module programmatically during bootstrap, after the initial Container was already built?

And after a usage search of the method it seems that it's also used in some laminas/mezzio packages 🙃

[Xerkus]

I don't think psalm can document class-string for callable class.

[rieschl]

I don't think psalm can document class-string for callable class.

I don't know that much about that psalm-fu, but using a plain invokable class without implementing FactoryInterface works when building the container with configure or instantiating it and when setting it with setFactory, so the psalm-param is wrong here 😛

[boesing]

Yah, no doubts here.
The biggest problem is, that it is quite tough to tell that something is a class-string of an object having a method __invoke accepting the ContainerInterface, etc. while also providing a safe-__construct so that it can be easily instantiated.

Therefore, I guess we have to get a little more lax there.
But anyways, I'd actually prefer not to keep these methods at all as I don't think there should be runtime stuff manipulating the SM. In laminas-cache, I implemented delegator factories which add additional instances of storage adapters to the AdapterPluginManager which could be a config as well. There is already an RFC around this in laminas/laminas-cache#197

But thats not final yet, just want to see if I can find real-world use-cases where it actually makes sense to keep setFactory, setAlias, etc.

[boesing]

FYI: vimeo/psalm#9599

[boesing]

Will be fixed with #179