[RFC] Define services via annotations #21103

javiereguiluz · 2016-12-30T09:11:09Z

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	yes
Symfony version	3.3

Context

One of the goals of Symfony 3.3 is to make the DependencyInjection configuration simpler for newcomers. In Symfony we already have service autowiring and soon we could also have getter injection.

Proposal

However, in my opinion they are missing something. They try to reduce the config needed to define services ... but maybe we should instead remove the config needed to define services. We've done this multiple times in Symfony:

@Route removes the need for routing.yml | .xml | .php
@Assert removes the need for validation.yml | .xml | .php
@ORM removes the need for Entity.orm.yml | .xml

So, why not define a new @Service annotation to remove the need for services.yml | .xml.

In practice

It would be very similar to JMSDiExtraBundle. You can configure the services in the PHP file of the related class, without having to define any file.

Alternative 1

use Doctrine\ORM\EntityManager;
use Psr\Log\LoggerInterface;

/** @Service('app.manager.newsletter') */
class NewsletterManager
{
    private $logger;
    private $em;

    /** @InjectParams */
    public function __construct(LoggerInterface $logger, EntityManager $em)
    {
        $this->logger = $logger;
        $this->em = $em;
    }

    // ...
}

Alternative 2

use Doctrine\ORM\EntityManager;
use Psr\Log\LoggerInterface;

/** @Service('app.manager.newsletter') */
class NewsletterManager
{
    /** Inject('logger') */
    private $logger;
    /** Inject('doctrine.orm.entity_manager') */
    private $em;

    // ...
}

Alternative 3 (PHP 7+)

use Doctrine\ORM\EntityManager;
use Psr\Log\LoggerInterface;

/** @Service('app.manager.newsletter') */
class NewsletterManager
{
    /** Inject */
    protected function getLogger(): LoggerInterface;

    /** Inject */
    protected function getEm(): EntityManager;

    // ...
}

Alternative 3 (PHP 5)

use Doctrine\ORM\EntityManager;
use Psr\Log\LoggerInterface;

/** @Service('app.manager.newsletter') */
class NewsletterManager
{
    /** Inject('logger') */
    protected function getLogger();

    /** Inject('doctrine.orm.entity_manager') */
    protected function getEm();

    // ...
}

Comparison

For comparison purposes, this is the traditional Symfony service configuration (which requires 1 PHP file + 1 config file):

use Doctrine\ORM\EntityManager;
use Psr\Log\LoggerInterface;

class NewsletterManager
{
    private $logger;
    private $em;

    public function __construct(LoggerInterface $logger, EntityManager $em)
    {
        $this->logger = $logger;
        $this->em = $em;
    }

    // ...
}

services:
    app.manager.newsletter:
        class: AppBundle\Manager\NewsletterManager
        arguments: ['@logger', '@doctrine.orm.entity_manager']
        # ...

And this is the autowiring configuration, which also requires 1 PHP file + 1 config file and the work to do is almost the same as before:

use Doctrine\ORM\EntityManager;
use Psr\Log\LoggerInterface;

class NewsletterManager
{
    private $logger;
    private $em;

    public function __construct(LoggerInterface $logger, EntityManager $em)
    {
        $this->logger = $logger;
        $this->em = $em;
    }

    // ...
}

services:
    app.manager.newsletter:
        class: AppBundle\Manager\NewsletterManager
        autowire: true
        # ...

Mandatory reminder for anything related to autowiring: this feature would be 100% optional and it won't have any impact in your application if you don't want to.

The text was updated successfully, but these errors were encountered:

linaori · 2016-12-30T09:39:59Z

DX wise it might seem nice, but it will also allow people to write a lot of code in a bad way, making it harder for them to decouple it at a later stage. It will surely be nice for new developers as they will have less files to configure, but introduces a path they will go down on which will make them think less in terms of libraries and will start seeing classes as services even more.

Services are nothing but a reference to something in a container and people often refer to their class as a service. This feature would make that line even thinner and I'm afraid it would confuse developers even more.

GuilhemN · 2016-12-30T09:41:25Z

At this point, what is the benefit against DunglasActionBundle ?
It would add a lot of complexity for a very small gain imo.

stof · 2017-01-01T12:51:11Z

Your second alternative does not work, as the container cannot inject dependencies into private parameters.

Your third and fourth alternatives depend on #20973 (and possibly on #21031), which should be clarified, as it is not yet accepted.

@GuilhemN DunglasActionBundle is about defining controllers differently. This is about services (which might not be controllers themselves)

@javiereguiluz there is one thing to consider there though: defining your services by annotations on the source code directly means that your entire source code now needs to be checked when rebuilding the container. This can have a very negative effect on DX if the container cache is considered as invalidated each time you edit your source code (we used to have this concern with autowired classes at the beginning btw). The implementation must be done in an efficient way (but invalidating the cache when you add the @Service annotation in a class not using it).
Btw, do we even need /** @InjectParams */ on the constructor ? Can we mark the service as autowired instead (maybe with a property in the @Service annotation to toggle setter and getter autowiring) ?

GuilhemN · 2017-01-01T12:55:46Z

@stof it is its primary goal but it also works well for other services.
ping @dunglas

dknx01 · 2017-01-02T08:05:13Z

Some weeks ago we had such a kind of discussion in our local user group (with the JMSDiExtraBundle).
Most of us don't like the idea very much, because you must look inside (each?) PHP file to find the definition. If you use the services.yml|xml|php file it is easier to find it or at least to know where to look. Also to change the service definition in your local copy or project is easier.

Beside of this I don't think it is really much easier for newcomers. I think it is easier for them to say "I must have a look into a services.yml|xml|php file. Or search through them."

Nicofuma · 2017-01-02T08:10:18Z

@stof actually, it is possible to inject dependencies into private properties if the class isn't private, just use an approach similar to what @nicolas-grekas did in #20973

theofidry · 2017-01-03T01:10:16Z

I'm 👍 for the first alternative, I'm not fond of the feature itself but it would have the merit to be more consistent with the annotations system elsewhere.

I would also add:

namespace App;

/** @Service */
class NewsletterManager
{
    //...
}

which would be equivalent to:

namespace App;

/** @Service('App\NewsletterManager') */
class NewsletterManager
{
    //...
}

As it would be more in line with #21133 and auto-wiring.

@javiereguiluz any plan for other services parameters, e.g. auto-wired, private & co.?

dunglas · 2017-01-03T07:35:39Z

I'm not sure that promoting the use of annotations for service definitions is a good idea:

It encourages to couple the domain (services) with the framework (which provides annotations)
It makes the usage of internal code (with annotations) different than external random libs (without annotation)
It breaks the principle of a container dealing with any class from the outside, the idea we try to push with https://github.com/symfony/symfony/projects/2 is that the container becomes (as much as possible) a framework's internal that the user doesn't have to care about, pushing annotations breaks this approach
What if a lib creator want to support several containers? He'll need to add every annotation provided by the different containers?

IMO, the PHP syntax should be sufficient to specify dependencies of a class (and it's what we do with autowiring, in most cases type hints are enough to inject the good service).
If we really want to be able to use annotations to do that, I think that it should be a standard one like JSR 330 in Java (javax.inject), not something specific to Symfony.
Should we propose a PSR or something like that? I'm not sure it's worth it, but it will be better than creating our own stuff.

Hanmac · 2017-01-03T08:58:03Z

@theofidry: i would like it if without name,
'AppBundle\Service\NewsletterManager' would be turned into "app.newsletter_manager" for example.
it would make it betterr looking compared to the other service names.

stof · 2017-01-03T09:06:23Z

@stof actually, it is possible to inject dependencies into private properties if the class isn't private, just use an approach similar to what @nicolas-grekas did in #20973

No you cannot. The only way to access private properties is from the class itself (out of our control) or through Reflection (which is bypassing the API of the class, and so a no-go IMO).
#20973 relies on protected visibility, and on methods which allow to hook into their access. A protected property without any setter would require some magic to initialize it (and would mean the class is unusable standalone)

stof · 2017-01-03T09:10:24Z

What if a lib creator want to support several containers? He'll need to add every annotation provided by the different containers?

@dunglas No, that would be bad as the annotation parser breaks on unknown annotations, which would force to install all containers all the time.
If you want to support multiple containers, the right way is to keep the container configuration in separate files, which is what we already allow in XML and YAML today.
Annotations are not a good fit for packages wanting to have optional deps (this is one of the reasons why FOSUserBundle does not use annotations for the ORM and ODM mapping btw)

IMO, the PHP syntax should be sufficient to specify dependencies of a class (and it's what we do with autowiring, in most cases type hints are enough to inject the good service).

that's kind of the reason why I asked whether @InjectParams was really required. IMO, as single @Service annotation relyinh on autowiring should be enough if we go this way.

maryo · 2017-01-03T09:24:32Z

I agree with @dunglas. Coupling domain objects with ORM is quite OK. You can hardly switch ORM. In case of validation it's mostly OK. The objects are mostly coupled with the validators. In case of routing it's worse but controllers are mostly container aware and depend on Symfony services because it's comfort so it's acceptable tradeoff for me.

But in case of DI... Many if not most classes are reusable and framework independent. Actually DI container independent.

Nicofuma · 2017-01-03T09:34:43Z

Imho, the question of defining services through annotation is not whether or not it is a good idea for libs and bundles to uses them because the answer is no. For me, the libs and bundle question is irrelevant and the only question to ask is whether or not it is useful for the services you write in your application and can make DI usage easier and faster (don't forget: your application is already coupled to the framework, you won't change it easily). And if I don't really like it for some reasons, I think it is useful and makes a lot of sense with autowiring and the direction DI is taking.

hhamon · 2017-01-03T09:42:16Z

I'm really not a big fan of configuring services with annotations. It seems to be simplier at first sight but it adds a lots of magic from a DX point of view. To me it's even more longer and complicated to write than a simple YAML file.

mnapoli · 2017-01-03T12:05:39Z

Hey @dunglas, there is a start of a discussion of a PSR/standard of @Inject (and related annotations) here: container-interop/container-interop#27 (several years old 😉) Right now the best option would probably be to bring it directly to the FIG though. I still think something like JSR-330 could be useful for when annotations are used (which is not always the case).

dunglas · 2017-01-03T12:26:48Z

What would be a nice first step however is to have native annotations in PHP...

linaori · 2017-01-03T12:39:50Z

I don't know why we still don't have those :/

Hanmac · 2017-01-04T09:46:39Z

i don't know if that does target this issue too, but i would like to hear your guys opinions about it.

i often do this in my bundles because i need some object repositories in my other services:

    app.base_repository:
        abstract:  true
        class: Doctrine\ORM\EntityRepository
        factory: ['@doctrine.orm.entity_manager', getRepository]

    app.xyz_repository:
        parent: app.base_repository
        arguments:
            - AppBundle\Entity\Xyz

whould such a @Service anotation work for this too?
hm or maybe that should be done as param for @ORM anotations?

or would some kind of dynamic Service Provider that does look for "app.%s_repository" help?

stof · 2017-01-04T09:53:42Z

The annotation would not help here as you need a factory.

Btw, be careful that defining the doctrine repository as a service is incompatible with resetting the entity manager (as the repository would not be replaced by the new repository created by the new entity manager), so I would advice against this.

linaori · 2017-01-04T09:59:21Z

@stof if you have to reset your entity manager, you probably have a bigger issue. I always recommend that if the entity manager is closed, it thus crashed somewhere and it's not safe to continue the normal flow.

stof · 2017-01-04T10:02:29Z

@iltar there is a very valid use case for resetting the manager: long-running process where you want to be able to process the next message in the queue even if the processing of the previous message failed (having to make your consumer process crash all the time is not good).

linaori · 2017-01-04T10:06:03Z

@stof and what about all the (now) un-managed entities floating around everywhere and nowhere? If you try to flush an entity used by another uow, it will most likely cause a bunch of issues and there are probably a dozen more side effects. However, this is slightly going off-topic.

My opinion on service annotations in a class? Bad idea for the aforementioned reasons, I don't think it's worth it. I think autowiring is a better path to go down on and is not too complex to add features to.

stof · 2017-01-04T10:07:38Z

@iltar I'm also clearing the entity manager between processing of different queue messages anyway (otherwise I would be leaking memory). But anyway, this is off-topic for this issue.

NavyCoat · 2017-01-05T14:17:54Z

@route removes the need for routing.yml | .xml | .php
@Assert removes the need for validation.yml | .xml | .php
@Orm removes the need for Entity.orm.yml | .xml

Alright. I can understand Annotations for Asserts. Yeah, they done lot of nice job. In my, opinion, code looks better, because assert statement is above statement and for developer is easier to read.

Even for ORM, annotations looks barely good enough, but this often generate lot of them.

To this day, I was thinking, that Route annotations are the most... stupid. You have to read every controller to find routes to your commands...
But defining services, in annotations? I think that only look deceptively simple.

Guikingone · 2017-01-06T09:50:34Z

Not a big fan of annotations, if we inject the service using annotations, we link the logic of services to the framework and the class become dependant of the framework logic, i support the vision @dunglas on this part.

Same thinking about getters injection, this way, the logic of the class become dependant of the framework component, what if we want to inject some classes coming from Zend ? Or just moving our service into Zend ?

I always inject using the constructor, this way, if i need to change the dependencies, i just change the constructor.

Nicofuma · 2017-01-06T10:33:09Z

Lets be honest, you don't change the framework of your project every morning. And when you do change the foundation of your project (yes,, the framework is the foundation of the project) it's a massive task and 2 annotations won't change that much especially if the annotation is only used with autowiring (to not have to write the same 4 lines of yaml for every service)

Guikingone · 2017-01-06T10:37:23Z

For sure, it's just about linking the logic to the framework, not complaining about the changement of framework on a project, i don't change the foundation every morning.

javiereguiluz · 2017-01-06T10:41:50Z

Just for reference, this is how autowiring works in Spring Framework ... and still most developers consider that framework professional, great, etc.

// setter injection
public class Customer
{
    // ...

    @Inject
    public void setPerson(Person person) {
        this.person = person;
    }
}

// Constructor injection
public class Customer
{
    @Inject
    public Customer(Person person) {
        this.person = person;
    }

    // ...
}

// Property injection
public class Customer
{
    @Inject
    private Person person;
    // ...
}

The Symfony autowiring feels like "half-done": you want to go fast with autowiring ... but you still need to waste your time with config files. If you want autowiring, you shouldn't have to deal with that.

dunglas · 2017-01-06T10:46:03Z

@javiereguiluz it will not be the case anymore if/when we'll integrate ActionBundle.

dunglas · 2017-01-06T10:47:24Z

About Spring, @Autowired is a proprietary and deprecated annotation, for new projects, the standard @Inject annotation (JSR 330) must be used instead.

ianfp · 2017-01-08T21:46:11Z

I'm in favour of this proposal. I have a project that is very heavy on server-side processing (not a simple CRUD REST backend) with lots of services, and I'm finding that maintaining both YAML files and the classes is becoming a hassle. I'm trying out JMSDiExtraBundle and I think it's an improvement, but this proposal seems to have a simpler syntax, which I like.

Specifically, I don't like complex nested annotations because my IDE (PhpStorm) can't syntax-check them:

/**
 * @InjectParams({
 *    "preferred" = @Inject("preferred_strategy"),
 *    "fallback" = @Inject("fallback_strategy"),
 * })
 */
public function __construct(Strategy $preferred, Strategy $fallback)

In cases where type-hints are not sufficient (eg, when autowiring doesn't work), why not something more like this:

/**
 *  @Inject("preferred_strategy", param="preferred")
 *  @Inject("fallback_strategy", param="fallback")
 */
public function __construct(Strategy $preferred, Strategy $fallback)

weaverryan · 2017-01-23T02:04:09Z

I've added a possible implementation at #21376

javiereguiluz · 2017-02-17T15:28:50Z

Closing because the related PR (#21376) was closed as "won't fix" and we're exploring other ways to make the DI better.

javiereguiluz added DependencyInjection Feature RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Dec 30, 2016

weaverryan mentioned this issue Jan 23, 2017

[WIP] Annotation support for Services #21376

Closed

javiereguiluz closed this as completed Feb 17, 2017

dunglas mentioned this issue Feb 26, 2017

[DI] Autowire arguments that are container parameters #21699

Closed

chalasr mentioned this issue Aug 2, 2017

[DX] Service declaration annotation #23758

Closed

linaori mentioned this issue Jan 17, 2018

[DI][Proposal] Annotation-based autowiring #25810

Closed

linaori mentioned this issue Apr 13, 2018

[RFC] - Not having constructors for service classes with annotations & traits #26915

Closed

nicolas-grekas mentioned this issue May 7, 2018

[DI] Extend semantic of @param to describe intent and use it when autowiring services and parameters #27172

Closed

wouterj mentioned this issue Jan 19, 2021

[DependencyInjection] Configure service tags via attributes #39776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] Define services via annotations #21103

[RFC] Define services via annotations #21103

javiereguiluz commented Dec 30, 2016 •

edited

Loading

linaori commented Dec 30, 2016 •

edited

Loading

GuilhemN commented Dec 30, 2016

stof commented Jan 1, 2017 •

edited by javiereguiluz

Loading

GuilhemN commented Jan 1, 2017

dknx01 commented Jan 2, 2017

Nicofuma commented Jan 2, 2017

theofidry commented Jan 3, 2017 •

edited

Loading

dunglas commented Jan 3, 2017 •

edited

Loading

Hanmac commented Jan 3, 2017

stof commented Jan 3, 2017

stof commented Jan 3, 2017

maryo commented Jan 3, 2017 •

edited

Loading

Nicofuma commented Jan 3, 2017 •

edited

Loading

hhamon commented Jan 3, 2017

mnapoli commented Jan 3, 2017

dunglas commented Jan 3, 2017

linaori commented Jan 3, 2017

Hanmac commented Jan 4, 2017

stof commented Jan 4, 2017

linaori commented Jan 4, 2017

stof commented Jan 4, 2017

linaori commented Jan 4, 2017

stof commented Jan 4, 2017

NavyCoat commented Jan 5, 2017 •

edited

Loading

Guikingone commented Jan 6, 2017

Nicofuma commented Jan 6, 2017

Guikingone commented Jan 6, 2017

javiereguiluz commented Jan 6, 2017 •

edited

Loading

dunglas commented Jan 6, 2017

dunglas commented Jan 6, 2017 •

edited

Loading

ianfp commented Jan 8, 2017

weaverryan commented Jan 23, 2017

javiereguiluz commented Feb 17, 2017

[RFC] Define services via annotations #21103

[RFC] Define services via annotations #21103

Comments

javiereguiluz commented Dec 30, 2016 • edited Loading

Context

Proposal

In practice

Alternative 1

Alternative 2

Alternative 3 (PHP 7+)

Alternative 3 (PHP 5)

Comparison

linaori commented Dec 30, 2016 • edited Loading

GuilhemN commented Dec 30, 2016

stof commented Jan 1, 2017 • edited by javiereguiluz Loading

GuilhemN commented Jan 1, 2017

dknx01 commented Jan 2, 2017

Nicofuma commented Jan 2, 2017

theofidry commented Jan 3, 2017 • edited Loading

dunglas commented Jan 3, 2017 • edited Loading

Hanmac commented Jan 3, 2017

stof commented Jan 3, 2017

stof commented Jan 3, 2017

maryo commented Jan 3, 2017 • edited Loading

Nicofuma commented Jan 3, 2017 • edited Loading

hhamon commented Jan 3, 2017

mnapoli commented Jan 3, 2017

dunglas commented Jan 3, 2017

linaori commented Jan 3, 2017

Hanmac commented Jan 4, 2017

stof commented Jan 4, 2017

linaori commented Jan 4, 2017

stof commented Jan 4, 2017

linaori commented Jan 4, 2017

stof commented Jan 4, 2017

NavyCoat commented Jan 5, 2017 • edited Loading

Guikingone commented Jan 6, 2017

Nicofuma commented Jan 6, 2017

Guikingone commented Jan 6, 2017

javiereguiluz commented Jan 6, 2017 • edited Loading

dunglas commented Jan 6, 2017

dunglas commented Jan 6, 2017 • edited Loading

ianfp commented Jan 8, 2017

weaverryan commented Jan 23, 2017

javiereguiluz commented Feb 17, 2017

javiereguiluz commented Dec 30, 2016 •

edited

Loading

linaori commented Dec 30, 2016 •

edited

Loading

stof commented Jan 1, 2017 •

edited by javiereguiluz

Loading

theofidry commented Jan 3, 2017 •

edited

Loading

dunglas commented Jan 3, 2017 •

edited

Loading

maryo commented Jan 3, 2017 •

edited

Loading

Nicofuma commented Jan 3, 2017 •

edited

Loading

NavyCoat commented Jan 5, 2017 •

edited

Loading

javiereguiluz commented Jan 6, 2017 •

edited

Loading

dunglas commented Jan 6, 2017 •

edited

Loading