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

Proposal - Automatic service aliases and bindings #44184

Closed
drupol opened this issue Nov 21, 2021 · 12 comments
Closed

Proposal - Automatic service aliases and bindings #44184

drupol opened this issue Nov 21, 2021 · 12 comments

Comments

@drupol
Copy link
Contributor

drupol commented Nov 21, 2021

Description

The S.O.L.I.D. principles are a set of five design principles intended to make
software designs more understandable, flexible, and maintainable.

One of these principles is the Open-Closed Principle, which promotes the use
of interfaces instead of concrete implementations.

In Symfony, when injecting services, we usually rely on concrete service implementations
rather than using an interface. This makes our code less flexible and sometimes harder to
test.

This behavior is native in Symfony when there is only one service which implements an interface.
What then when multiple services implements the same interface? It's no more possible,
unless by adding them manually in the container.
Sadly, you have to do it manually and for each service/class.

I propose to update this mechanism and automatically creates aliases based on the discovered
services. For example, it could be enabled by adding a new property in the container
(just like the autowire and autoconfigure properties).

I created a proof-of-concept bundle that implements this and it can be found here.

The bundle implements the feature using a specific tag, there is maybe a better method
to do that, I'm open to some kind of feedback.

Feel free to let me know if this is a good or a bad idea, let's discuss about it!

@tgalopin
Copy link
Member

tgalopin commented Nov 21, 2021

Hi @drupol !

AFAIK, this behavior already happens natively: when an automatically discovered service implements an interface, the interface is aliased to this service so that you can typehint it in a different service. However, when multiple services implement the same interface, this alias isn't created automatically because it's not possible to know automatically which service to alias to.

This behavior applies only in user applications, not in vendor bundles, for performance reasons.

@drupol
Copy link
Contributor Author

drupol commented Nov 21, 2021

Hi !

Indeed, I use that behavior when there is only one service which implements an interface. However, when we have multiple services (like Doctrine repositories), it's not working, and this is why I'm opening this discussion.

I've described further the issue in the POC bundle with concrete examples.

@tgalopin
Copy link
Member

I'm not sure to understand what you are proposing. Do you mean automatically adding named autowiring aliases based on the service name?

@drupol
Copy link
Contributor Author

drupol commented Nov 21, 2021

I'm not sure to understand what you are proposing. Do you mean automatically adding named autowiring aliases based on the service name?

Yes, that's exactly that.

I'm proposing to automatically creates those bindings automatically.

@derrabus
Copy link
Member

derrabus commented Nov 21, 2021

Thank you for your proposal. I think, we can brainstorm a bit over your idea, but I don't think I like the currently proposed solution.

One of these principles is the Open-Closed Principle, which promotes the use
of interfaces instead of concrete implementations.

A common misunderstanding of this principle is that "interface" in that context means the language construct interface in PHP. That is not the case. A class exposes a public interface as well and depending on that is in many situations perfectly fine, especially if there is only one implementation or all implementations share a common parent class.

The README of your proof of concept showcases a perfect example of a class that depends on a concrete implementation even if you type-hint an interface.

The controller needs a ObjectRepository $userRepository. I could pass a HamsterRepository, PizzaRepository or SuperVillanRepository to that controller (they all implement that interface), but my attempt will probably fail badly. The UserRepository implementation is the only one that fits because it's the only one that returns User instances and that is the behavior the controller probably expects.

I'm not saying that there's no use for your idea, but for that example, the solution would be to declare UserRepository as property type. If we implement your idea, we would need a better motivation.

If I understand your bundle correctly, it "guesses" autowire aliases based on the class name, for all interfaces that class implements. I'm afraid that we will bloat our container this way. After all, not all interfaces are meant to be autowired and I really don't need hundrets autowire aliases of ServiceSubscriberInterface, LoggerAwareInterface or Countable.

@tgalopin
Copy link
Member

tgalopin commented Nov 21, 2021

I think my point of view is close to @derrabus one, but I'll say it differently to be sure: named autowiring aliases are necessary in a vendor environment in which a package needs to define several specific services (based on a specific configuration for instance) while still letting users be able to obtain them easily through autowiring. Because a vendor package cannot create classes on the fly for these services, it needs to use named autowiring aliases to "emulate" the behavior.

In a userland application, I think these specific services should have their own class and you should use them as typehint directly. I don't see any use-case where creating a new class for a specific implementation would make less sense than using a named alias.

@derrabus
Copy link
Member

derrabus commented Nov 21, 2021

Good examples of the mechanism @tgalopin describes are Symfony's cache pools or Monolog's log channels.

An application might have multiple caches and loggers. And a class that consumes a logger or a cache does not care about where the log entries and cache items are written to. It could operate on any logger or cache.

I can imagine that an application might have similar classes, but my expectation would be that those cases are pretty rare. And in that case, I think it's fair to expect the application to manually create the autowire aliases.

@GromNaN
Copy link
Member

GromNaN commented Nov 21, 2021

I think this feature is already supported when it's relevant in Symfony: there are many services that get a named autowiring alias if we look at ContainerBuilder::registerAliasForArgument usage: logger, sanitizer, http client, lock, messenger, doctrine em...

Example in symfony-demo:

$ bin/console debug:autowiring logger

Autowirable Types
=================

 The following classes & interfaces can be used as type-hints when autowiring:
 (only showing classes/interfaces matching logger)
 
 Describes a logger instance.
 Psr\Log\LoggerInterface (monolog.logger)
 Psr\Log\LoggerInterface $cacheLogger (monolog.logger.cache)
 Psr\Log\LoggerInterface $consoleLogger (monolog.logger.console)
 Psr\Log\LoggerInterface $debugLogger (monolog.logger.debug)
 Psr\Log\LoggerInterface $doctrineLogger (monolog.logger.doctrine)
 Psr\Log\LoggerInterface $eventLogger (monolog.logger.event)
 Psr\Log\LoggerInterface $htmlSanitizerLogger (monolog.logger.html-sanitizer)
 Psr\Log\LoggerInterface $mailerLogger (monolog.logger.mailer)
 Psr\Log\LoggerInterface $phpLogger (monolog.logger.php)
 Psr\Log\LoggerInterface $profilerLogger (monolog.logger.profiler)
 Psr\Log\LoggerInterface $requestLogger (monolog.logger.request)
 Psr\Log\LoggerInterface $routerLogger (monolog.logger.router)
 Psr\Log\LoggerInterface $securityLogger (monolog.logger.security)
 Psr\Log\LoggerInterface $translationLogger (monolog.logger.translation)
$ bin/console debug:autowiring sanitizer

Autowirable Types
=================

 The following classes & interfaces can be used as type-hints when autowiring:
 (only showing classes/interfaces matching sanitizer)

 HtmlSanitizer\SanitizerInterface (html_sanitizer.default)
 HtmlSanitizer\SanitizerInterface $default (html_sanitizer.default)
$ bin/console debug:autowiring entity

Autowirable Types
=================

 The following classes & interfaces can be used as type-hints when autowiring:
 (only showing classes/interfaces matching entity)
 
 EntityManager interface
 Doctrine\ORM\EntityManagerInterface (doctrine.orm.default_entity_manager)
 Doctrine\ORM\EntityManagerInterface $defaultEntityManager (doctrine.orm.default_entity_manager)

@drupol
Copy link
Contributor Author

drupol commented Nov 26, 2021

Thanks all for your valuable comments.

My approach might still be interesting for some use cases, but at this point, I think I don't need it anymore.

However, something that might be useful, would be the following:

services:
  App\Service\MyCustomManager:
    arguments:
      $services: !interface_iterator "App\\Service\\My\\Custom\\Interface"

The interface_iterator does not exist at this point, but it would be a very useful feature for me.
I currently use that feature by using a tag and I guess that implementing this would be a better feature proposal.

What do you think?

@stof
Copy link
Member

stof commented Nov 26, 2021

Well, we explicitly rejected in the past the idea of using a type directly to collect services into an iterator. Using tags to collect services, and then using _instanceof or autoconfiguration to apply tags automatically is much better:

  • _instanceof and autoconfiguration are opt-in feature, allowing config files defining services to decide whether they want to let tags be added automatically or no (for shared bundles, letting project-level rules reconfigure the bundle services is generally a support nightmare as the bug report will then apply on a different config than the one shipped in the bundle, so they will generally not opt-in for autoconfiguration)
  • tags can also be set through other means (explicitly, through compiler passes doing conditional logic), which is more flexible.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 26, 2021

There has been #41188 on the topic, which led to #41207, and both have been closed as "won't fix" for reasons listed there.

Maybe this proposal can lead to a different approach, but I'm not sure yet. Eg could an attribute help, similarly to what AsTaggedItem does for iterators/service locators? But when we have the name, which interfaces should be put in front of it to create the full alias? How do we resolve conflicts? There are many open questions.

What you can do right now is alias in services.yaml:

services:
    App\MyInterface $foo: @App\MyImplementation

This answers all the questions above :)

@drupol
Copy link
Contributor Author

drupol commented Nov 26, 2021

Thank you all once again, things are clear now.

@drupol drupol closed this as completed Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants