[WIP] Annotation support for Services #21376

weaverryan · 2017-01-23T02:00:22Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21103
License	MIT
Doc PR	TODO

Hi guys!

This is a WIP solution for adding annotations to services, and is similar to some proposals in #21103. Notice, this does NOT include a "discovery" mechanism for services: you must still register your service like normal (e.g. in YML or XML file), but then the annotations in the service are used to augment the service Definition. But, if #21289 is merged, then that'll cover automatic "discovery" for these services :).

For me, the BIG motivation is to augment autowiring, when you have just one argument that can't be autowired. Annotations read much better than, for example, setting the index "2" argument in YAML.

_defaults:
    autowire: true
    tags:
        # activates the feature
        - { name: annotated }

parameters:
    mandrill_api_key: XXXXXXXXX

services:
    App\NewsletterManager: ~
        # you could add tags, arguments or anything else here
        # these would override the annotations, if they overlap

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

/**
 * You could optionally configure any service definition options here:
 * @DI\Definition(public=false)
 */
class NewsletterManager
{
    private $logger;
    private $em;
    private $mandrillApiKey;
    private $serializer;

    /**
     * @DI\Argument(name="mandrillApiKey", value="%mandrill_api_key%")
     */
    public function __construct(LoggerInterface $logger, EntityManager $em, $mandrillApiKey)
    {
        $this->logger = $logger;
        $this->em = $em;
        $this->mandrillApiKey = $mandrillApiKey;
    }
}

Pretty much anything that you can set in YAML would be supported (most is still a TODO). The feature is currently activated by a tag... which is really ugly (though _defaults at least helps this). We could add this to the Definition object (like we did with autowire) if we want.

In #22103, there was some talk of following a Spring standard. While I think it IS important to look and learn from them, this keeps very close to our existing standard: effectively re-using all our existing terminology, but in YAML. The @Argument annotation re-uses all the options from the <argument> tag in XML service config.

Cheers!

TODOS

Add support for calls
Add support for factory, deprecated, configurator, decoratedService, autowiringTypes and autowiring/autowiringMethods
Add support for injection on properties
Add an @Tag annotation
Finish collection and iterator argument types
Handle parent class / interface annotations
Do not throw an exception when you inherit an annotation from a base class, but that arg doesn't exist on the overridden constructor (would currently throw an error)
Add a new "Resource" class so that the container cache is rebuilt (and rebuilt intelligently)
Add a lot more tests for many edge cases

dosten · 2017-01-23T02:15:22Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceAnnotationsPass.php

+     */
+    public function process(ContainerBuilder $container)
+    {
+        if (class_exists(AnnotationReader::class)) {


if (!class_exists(AnnotationReader::class)) ?

dunglas · 2017-01-23T10:01:04Z

src/Symfony/Component/DependencyInjection/Annotation/Service.php

+ */
+class Service
+{
+    private $shared;


Should we add autowired too?

Definitely - it's one of my todos :)

dunglas · 2017-01-23T10:02:37Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceAnnotationsPass.php

+        }
+    }
+
+    private function augmentServiceDefinition(Definition $definition)


It should read recursively annotations from parent classes and implemented interfaces.

dunglas · 2017-01-23T10:03:42Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceAnnotationsPass.php

+
+            $onInvalid = $arg->getOnInvalid();
+            $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE;
+            if ('ignore' == $onInvalid) {


dunglas · 2017-01-23T10:03:47Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceAnnotationsPass.php

+            $invalidBehavior = ContainerInterface::EXCEPTION_ON_INVALID_REFERENCE;
+            if ('ignore' == $onInvalid) {
+                $invalidBehavior = ContainerInterface::IGNORE_ON_INVALID_REFERENCE;
+            } elseif ('null' == $onInvalid) {


dunglas · 2017-01-23T10:08:12Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceAnnotationsPass.php

+    {
+        $arguments = array();
+        $i = 0;
+        foreach ($method->getParameters() as $parameter) {


Why not using the key instead of the $i variable?

foreach($method->getParameters() as $i => $parameter) { $arguments[$parameter->getName()] = $i; }

It should do the same thing: https://3v4l.org/Zpq4W

dunglas · 2017-01-23T10:14:01Z

I'm still not a fond of having (non-standard) annotations for DI. I've explained my reasons in depth here: #21103 (comment)

However I recognize that it is practical for parameters injection. I've proposed a less intrusive alternative in #20738, but I'm not very convinced by it either.

Maybe should we keep with the current solution for now:

_defaults:
    autowire: true

parameters:
    mandrill_api_key: XXXXXXXXX

services:
    App\NewsletterManager: ['', '', '%mandrill_api_key%'] # This is already working

ogizanagi · 2017-01-23T10:31:12Z

@dunglas :

services:
    App\NewsletterManager: ['', '', '%mandrill_api_key%'] # This is already working

This is not already working, until #21313 is merged 😄

weaverryan · 2017-01-23T15:21:38Z

@dunglas I agree with much of what you are saying :), and I see that we're both after trying to solve the "parameter" problem in some decent way. I'm also not convinced by #20738, so we could either stay with the current YAML solution or use annotations (or maybe there's a 3rd option). About your comments (#21103 (comment)), I don't see these as a significant problem. Overall, the fact that this feature would be meant for end-users - and not library authors - puts most of those concerns to rest. And in app code, coupling your code to your framework isn't a problem (actually, I see it as an advantage for speed and clarity). Also, it may help the container to be seen as simply an "internal detail" to end-users. If auto-wiring were perfect, then life would be good. But since there will always be edge-cases (parameters being the big one), with annotations, the user could avoid needing to suddenly (and strangely) go into services.yml and add a service key + arguments key (e.g. where the first two args are empty strings). Instead, they would just annotate one arg in their class :).

But, we'll see what others think - I wanted to get some real code to help with this exact discussion!

Cheers!

GuilhemN · 2017-01-23T20:27:16Z

For me, the BIG motivation is to augment autowiring, when you have just one argument that can't be autowired. Annotations read much better than, for example, setting the index "2" argument in YAML.

This made me think, can't we have named arguments in yaml? (e.g.

services:
    Foo:
        arguments:
            mandrillApiKey: %mandrill_api_key%

).

Having annotations look huge to me if this is the only motivation, don't you think ?

dunglas · 2017-01-23T20:29:18Z

@GuilhemN I've proposed this exact solution to @nicolas-grekas today and I'm working on a PR to implement it 🙌

weaverryan · 2017-01-23T20:46:50Z

@dunglas @GuilhemN I don't think I agree with that approach, as it's unexpected that changing argument names would have a side effect (this is true with annotations too, but at least the annotation & arg name are together). But, let's open that PR and decide the best solution!

dunglas · 2017-01-23T22:42:45Z

Alternative proposal: #21383

nicolas-grekas · 2017-01-24T10:43:53Z

Note that to me, this PR and #21383 do not compete. We could very well have both onboard.

nicolas-grekas · 2017-01-24T10:45:52Z

src/Symfony/Component/DependencyInjection/Compiler/ServiceAnnotationsPass.php

+            return;
+        }
+
+        $this->reader = new AnnotationReader();


what about using $container->get('annotation_reader') instead? this service is already used at compile time by some other extensions.

and it already causes issues regarding incomplete service configuration (remember the burden when switching the cache configuration in 3.2 ?). We always documented that getting service instances during a compiler pass is an unsupported usage of the container (and so you are on your own and it may break in weird ways), so we should not do it in Symfony itself (especially when it can still break because of a change done by another bundle)

I more than remember since I worked on that again in #21381.
Yet it allowed to fix a core issue, which is that there should be no cache enabled at build time.
Thus, using annotation_reader could be also seen as a way for us to support using it at compile time a first class citizen - which we have to anyway, as history proved us.

stof · 2017-01-24T11:05:05Z

src/Symfony/Component/DependencyInjection/Annotation/Service.php

+
+    private $lazy;
+
+    public function __construct(array $data)


why not removing the constructor and using public properties, letting the annotation parser validate properties itself ? This is the recommended usage of doctrine/annotations when annnotation classes don't need to be reused for other purposes (which is why we don't do in the validator component: constraints themselves are used as annotations, but they are not only annotations)

…(dunglas, nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Add support for named arguments | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | todo This PR introduces named arguments for services definitions. It's especially useful to inject parameters in an autowired service. It is (at least partially) an alternative to #21376 and #20738. Usage: ```yml services: _defaults: { autowire: true } Acme\NewsletterManager: { $apiKey: "%mandrill_api_key%" } # Alternative (traditional) syntax services: newsletter_manager: class: Acme\NewsletterManager arguments: $apiKey: "%mandrill_api_key%" autowire: true ``` ```php use Doctrine\ORM\EntityManager; use Psr\Log\LoggerInterface; namespace Acme; class NewsletterManager { private $logger; private $em; private $apiKey; public function __construct(LoggerInterface $logger, EntityManager $em, $apiKey) { $this->logger = $logger; $this->em = $em; $this->apiKey = $apiKey; } } ``` Commits ------- 8a126c8 [DI] Deprecate string keys in arguments 2ce36a6 [DependencyInjection] Add a new pass to check arguments validity 6e50129 [DependencyInjection] Add support for named arguments

weaverryan · 2017-02-13T17:02:58Z

I'll close this for now (thanks to #21383). We may still want this in the future, to avoid needing to touch a YAML file once you need to map a custom, non-autowired arg... but let's see how this all sorts out.

[WIP] Initial system for adding DI annotation support to classes

3154e12

carsonbot added Status: Needs Review Feature labels Jan 23, 2017

weaverryan added the DependencyInjection label Jan 23, 2017

weaverryan mentioned this pull request Jan 23, 2017

[RFC] Define services via annotations #21103

Closed

Thanks fabbot!

f679a2a

dosten reviewed Jan 23, 2017

View reviewed changes

dunglas requested changes Jan 23, 2017

View reviewed changes

weaverryan added 3 commits January 23, 2017 09:54

Fixing my reversed logic!

5296f01

Simply using the internal index, which is equal

c6ac896

Adding strict checking, and clear error handling

f3da69a

weaverryan force-pushed the di-annotations branch from f61962d to f3da69a Compare January 23, 2017 15:09

Thanks fabbot!

ace5106

dunglas mentioned this pull request Jan 23, 2017

[DependencyInjection] Add support for named arguments #21383

Merged

nicolas-grekas reviewed Jan 24, 2017

View reviewed changes

stof reviewed Jan 24, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Jan 24, 2017

weaverryan closed this Feb 13, 2017

weaverryan mentioned this pull request Jul 2, 2017

Autowire: Try with variable name in case of doubt #23346

Closed

nicolas-grekas mentioned this pull request May 7, 2018

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Annotation support for Services #21376

[WIP] Annotation support for Services #21376

weaverryan commented Jan 23, 2017 •

edited

Loading

dosten Jan 23, 2017

weaverryan Jan 23, 2017

dunglas Jan 23, 2017

weaverryan Jan 23, 2017

dunglas Jan 23, 2017

dunglas Jan 23, 2017

dunglas Jan 23, 2017

dunglas Jan 23, 2017

weaverryan Jan 23, 2017

dunglas commented Jan 23, 2017

ogizanagi commented Jan 23, 2017

weaverryan commented Jan 23, 2017

GuilhemN commented Jan 23, 2017 •

edited

Loading

dunglas commented Jan 23, 2017 •

edited

Loading

weaverryan commented Jan 23, 2017

dunglas commented Jan 23, 2017

nicolas-grekas commented Jan 24, 2017

nicolas-grekas Jan 24, 2017

stof Jan 24, 2017

nicolas-grekas Jan 24, 2017 •

edited

Loading

stof Jan 24, 2017

weaverryan commented Feb 13, 2017

[WIP] Annotation support for Services #21376

[WIP] Annotation support for Services #21376

Conversation

weaverryan commented Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dunglas commented Jan 23, 2017

ogizanagi commented Jan 23, 2017

weaverryan commented Jan 23, 2017

GuilhemN commented Jan 23, 2017 • edited Loading

dunglas commented Jan 23, 2017 • edited Loading

weaverryan commented Jan 23, 2017

dunglas commented Jan 23, 2017

nicolas-grekas commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Jan 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weaverryan commented Feb 13, 2017

weaverryan commented Jan 23, 2017 •

edited

Loading

GuilhemN commented Jan 23, 2017 •

edited

Loading

dunglas commented Jan 23, 2017 •

edited

Loading

nicolas-grekas Jan 24, 2017 •

edited

Loading