Improve Delegator Documentation Specifically for Aliased Services

[gsteel]

RFC

Q	A
Proposed Version(s)	current
BC Break?	No

Goal

Update docs to state that delegators cannot target service aliases.

Background

For some reason, I personally can never remember that you can't wrap a service alias with a delegator - you must target the service name that corresponds to concrete factory.

Because this frequently trips me up, I'd like to submit a pull request to update the docs at delegators.md - so I hope it's OK to leave this issue here as a reminder for myself to do this when I have time…

FWIW, I get that the point of delegators is to modify instantiation logic, but what is the primary reason that this does not apply to aliases (If anyone has the time to comment)??

[boesing]

Somewhere I pointed that out as well. AFAIR that was in some of the plugin manager issues where one changed old behavior (ValidatorManager) to ValidatorPluginManager::class but added the alias for ValidatorManager.

zendframework/zend-mvc#294

Didn't read all of the comments in that PR but I personally cannot remember on why this did not work for aliases. Maybe, we can add that missing feature tho.
Maybe worth digging deeper somewhen in the future but for now, thats a good addition.

[weierophinney]

It doesn't work on aliases because delegators are applied after the alias has already been resolved, and the alias isn't known in the scope in which the delegators are applied.

If an alias is in play, it is resolved, and then https://github.com/laminas/laminas-servicemanager/blob/3.7.x/src/ServiceManager.php#L224 is invoked - with the resolved name. It's in the doCreate() workflow that delegators are applied.

[gsteel]

It would be handy (for me at least) to have delegators applied to aliases - My use case is that I use aliases to hide an implementation like a PSR18 Http Client for example. If I want to, for example, wrap whatever client is configured in another implementation that logs requests for example, I also have to know up-front which implementation/service name I need to target in order to execute the delegator.

For now though, I just wanted to mention how it works in the docs!

[boesing]

@weierophinney we know in get context if it was an alias or not and if so, we could apply delegators and initializers based on the alias name.

[weierophinney]

@boesing correct - but that would require passing the alias to doCreate() as well. If we were to go that route, we'd have to fallback to delegators on the resolved name if no delegators were found for the alias; otherwise, we'd have a BC break (as existing behavior uses the resolved name).

[gsteel]

Once you have the resolved name and you're ready to doCreate could a step be added, before delegation occurs to look up aliases pointing at the resolved service and investigate each alias for registered delegators, compile them in a deterministic order and then apply each delegator? I expect that delegators targeting the resolved service would execute first, then those targeting aliases after (???)

[boesing]

I guess that could work, yes.
We might want to apply initializers as well.
Perhaps we can collect informations on which delegators/initializers were already applied to the service so we wont apply them more than once?

[weierophinney]

compile them in a deterministic order and then apply each delegator

Oof, I'm not sure I like that idea.

There's basically two ways to think of aliases.

1. They resolve to the exact same instance as they resolve to. In that case, attaching delegators should only happen via the resolved name, as otherwise we might end up with delegators for each alias running, which might lead to unexpected results.

2. They result in discrete instances. In that case, I would not want the delegators for a different alias or the resolved instance to run, as, just like (1) that would lead to unexpected results. Only the delegators defined for the alias should run in that case.

Right now, we are supporting (1) in laminas-servicemanager. I think what you opened in this thread is the idea of moving to (2), which would be a BC break if we make it work as I suggest.

[gsteel]

I can see that there's a lot of potential here for bc breaks and excessive complexity so I'm not advocating for any change, but the discussion is interesting.

I wouldn't like to attempt a change like this and I think it would be reasonable to assume a fair performance penalty if implemented.

I will stick to the original plan though and send in a pull to make a note in the documentation when I get a chance. 😁

[boesing]

Well, I'd love to have that feature on aliases aswell.
When I request a service via an alias, I want that all initializers and delegators being applied which might be set to that alias to avoid that somewhere, that alias is being overridden, my initializers/delegators are not applied anymore.

I have a concrete example here:

interface ClientInterface extends \Psr\Client\ClientInterface
{}

return [
    'aliases' => [
        ClientInterface::class => MyClient::class,
    ],
    'delegators' => [
        ClientInterface::class => [
            AdditionalHeadersClientDecorator::class, // <- would apply some additional headers to each request
        ],
    ],
];

^ This is actually not possible, so I add that delegator to the MyClient because the alias is pointing to that.
If someone now starts to replace my MyClient with WhateverClient and re-targets the ClientInterface::class alias, these additional headers wont get applied anymore.

---

TBH, I dont see any BC break as either doCreate and createDelegatorFromName are private within ServiceManager.
Yes, if someone declared delegators/initializers for aliases, today, none of these are applied.

We could provide a configuration toggle which enables this new behavior while make that default behavior in v4 and remove that config toggle again.

[weierophinney]

@boesing Let me make sure I understand: you want the delegators for ClientInterface to run in addition to those for MyClient? or instead of?

I ask, because the instead of part is fine, and it's something I'd support. However, it's not the current behavior; the current behavior is to run delegators and intializers only for the resolved service name, and we don't currently store separate instances for aliases versus the resolved service name. I think we could likely do something like this as a feature toggle, though, during instantiation of the SM instance. (This is easy to accomplish in Mezzio, as you would do it in the config/container.php file; in the MVC, it would likely be something you provide via the service_listener_options key of the config/application.config.php file.) I do not think it should be the default behavior. I think the current default is predictable and easy to understand, and having a toggle to switch between the behaviors would allow us to get a feel for what users actually use; if a lot of developers are turning it on, we switch default behavior in a new major version, but keep the toggle so users can toggle back to the original behavior.

The in addition to scenario would be a nightmare. The reason is because a user would need to know the entire inheritance tree in order to determine which delegators/initilizers trigger, and in which order. It would also require us to store instances per name, because otherwise you potentially have delegators on a child service operating on the same instance as a service pulled from a parent, and you get unexpected behavior. I think this approach (a) adds a ton of complexity to the implementation, (b) makes understanding the lifecycle of creating an instance too difficult, and (c) is likely to lead to a lot of WTF moments when debugging.

[boesing]

@weierophinney when alias is requested, the alias delegators + those from the service the alias is pointing to should be applied.

If the service is requested explicitly, only those from the Service should be applied.

Same for the initializers.

[weierophinney]

when alias is requested, the alias delegators + those from the service the alias is pointing to should be applied.

See my notes above; I think this will cause confusion for users. In particular, what if the expectation is that the alias delegators are the only ones applied? What if they do not want other delegators applied?

It's exactly these sorts of questions that lead to @Ocramius suggesting that aliases should be abolished entirely, in favor of just assigning a factory directly to the "alias". The benefit of that particular approach is that everything then becomes explicit: if you want both a set of delegators for the alias as well as for the resolved service name, you specify the full list to the "alias" service, and the partial list to the resolved service name, and you get two instances with two different behaviors, all done with factories:

[
    'factories' => [
        ResolvedService::class => ResolvedServiceFactory::class,
        AliasedService::class => ResolvedServiceFactory::class,
    ],
    'delegators' => [
        ResolvedService::class => [
            Delegator1ForResolvedService::class,
            Delegator2ForResolvedService::class,
        ],
        AliasedService::class => [
            Delegator1ForResolvedService::class,
            Delegator2ForResolvedService::class,
            DelegatorForAliasedService::class,
        ],
];

What's even better about the above approach is that you can explicitly state the order in which they should apply. I can choose to apply the delegators for the alias before or after those I'd normally use with the resolved service name, or intersperse between, or override completely. (This would work the same for initializers. But, honestly, initializers really aren't necessary when you have delegators available.)

Generally speaking, I think we should favor explicit definitions over implied workflows.