Existing situation
Currently, services provided by a package (module) are registered in a services.yml file shipped with the module.
This yml file also controls the dependencies of each service.
Pros:
- One central place (per module) to see a list of all provided services.
Cons:
- To create/write a service, you need to edit two places.
There are also some things that need services from the container, but do not really need to be exposed as services themselves:
Plugins, page controllers. And hook implementations - see #2237831: Allow module services to specify hooks.
Proposal
Allow services and/or service consumers to declare their dependencies with annotations.
We could come up with a new annotation syntax, but for now I want to keep it simple and propose the same Doctrine annotation syntax as we already use for plugins.
namespace Drupal\forum;
/**
* @Service(
* id = "forum.uninstall_validator",
* arguments = (
* "@entity.manager",
* "@entity.query",
* "@config.factory",
* "@string_translation"
* ),
* lazy = true
* )
*/
class ForumUninstallValidator {
[..]
}
Annotations can be on classes, or on (static) factory methods.
We can then convert these annotations into service definitions.
Consequences for services
For services, this is mostly *equivalent* with the existing services.yml declaration:
In either case, the definitions are controlled/owned by the package/module that provides the service, and in either case they can be altered by other modules.
The only difference is the pro/con mentioned above, which is now turned around. And added time/cost to scan the class files.
Pros:
- Only one file to edit, when creating a service.
Cons:
- No more central place (per module) to see a list of available services.
- Added time/cost to scan class files and look for annotations.
- Some people think that annotations are bad and/or ugly.
Annotated service consumers
This can be a solution to avoid dealing with the container directly.
namespace Drupal\node\Controller;
/**
* Returns responses for Node routes.
*
* @Arguments(
* '@date.formatter',
* '@renderer',
* )
*/
class NodeController extends ControllerBase implements ContainerInjectionInterface {
[..]
public function __construct(DateFormatterInterface $date_formatter, RendererInterface $renderer) {
[..]
}
[..]
}
Annotated plugins
Since plugins already have annotations, it would not be a big change to also annotate the service dependencies - instead of dealing with the container in a static method.
Parameter currying
We could also mix service arguments into other signatures..
/**
* Implements hook_foo_alter()
*
* @Arguments(
* 1 = '@renderer',
* )
*/
function mymodule_foo_alter(&$foo, RendererInterface $renderer) {
[..]
}
This would fix the second parameter, and leave a curried callable with only one free parameter left.
One problem with this is that mymodule_foo_alter() no longer matches the generic signature of e.g. hook_foo_alter(). So we could no longer use stuff like the proposed @implements or @signature tag .. https://github.com/phpDocumentor/phpDocumentor2/issues/1689
Comments
Comment #2
dawehnerSo mh, this would be another way of doing things. Basically all places which would leverage that functionality currently uses the static factory pattern. There is at least some form of standard already.
Comment #3
bojanz commentedPlease no. It's time we stopped abusing annotations.
Many classes using an annotation for discovery + static factory pattern could actually move to being registered in services.yml. I'd like that direction to happen more than the opposite.
Comment #4
donquixote commented@bojanz: What is your personal reason to dislike annotations?
I have read different opinions about this in the past. Some people don't like the syntax, or the discovery process. Some don't like the entire concept of having stuff i the doc comment that is used for something other than documentation.
Personally I see some pros and cons, but currently don't have a strong opinion either way.
And, would you prefer annotation with annotated dependencies instead of annotation + static method with container parameter?
E.g. if you have the choice between this..
.. and this ..
Comment #5
dawehnerMy personal reasons why annotations are a bad thing here:
* It is yet another pattern
* If you need to debug stuff its potentially one more level of things you need to deal with
* You get potentially easier autocompletion support, at least container services autocompletion works for me inside create(), not sure about annotaitons.
Comment #6
Crell commented-1. One of the key reasons for doing proper DI is to decouple code from the framework. Setting up your dependencies with an annotation presumes the framework, the DI Container, and the specific service names. Yes, you could futz with configuration to change that elsewhere but now your annotation is lying to you.
Are we really that scared of 2 files? :-)
Comment #7
donquixote commented@Crell:
The annotation is as much a part of the package/module as the services.yml file.
Both of them presume the service names.
Neither of them explicitly presumes the framework or the container.
They simply presume that there is some mechanism that will provide a service for a given service id.
If you need to alter the info from the annotation, you would also need to alter the info from the services.yml.
Of course it is still legitimate to dislike this idea, but imo this is more a stylistic and DX question than anything else. And performance and complexity of the container mechanism.
Comment #8
dawehnerThese are mostly compiler time costs, so potentially it doesn't matter much. Annotations might be okay for custom code but its serious smell IMHO for generic stuff.
Comment #9
benjy commentedI'm not a huge fan of annotations, they're harder to write than YML for me and I really have no issues with the two files, the consolidated list for a module is actually quite handy.
As bojanz said, I think moving the other way would be better. We could have also standardised core on yml discovery/definition for plugins for further consistency.
Comment #10
donquixote commentedI had some situations where I was creating, renaming and deleting classes a lot, until I was finally happy.
In these cases, the separate files would have been annoying to keep in sync.
Otherwise, maybe they are not such a big deal.
Comment #11
dawehnerWell at least IDEs like storm should help you with that.
Comment #12
twistor commentedHow is this any different from a factory method?
We've been using something like this for Feeds plugins and it works quite well. The only trouble is that subclasses need to copy the dependency annotation. This can probably be solved by introspection, but it's tricky and I haven't cared to yet.
Comment #14
donquixote commentedOr in case of a module-provided services.yml file (for a module-provided service), the services.yml would be lying to you.
Or in case of a static factory (for a module-provided plugin) the only way is a container adapter. So now the container exposed to the static method is lying to you.
If we sell the annotation or services.yml dependency setting as a suggestion only, I see no problem with "lying".
Comment #15
donquixote commentedBtw,
https://twitter.com/BartFeenstra/status/710405226537291776
https://www.drupal.org/project/retriever
https://github.com/bartfeenstra/dependency-retriever
Comment #16
xanoGuilty as charged!
This excellent quote from @bojanz got me thinking:
Symfony's service container isn't just a service container. It contains its own factory as well. While not all classes in our code base are services, having non-PHP instructions on how to instantiate them is useful. I wrote https://www.drupal.org/project/retriever with the idea of annotations in mind (as bad as they sometimes are, sticking metadata to the class in question is such a useful thing), but the codebase is written in such a way that it could also take DI suggestions from a YAML file, for instance. All it needs is a different implementation of the
Finderinterface.Comment #21
andypostIs this issue still makes sense?
Comment #22
cilefen commentedhttps://symfony.com/doc/current/service_container/3.3-di-changes.html
Comment #29
bradjones1Given the age of this issue and more general efforts such as #3021900: Bring new features from Symfony 3/4/5/6/7 into our container, I think #3221128: Enable service autoconfiguration for core event subscribers is probably a better way to get at this type of functionality.
Comment #30
bradjones1Comment #34
longwaveSymfony now supports this with the #[Autowire] attribute which is undoubtedly a nicer syntax than annotations, closing this as a duplicate of #3295751: Autowire core services that do not require explicit configuration
Comment #35
donquixote commentedTrue, autowire is the way to go!
Although that alone does not give us full discovery of services.