Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC]: Delegator factories for alias and service name #168

Open
boesing opened this issue Jan 22, 2023 · 8 comments · May be fixed by #190
Open

[RFC]: Delegator factories for alias and service name #168

boesing opened this issue Jan 22, 2023 · 8 comments · May be fixed by #190
Labels

Comments

@boesing
Copy link
Member

boesing commented Jan 22, 2023

RFC

Q A
Proposed Version(s) 4.0.0
BC Break? Yes/No

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 the factories 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:

return [
    'factories' => [
        SystemClock::class => static fn () => new SystemClock(new DateTimeZone('Europe/Berlin')),
    ],
    'aliases' => [
       \Psr\Clock\ClockInterface::class => SystemClock::class,
    ],
    'delegators' => [
        \Psr\Clock\ClockInterface::class => [], // Add something which should be delegated whenever PSR-20 clock is being retrieved
        SystemClock::class => [], // Whatever needs to be delegated regarding the system clock specific impl.
    ],
];
@boesing
Copy link
Member Author

boesing commented May 3, 2023

Hm, I worked on this quite a lot and had some serious problems which I try to summarize here:

internal service caching

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

service request

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 added a commit to boesing/laminas-servicemanager that referenced this issue May 3, 2023
This is just an initial commit and should provide the idea and a base to play around.
There are still some problems which can be reviewed in laminas#168

Signed-off-by: Maximilian Bösing <[email protected]>
@boesing boesing linked a pull request May 3, 2023 that will close this issue
@boesing boesing linked a pull request May 3, 2023 that will close this issue
@internalsystemerror
Copy link
Member

internalsystemerror commented May 8, 2023

@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.

@boesing
Copy link
Member Author

boesing commented May 8, 2023

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.

@internalsystemerror
Copy link
Member

internalsystemerror commented May 8, 2023

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.

@Xerkus
Copy link
Member

Xerkus commented May 9, 2023

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?

@boesing
Copy link
Member Author

boesing commented May 9, 2023

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?

@gsteel
Copy link
Member

gsteel commented May 9, 2023

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.

@boesing
Copy link
Member Author

boesing commented May 14, 2023

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.

@boesing boesing removed this from the 4.0.0 milestone May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants