Open boesing opened 1 year ago
Hm, I worked on this quite a lot and had some serious problems which I try to summarize here:
ServiceManager
caches internally, both for services and aliases. Depending on the order of which a service is requested from the ServiceManager
, delegators might not get applied for aliases in case the service was requested from the container via service name before.
Example:
use Laminas\ServiceManager\Factory\InvokableFactory;
use Laminas\ServiceManager\ServiceManager;
class Service
{}
class DelegatedService extends Service
{}
$config = [
'factories' => [
'service' => InvokableFactory::class,
],
'aliases' => [
'alias' => 'service',
],
'delegators' => [
'alias' => [
fn () => new DelegatedService(),
],
],
];
$sm = new ServiceManager($config);
$service = $sm->get('service');
$serviceViaAlias = $sm->get('alias');
assert($service === $serviceViaAlias); // passes
assert( !$serviceViaAlias instanceof DelegatedService); // passes as the `service` request put the requested service to internal cache
Should services requested by their service name (not the alias) also apply all delegators of its aliases? So what to do in this case:
<?php
use Laminas\ServiceManager\Factory\InvokableFactory;
use Laminas\ServiceManager\ServiceManager;
class Service
{}
class DelegatedService extends Service
{
public function __construct(public readonly int $alias)
{}
}
$config = [
'factories' => [
'service' => InvokableFactory::class,
],
'aliases' => [
'alias' => 'service',
'alias2' => 'service',
],
'delegators' => [
'alias' => [
fn () => new DelegatedService(1),
],
'alias2' => [
fn () => new DelegatedService(2),
],
],
];
$sm = new ServiceManager($config);
$service = $sm->get('service');
// should we receive delegated service at all? there are delegators registered for aliases pointing to the service we are fetching
At this point, I am not feeling good applying the changes proposed in this RFC. Does any1 have an idea regarding these issues?
@boesing FWIW I think regardless of the alias used it should be the same instance. Would it be feasible to resolve all aliased delegators to their service name prior to checking for the delegators to add? That could be done in the constructor to ensure its only done once. Then when assigning delegators, you only need to check for the service name?
If a library provides an interface and consumes it, but an app needs a delegator on their specific alias to function correctly, I think the lib would also need that. That's my thinking.
Aliases are not array<interface-string,class-string>
and thus, not all aliases are actually interfaces. Not sure if I understood your suggestion correct tho, but this might be problematic?
If I understand you correct, you suggest to unalias aliased delgators and merge these with the service? Might worth a try tho, especially when it comes to the alias => service direction, that might work. 🤔
Don't have the mood to dive into that right now. Will see if I find some time until thursday.
I was just using the interface for the example lib + app situation. But yes unalias the aliases, since they are resolved to the service name (the one that is invokable or provides a factory) after being called, you could then check only for that when checking what delegators to apply.
Since aliases refer to the same shared service and because behavior would be incosistent and unpredictably changing at runtime otherwise, alias delegators need to be always applied to the service created or never. They can not be applied only when service is requested via alias.
Alternative suggestion to consider: what if instead of doing this in service manager we forbid delegators on aliases (it would be an exception) but provide configuration pre-processing that would rewrite delegators to be on a factory for the service the alias was pointing to?
what if instead of doing this in service manager we forbid delegators on aliases (it would be an exception) but provide configuration pre-processing that would rewrite delegators to be on a factory for the service the alias was pointing to?
Not sure if I understand this proposal.
Factories are more likely to be replaced than services in upstream projects. Thats due to the fact that they might instantiate something else (i.e. when factories is used for interfaces to prevent some1 to fetch services directly from the container and to enforce the interface). Regarding config processing, there are also these ServiceManager#setAlias
and ServiceManager#addDelegator
methods, not sure if pre-processing configuration would work with these methods in-place?
Alternative suggestion to consider: what if instead of doing this in service manager we forbid delegators on aliases (it would be an exception) but provide configuration pre-processing that would rewrite delegators to be on a factory for the service the alias was pointing to?
From a user's POV, I'd prefer that either delegators on aliases work or don't - pre-processing config seems magical - I guess unless it was strictly opt-in.
If supporting delegators for aliases is not feasible, I'd like to see a hard fail. At the moment, if you delegate around an alias, nothing happens, the delegator doesn't run and you scratch your head for a while trying to figure out why, before remembering 'it doesn't work'.
Perhaps forcing delegators to be class-string
would enable cached resolution of delegators in a consistent way:
$config = [
'factories' => [
'Service' => ServiceFactory::class,
],
'aliases' => [
'alias' => 'Service',
'alias2' => 'alias', // It _is_ possible to alias aliases right?
'alias3' => 'Service',
],
'delegators' => [
'Service' => [
ServiceDelegator::class,
],
'alias' => [
AliasDelegator::class,
],
'alias2' => [
AliasDelegator::class,
],
'alias3' => [
AnotherAliasDelegator::class,
],
],
];
// Resolved list of delegators:
$delegators = [
ServiceDelegator::class,
AliasDelegator::class,
AnotherAliasDelegator::class,
];
The problem is that it would require resolving all aliases up-front in order to compile, order, and possibly de-duplicate delegators in a predictable way.
Whilst delegators can be any callable, it would not be possible to 'de-duplicate' them if that's even a footgun we'd attempt to avoid for users.
With that in mind, I can see how config post-processing would alleviate ServiceManager of this complexity.
Detaching this from v4.0.0. This feature won't introduce a BC break as it is not available as of now and thus once its released, can be considered as an addition.
RFC
Goal
Allow users to have delegator factories for aliases.
Background
As of now, only delegator factories are applied to services registered to
factories
. In the past, we put services without its FQCN into thefactories
while aliasing the FQCN to that service name. That was done in laminas-cache, laminas-hydrator, etc.In one minor release, that was changed which ended up being a BC break because upstream projects already registered delegator factories to these services. AFAIR that change got reverted afterwards.
Considerations
This would not require any change from upstream projects as only additional delegators are called.
Proposal(s)
I'd retrieve delegator factories for both service and alias (if aliased and requested via that alias).
Appendix
It might be problematic when it comes to aliases referring to aliases referring to aliases. AFAIR, the
ServiceManager
resolves aliases until either a factory or an invokable could be found. Thats where we might lose information. I'd say we can add this as a known limitation and still implement the feature. Usually, the most common usecase is something like: