[DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention #22060

nicolas-grekas · 2017-03-19T22:34:47Z

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

This PR adds a new autowiring mode, based only on the class <> id convention.
This way of autowiring is free from any conflicting behavior, which is what I was looking for to begin with.

The expected DX is a bit more involving than the current way we do autowiring. But it's worth it to me, because it's plain predictable - a lot less "magic" imho.

So in this mode, for each App\Foo type hint, a reference to an "App\Foo" service will be created. If no such service exists, an exception will be thrown. To me, this opens a nice DX: when type hinting interfaces (which is the best practice), this will tell you when you need to create the explicit interface <> id mapping that is missing - thus encourage things to be made explicit, but only when required, and gradually, in a way that will favor discoverability by devs.

Of course, this is opt-in, and BC. You'd need to do eg in yaml: autowire: by_id.
For consistency, the current mode (autowire: true) can be configured using autowire: by_type.

theofidry · 2017-03-20T01:04:37Z

cool, will try it during the week/weekend!

javiereguiluz · 2017-03-20T08:08:26Z

Could you please show a before/after comparison of what the user has to do to use this feature? Or some small but complete code example? Otherwise it's hard to follow :( Thanks!

GuilhemN · 2017-03-20T08:23:55Z

src/Symfony/Component/DependencyInjection/Definition.php

        return $this->autowired;
    }

    /**
     * Sets autowired.
     *
-     * @param bool $autowired
+     * @param int $autowired


int|false?

int only, even if bools are accepted, that's only "by cast", which is fine even on php 7.

Imo we should either explicitly allow false or add a new constant, it feels weird to call $definition->setAutowired(0);.

on php 7, adding just int would be fine, no BC break (in terms of behavior, not signature of course), and even then, false would be accepted.

turned to bool|int

nicolas-grekas · 2017-03-20T22:56:47Z

I played with this PR on symfony-demo, I really like the taste it provides :)
I found some enhancements for exception messages: these now provide suggestions when the corresponding type is not found as an id.

A typical example is this one, displayed while building an incompletely defined container:

  [Symfony\Component\DependencyInjection\Exception\RuntimeException]           
  Cannot autowire argument $manager of method AppBundle\Form\Type\TagsInputTy  
  pe::__construct() for service "AppBundle\Form\Type\TagsInputType": Service   
  "Doctrine\Common\Persistence\ObjectManager" does not exist and it cannot be  
   auto-registered. This type could be aliased to the existing "doctrine.orm.  
  default_entity_manager" service.

The resolution is easy, just add the following to services.yml:

    Doctrine\Common\Persistence\ObjectManager:
        '@doctrine.orm.default_entity_manager'

Of course, this alias should be provided by default by DoctrineBundle. By doing so, we would fix "by-id" autowiring immediatly, but we would also prevent any ambiguous "by-type" autowiring - by stating that "doctrine.orm.default_entity_manager" is the service to use by default when the "Doctrine\Common\Persistence\ObjectManager" type is wired.

A possible working resulting services.yml file for symfony-demo could be the following:

services:
    _defaults:
        public: false
        autowire: by_id

    _instanceof:
        Symfony\Component\EventDispatcher\EventSubscriberInterface:
            tags: [ kernel.event_subscriber ]

        Symfony\Component\Form\AbstractType:
            tags: [ form.type ]

        Twig_Extension:
            tags: [ twig.extension ]

    AppBundle\:
        resource: ../../src/AppBundle/{EventListener,Form/Type}

    AppBundle\Controller\:
        resource: ../../src/AppBundle/Controller
        public: true

    AppBundle\Twig\AppExtension:
        arguments: { $locales: '%app_locales%' }

    AppBundle\EventListener\CommentNotificationListener:
        arguments: { $sender: '%app.notifications.email_sender%' }

    AppBundle\EventListener\RedirectToPreferredLocaleListener:
        arguments:
            $locales: '%app_locales%'
            $defaultLocale: '%locale%'

    AppBundle\Security\PostVoter:
        tags: [ security.voter ]

    Twig_Extensions_Extension_Intl:
        class: Twig_Extensions_Extension_Intl

# The following aliases are required for now when using by-id autowiring,
# until these types are aliased in each core bundle, which they should be.
# This kind of explicit aliasing is the only difference between by-id and
# by-type autowiring in this file.

    Doctrine\ORM\EntityManager:
        '@doctrine.orm.default_entity_manager'

    Doctrine\Common\Persistence\ObjectManager:
        '@doctrine.orm.default_entity_manager'

    Swift_Mailer:
        '@swiftmailer.mailer.default'

    Symfony\Component\Routing\Generator\UrlGeneratorInterface:
        '@router.default'

stof · 2017-03-21T09:05:43Z

src/Symfony/Component/DependencyInjection/Loader/schema/dic/services/services-1.0.xsd

+
+  <xsd:simpleType name="autowire">
+    <xsd:restriction base="xsd:string">
+      <xsd:pattern value="(true|false|by-type|by-id)" />


this does not allow parameters anymore, unlike boolean (see just above), so it is a BC break.

actually I wanted to trigger this discussion :)
so: in Definition::setAutowired, there is a cast to bool currently.
This means that parameters never worked, did they?
Adding %.+% to the regexp might fix the BC by still allowing one to pass a param,
but it won't fix the fact that this param is ignored.
Or did I miss something?
Btw, this is the case for all definition setters that "cast" - and has never been an issue really (autowiring/abstract/etc. should not be parametric.)

if it never worked, this is fine for me.

stof · 2017-03-21T09:10:44Z

for the url generator, please add the alias as this has to be done in FrameworkBundle

stof · 2017-03-21T09:41:52Z

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

@@ -257,9 +258,9 @@ private function autowireMethod(\ReflectionMethod $reflectionMethod, array $argu
                continue;
            }

-            $typeName = InheritanceProxyHelper::getTypeHint($reflectionMethod, $parameter, true);
+            $type = InheritanceProxyHelper::getTypeHint($reflectionMethod, $parameter, true);


As all these renamings are already merged as part of another PR now, can you rebase your branch to make the diff more readable ?

nicolas-grekas · 2017-03-21T15:28:34Z

First commit now split as separate PR, adding logging to AutowirePass, see #22095.
This made me realize that by-id autoregistration has a high chance of creating a duplicate service when an alias is missing. Better encourage writing explicit aliases.
PR updated accordingly. Still very useful to me to help devs wire things explicitly but progressively.

…nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [*Bundle] Add autowiring aliases for common services | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As spotted while working on #22060, we're missing many aliases to prevent any autowiring ambiguities. I also removed the "Symfony\Component\EventDispatcher\EventDispatcher" and "Symfony\Component\DependencyInjection\Container" aliases: we'd better encourage using the corresponding interfaces instead. On ControllerTrait, we need to type hint against SessionInterface, because otherwise, when session support is disabled, autowiring still auto-registers an "autowired.Session" service, which defeats the purpose of being able to enable/disable it. Commits ------- 08c2ee3 [*Bundle] Add autowiring aliases for common services

nicolas-grekas · 2017-03-21T21:59:40Z

Exception message enhanced a bit more, with suggestions for already aliased types:

    [Symfony\Component\DependencyInjection\Exception\RuntimeException]             
    Cannot autowire argument $entityManager of method AppBundle\EventListener\C    
    heckRequirementsSubscriber::__construct() for service "AppBundle\EventListe    
    ner\CheckRequirementsSubscriber": Service "Doctrine\ORM\EntityManager" does    
     not exist. This type could be aliased to the existing "doctrine.orm.defaul    
    t_entity_manager" service. This service is already aliased to "Doctrine\ORM    
    \EntityManagerInterface".

weaverryan · 2017-03-22T12:06:16Z

Hi guys!

Let me summarize the new mode! This mode no longer looks through the system and tries to find services that match the type-hint. Instead, it's much simpler: it only looks to see if a service id exists that matches the type-hint. Otherwise, it throws an exception: it does not try to automatically create an autowired private service for that class (if it's a class).

I like this! (but I don't like having 2 modes). It doesn't work how I expect autowiring to work (i.e. finding services matching a type-hint), but it's perfectly straightforward. Also, if you take the other mode to an extreme, you would have zero services explicitly wired (other than controllers) because the type-hints in classes would cause autowiring to auto-create services. But with this mode, we're saying: all services must be explicitly wired (while in reality for DX, the PSR-4 loader helps a lot here). That feels a bit better: I will be able to see in my services.yml file why every service exists.

Anyways, I like this... but I don't like the 2 modes. I don't mean we should remove the old one. Instead, if we accept this, it should be with the intention that this becomes the recommended mode for autowiring.

Cheers!

weaverryan · 2017-03-22T12:07:24Z

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

+        } elseif (isset($this->ambiguousServiceTypes[$type])) {
+            $message = sprintf('Service "%s" does not exist. This type could be aliased to any of these existing services: "%s"', $type, implode('", "', $this->ambiguousServiceTypes[$type]));
+        } elseif (isset($this->types[$type])) {
+            $message = sprintf('Service "%s" does not exist. This type could be aliased to the existing "%s" service', $type, $this->types[$type]);


It's perhaps outside the scope of this PR, but we could also add this "alias" recommendation to the "other" mode, when there are multiple definitions (

symfony/src/Symfony/Component/DependencyInjection/Compiler/AutowirePass.php

Line 56 in daac6c2

throw new RuntimeException(sprintf('Unable to autowire argument of type "%s" for the service "%s". Multiple services exist for this %s (%s).', $type, $id, $classOrInterface, $matchingServices));

)

weaverryan · 2017-03-22T12:14:53Z

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

+            }
+            if ($aliases) {
+                $message .= sprintf('. This service is already aliased to "%s"', implode('", "', $aliases));
+            }


What case is this catching? I don't see anything in AutowirePassTest for this.

See my previous comment, last sentence of the block:
#22060 (comment)
This PR has no tests yet :)

Ah, thank you! What about:

. Or you can update your type hint to "%s".

(but, if we did this, we probably need some extra logic to make the sentence read correctly if there are multiple $aliases):

. Or you can update your type hint to be one of the following: "%s".

@weaverryan see #22135

theofidry · 2017-03-22T12:41:34Z

Wholeheartedly agree with @weaverryan 😄

nicolas-grekas · 2017-03-24T10:00:12Z

Test case added, failure unrelated. Ready.

…icolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Add hints to exceptions thrown by AutowiringPass | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As spotted while working on #22060 Commits ------- d7557cf [DI] Add hints to exceptions thrown by AutowiringPass

…n the class<>id convention

fabpot · 2017-03-25T20:19:12Z

Thank you @nicolas-grekas.

…t of it based on the class<>id convention (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR adds a new autowiring mode, based only on the class <> id convention. This way of autowiring is free from any conflicting behavior, which is what I was looking for to begin with. The expected DX is a bit more involving than the current way we do autowiring. But it's worth it to me, because it's plain predictable - a lot less "magic" imho. So in this mode, for each `App\Foo` type hint, a reference to an "App\Foo" service will be created. If no such service exists, an exception will be thrown. To me, this opens a nice DX: when type hinting interfaces (which is the best practice), this will tell you when you need to create the explicit interface <> id mapping that is missing - thus encourage things to be made explicit, but only when required, and gradually, in a way that will favor discoverability by devs. Of course, this is opt-in, and BC. You'd need to do eg in yaml: `autowire: by_id`. For consistency, the current mode (`autowire: true`) can be configured using `autowire: by_type`. Commits ------- c298f2a [DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention

…c9dc7) This is done to backport DIC from Symfony 3.3 to Symfony 3.2.6. It is temporary workaround as we want to use Symfony 3.2.6 but also features like "by-id" autowiring (symfony#22060). After Symfony 3.3 will be released and we migrate to it we can get rid of this "blend-commit".

jsamouh · 2017-03-26T09:07:07Z

@nicolas-grekas , you think you update the doc soon ^^ ? need it to prevent my partner using this implementation when autowire fails causing by service definition priority.
thks for all and good work 👍

curry684 · 2017-03-26T21:40:22Z

http://symfony.com/blog/new-in-symfony-3-3-configurable-autowiring-setters is still live without changes, which I was trying last Friday before this PR got merged, and cost me a lot of time as autowire: [ setX ] was still accepted at the time in service config but not doing anything at all. Current master throws an error on that thankfully, but the blog announcement is still up.

edit: removed by now

dunglas · 2017-04-03T21:44:12Z

Is this new mode still necessary now that we have #22254? IIUC, they fix almost the same issue (multiple implementations of an interface). For classes without interfaces, the default mode will less likely fail and is faster/easier to use from a dev POV.

Anyway, this new mode can also be useful in some cases (e.g. the domain layer - it's an intermediate between 100% manual wiring and the "real" autowiring mode) but IMO it should not be the default.

…g reflection against all existing services (nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | no | New feature? | yes | BC breaks? | yes - compile time only | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - (patch best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/22295/files?w=1).) "By-id" autowiring, as introduced in #22060 is free from all the issues that "by-type" autowiring has: - it has no magic and requires explicit type<>id matching (*vs* using reflection on all services to cherry-pick *the* one that matches some type-hint *at that time*, which is fragile) - it is free from any ambiguities (*vs* the Damocles' sword of breaking config just by enabling some unrelated bundle) - it is easily introspected: just look at DI config files (*vs* inspecting the type-hierarchy of all services + their type-hints) - ~~it is side-effect free, thus plain predictable (*vs* auto-registration of discovered types as services)~~ - it plays nice with deprecated services or classes (see #22282) - *etc.* Another consideration is that any "by-type" autowired configuration is either broken (because of future ambiguities) - or equivalent to a "by-id" configuration (because resolving ambiguities *means* adding explicit type<>id mappings.) For theoreticians, we could say that "by-id" autowiring is the asymptotic limit of "by-type" autowiring :-) For all these reasons - and also because it reduces the complexity of the code base - I propose to change the behavior and only support "by-id" autowiring in 3.3. This will break some configurations relying on "by-type" autowiring. Yet the break will only happen at compile-time, which means this won't *silently* break any apps. For all core Symfony services, they will work out of the box thanks to #22098 *et al.* For the remaining services, fixing ones config should be pretty straightforward: just follow the suggestions provided by the exception messages. If they are fine to you, you'll end up with the exact same config in the end. And maybe you'll spot issues that were hidden previously. Commits ------- cc5e582 [BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services

nicolas-grekas added this to the 3.3 milestone Mar 19, 2017

carsonbot added Status: Needs Review DependencyInjection Feature labels Mar 19, 2017

nicolas-grekas mentioned this pull request Mar 19, 2017

[DI] Replace "autowired." prefix by "autoregistered" tag for services created by AutowirePass #21871

Closed

nicolas-grekas force-pushed the di-autorequired branch from 50852ed to db800fc Compare March 20, 2017 08:15

GuilhemN reviewed Mar 20, 2017

View reviewed changes

nicolas-grekas force-pushed the di-autorequired branch 4 times, most recently from d7fd5a6 to 59a4bf7 Compare March 20, 2017 22:44

stof reviewed Mar 21, 2017

View reviewed changes

nicolas-grekas force-pushed the di-autorequired branch 2 times, most recently from 36d76e9 to ee07c1b Compare March 21, 2017 15:23

nicolas-grekas changed the title ~~[DI] Add side-effect-restricted variant for autowiring, based solely on ids~~ [DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention Mar 21, 2017

nicolas-grekas mentioned this pull request Mar 21, 2017

[*Bundle] Add autowiring aliases for common services #22098

Merged

nicolas-grekas force-pushed the di-autorequired branch 3 times, most recently from 241de74 to 7b80c02 Compare March 21, 2017 21:52

weaverryan reviewed Mar 22, 2017

View reviewed changes

nicolas-grekas force-pushed the di-autorequired branch from 7b80c02 to cdee332 Compare March 22, 2017 20:33

nicolas-grekas mentioned this pull request Mar 23, 2017

[DI] Add hints to exceptions thrown by AutowiringPass #22135

Merged

nicolas-grekas force-pushed the di-autorequired branch 4 times, most recently from 07fef83 to 854a51b Compare March 24, 2017 09:50

[DI] Add "by-id" autowiring: a side-effect free variant of it based o…

c298f2a

…n the class<>id convention

nicolas-grekas force-pushed the di-autorequired branch from 854a51b to c298f2a Compare March 24, 2017 14:54

fabpot merged commit c298f2a into symfony:master Mar 25, 2017

nicolas-grekas deleted the di-autorequired branch March 25, 2017 20:19

sustmi mentioned this pull request Mar 25, 2017

[DI] Successful construction of DIC depends on order of service definitions when depending on an interface which implementation is auto-registered #22162

Closed

javiereguiluz mentioned this pull request Mar 26, 2017

[DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention symfony/symfony-docs#7706

Closed

nicolas-grekas mentioned this pull request Apr 5, 2017

[BC BREAK][DI] Always autowire "by id" instead of using reflection against all existing services #22295

Merged

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention #22060

[DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention #22060

nicolas-grekas commented Mar 19, 2017 •

edited

Loading

theofidry commented Mar 20, 2017

javiereguiluz commented Mar 20, 2017

GuilhemN Mar 20, 2017

nicolas-grekas Mar 20, 2017

GuilhemN Mar 20, 2017

nicolas-grekas Mar 20, 2017 •

edited

Loading

nicolas-grekas Mar 20, 2017

nicolas-grekas commented Mar 20, 2017 •

edited

Loading

stof Mar 21, 2017

nicolas-grekas Mar 21, 2017 •

edited

Loading

stof Mar 21, 2017

stof commented Mar 21, 2017

stof Mar 21, 2017

nicolas-grekas Mar 21, 2017

nicolas-grekas commented Mar 21, 2017 •

edited

Loading

nicolas-grekas commented Mar 21, 2017

weaverryan commented Mar 22, 2017

weaverryan Mar 22, 2017

weaverryan Mar 22, 2017

nicolas-grekas Mar 22, 2017

weaverryan Mar 23, 2017

nicolas-grekas Mar 23, 2017

theofidry commented Mar 22, 2017

nicolas-grekas commented Mar 24, 2017

fabpot commented Mar 25, 2017

jsamouh commented Mar 26, 2017

curry684 commented Mar 26, 2017 •

edited

Loading

dunglas commented Apr 3, 2017

[DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention #22060

[DI] Add "by-id" autowiring: a side-effect free variant of it based on the class<>id convention #22060

Conversation

nicolas-grekas commented Mar 19, 2017 • edited Loading

theofidry commented Mar 20, 2017

javiereguiluz commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Mar 21, 2017 • edited Loading

nicolas-grekas commented Mar 21, 2017

weaverryan commented Mar 22, 2017

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

theofidry commented Mar 22, 2017

nicolas-grekas commented Mar 24, 2017

fabpot commented Mar 25, 2017

jsamouh commented Mar 26, 2017

curry684 commented Mar 26, 2017 • edited Loading

dunglas commented Apr 3, 2017

nicolas-grekas commented Mar 19, 2017 •

edited

Loading

nicolas-grekas Mar 20, 2017 •

edited

Loading

nicolas-grekas commented Mar 20, 2017 •

edited

Loading

nicolas-grekas Mar 21, 2017 •

edited

Loading

nicolas-grekas commented Mar 21, 2017 •

edited

Loading

curry684 commented Mar 26, 2017 •

edited

Loading