[policy] Service autowiring - decide approach for multiple service implementations based on the same class and different arguments

I think this could be a good idea - but is this at odds with constructor property promotion? It means that the properties would have to be called e.g. $this->cacheDefault which is an implementation detail? I suppose we could just not use property promotion for these, though, and rename the argument to a cleaner property name in the constructor.

#4 we have options here I think. Best option is, yes, to rename the property in the constructor. That would mean no #[Autowire] before the constructor argument. Then, if that could fit with property promotion/readonly, even better. If not, fall back in remapping the constructor argument to whatever is the class property in the constructor body.

let’s add a couple of examples

Filed https://github.com/mglaman/phpstan-drupal/issues/578 for mglaman/phpstan-drupal upstream.

Converted Workspaces module to autowiring, it both uses cache.default service and an implementation of the logger (logger.channel.workspaces).

I think this is perhaps OK for cache bins but I am not convinced for loggers, and still not sure this is better than using the Autowire attribute.

Given that each module needs its own logger, should we automatically register a logger service for each module, and then autowire LoggerInterface to use the module specific logger with no extra configuration?

Curious about plugin managers and loggers, are they need interfaces?

Made a MR for File Metadata Manager → , 📌 More service autowiring Fixed , to see how this would look in contribland.

A possible alternative for loggers is "bind":
https://symfony.com/doc/current/service_container.html#binding-arguments...
This works for cases where all the services in one yml file should use the same logger. So mostly for module services.yml files but not for core.services.yml.

services:
    _defaults:
        bind:
            # pass this service for any LoggerInterface type-hint for any
            # service that's defined in this file
            Psr\Log\LoggerInterface: '@logger.channel.mymodule'
    logger.channel.mymodule:
        parent: logger.channel_base
        arguments: ['mymodule']

But the aliases are also ok, and work better in core.services.yml.

----

In symfony you can also pass "complex expressions" as arguments:
https://symfony.com/doc/current/service_container/expression_language.html
Last time I checked this was not possible in Drupal.

This would be a solution for entity storage parameters.
arguments: ['@=service("entity_type.manager").getStorage("user")']

In a local experiment I did something with a factory service, and a parameter attribute that would automatically register the correct argument based on the interface and a value:

class EntityTypeManager {
  [..]
  /**
   * Registers a service '...\EntityStorageInterface.factory',
   * as a callable wrapper for this method.
   */
  #[ServiceFactory]
  public function getStorage(string $entity_type_id): EntityStorageInterface {..}
  [..]
}

class MyCustomService {
  public function __construct(
    /**
     * Registers an argument expression that calls the '...\EntityStorageInterface.factory' service with 'user'.
     */
    #[ServiceFromFactory('user')]
    private EntityStorageInterface $userStorage,
  ) {}
}

With this mechanism we would not need to register all the separate services.

But I don't know how significant is the overhead from registering more services.
Clearly the memory usage and the time to load the cached container will go up, but perhaps not by a lot.

We should stop injecting storage as possible, moreover it's issue for serialization, forms plugins so there's #3309369: Remove special-casing of entity storages in container serialization →

@donquixote I forgot about bind, I like that idea much better for loggers at least. As far as I can see this isn't implemented in our container, I think we should open a separate issue to explore adding this feature.

For caches I think we are choosing between:

1. #[Autowire(service: 'cache.default')]
   protected CacheBackendInterface $cache,
   

2. protected CacheBackendInterface $cacheDefault,
   

3. CacheBackendInterface $cacheDefault,
   ...
   $this->cache = $cacheDefault
   

I am still leaning towards option 1 here as I find the other two slightly more ugly. Many users should already recognise the cache.default service, and the explicit attribute seems easy to understand when porting from existing services.yml files, whereas the magic variable name $cacheDefault is new syntax, and another mapping to learn. I am open to arguments in other directions though :)

@longwave
I like #[Autowire(service: 'cache.default')], but I think this is mostly because the service name is so short, memorable and obvious. I am happy if we go for that for cache.

My second proposal might still be interesting for other services with more obscure names.
Perhaps these would be mostly in contrib, because core will grab the short service names, whereas contrib wants to avoid name clashes.

If we applied my second proposal to cache, it would look something like #[NewAutowireAttribute(bin: 'default')] or #[AutowireArgument('default')].
So you only have to specify the argument that distinguishes this cache from other caches, and the rest is coming from the interface.
But again, cache is not the most convincing example to justify this mechanism.

Having said that Symfony is designed to be flexible, and Drupal could, if we wanted, be more opinionated - should we automatically provide loggers for all modules?

My 0.02: no. It would mean assuming that every module needs one logger, no more no less, and kinda forcing using that logger only. For me these are too strong assumptions.

I disagree with the forcing - by default I think it would be nice if you could typehint LoggerInterface and get a logger automatically injected and configured for your module without having to manually declare a logger channel service. If these automatically declared loggers were private, they should be removed from the container if they are not actually wired to anything (although this is more difficult if we want to autowire plugins as well). And nothing should stop you explicitly wiring a different logger or using bind to bind a different one by default?

This stuff is above my head but #14 I would vote for the option that will be common in other scenarios. We are talking about cache but if there are other things to autowire can all 3 of those approaches be used?

Marking to get into the committers bucket.

Before this is merged, can we summarize in "Proposed resolution"?

Yes please.

Yes this needs an issue summary before it should be RTBC. Also there's a patch here but the title says policy.

mondrake → closed merge request !4200

Done IS update, closed the MR.

I think the proposal is good and had no idea it was possible.

I left one comment on the MR before realising it was just an example and already closed.

However, I think we need to document this somewhere - should it be added to a CR as an informational thing, or just to documentation? And we should have follow-ups to use the pattern for core, I guess based on the MR here. Moving to needs review since these are metadata/admin things not changes to the actual proposal.

I've just seen 📌 Support 'bind' in service container RTBC which seems like the 'official' way to do it, if that's the case, is this needed? I think we need to justify having two ways to do this.

Looks like the direction now is to use PHP attributes to disambiguate service arguments directly in the service constructors. Won’t fix?

Brought this up in #needs-review-queue-initative channel

And it was suggested to postpone while other autowire tickets are worked on.

Just searching for "autowire" got a number of still in progress issues →