[DependencyInjection] Optional class for class named services #20264

hason · 2016-10-21T14:37:20Z

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

I prefer class named services (in applications) because naming things are too hard:

services:
    Vendor\Namespace\Class:
        class: Vendor\Namespace\Class
        autowire: true

This PR solves redundant parameter for class:

services:
    Vendor\Namespace\Class:
        autowire: true

Inspirations: https://laravel.com/docs/5.3/container, #18268, http://php-di.org/,

javiereguiluz · 2016-10-21T14:47:20Z

I've never used autowiring, so probably the following comment is stupid but ... why configuring autowiring seems so verbose? In my imagination, this feature would work like this:

framework:
    # everything is autowired
    autowire: true

    # nothing is autowired (default value)
    autowire: false

hason · 2016-10-21T14:53:19Z

@javiereguiluz Great suggestion but I would like to enable autowiring per file (only for services defined in this file):

autowire: true

services:
    Vendor\Namespace\Class: ~

dunglas · 2016-10-22T08:52:14Z

@javiereguiluz we chosen to make autowiring an opt-in feature on a per service basis to prevent BC breaks and to force the developer to think about it before blindly enabling this feature.

Also, having such global config will enable autowiring for all services defined by the framework and the bundle and it doesn't look reasonable. I think that it's important to be able to choose which services will be autowired.

However, they are bundles out there allowing to enable autowiring for all services.

ro0NL · 2016-10-22T12:05:26Z

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

+                continue;
+            }
+
+            foreach ($container->getOriginalIds($id) as $class) {


Do we really need the original id here? And why do we expect more variants? Imo. there are only 2 (normalized/lowercased and not normalized/original).

For PHP the case sensitivity doesnt matter anyway, ie. it could be just;

if (class_exists($id, true)) { $definition->setClass($id); }

Otherwise, what about using reflection to get the original class name here? Ie. if it's required for setClass but perhaps is good for cosmetic reasons also.

The problem is with composer autoloader which is case sensitive.

I see. What about this then;

try { $definition->setClass((new \ReflectionClass($id))->getName()); } catch (\ReflectionException $e) { }

edit: probably the same issue right? in terms of autoloading.

More variants <=> more ids are converted to one normalized form.

ro0NL · 2016-10-22T12:11:31Z

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

+{
+    public function process(ContainerBuilder $container)
+    {
+        foreach ($container->getDefinitions() as $id => $definition) {


Could be $class => $definition to clarify.

No. $id is case insensitive, composer autoloader is case sensitive.

Tobion · 2016-10-22T12:54:24Z

I think I like the idea because naming services is really hard and annoying. So when you only have a single implementation of a class, why not make id == class.

For YAML it makes sense to use id -> class because the id is the key. But I wonder if it should be class -> id in XML:

<service class="Vendor\Namespace\Class" />

So the ID is optional instead of the class. This might be more logical and it keeps the IDE features working like autocomplete for the class property.

ro0NL · 2016-10-22T13:02:24Z

I think I like the idea because naming services is really hard and annoying.

Same here :)

@Tobion should the XML loader then reset the class after it has been set as id, so everything is consistently dealt by this pass?

Ie. <service class="Vendor\Namespace\Class" /> equals <service id="Vendor\Namespace\Class" />

ro0NL · 2016-10-22T14:15:06Z

@hason not sure.. but what about out-of-the-box support, right here: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ContainerBuilder.php#L762 No need for a pass.

theofidry · 2016-10-26T10:27:55Z

I agree with @Tobion here, I personally find service naming tiresome and hate above anything to realise sometimes the naming is not consistent for whatever reason, and that actually at the end of the day my names are just a normalized class name unless there is a reason to have the same class declared as a service multiple times, but in which case naming is usually easier.

That said this is unrelated to the auto-wiring feature, or you could imagine making the class property forbidden if the ID of the service is a FQCN.

hason · 2016-10-26T11:41:02Z

@ro0NL Thanks for your suggestion.

nicolas-grekas · 2016-11-25T11:10:12Z

rebase needed

theofidry · 2016-11-25T11:23:37Z

One thing to take into consideration is that right now, the DI make the service id lowercase, i.e.:

services:
  Foo:
    class: App\Foo

  foo:
    class: foo

both will have the same ID foo. If for regular services ID like above it was not a problem, for class names it can be bit more annoying as they are case sensitive.

I don't think this is a big problem though, but I feel like this limitation should be mentioned somewhere in the doc.

stof · 2016-11-25T11:29:59Z

@theofidry PHP class names are not case sensitive (the PSR-0 autoloading is), so you cannot have both a foo and Foo classes.

But as we lowercase ids (as soon as we can), we indeed cannot guess the class from the id (at least not later than the loader level)

stof · 2016-11-25T11:31:08Z

Btw, the XML loader already allows to omit the id, and will generate a random id in this case (which is fine as long as you use this service only as an autowired dependency rather than injecting it or retrieving it manually)

hason · 2016-11-25T12:53:40Z

@nicolas-grekas Rebased.

…wired definitions (wouterj) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Added a shortcut method for autowired definitions | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | todo This is a simple proposal to make adding autowired definitions to the `ContainerBuilder` a little easier. Registering autowired services in PHP code was quite verbose at the moment, while the whole point of autowiring is quick service registration. **Before** ```php $container->register('app.twig_extension', AppExtension::class) ->setAutowired(true) ->addTag('twig.extension') ; ``` **After** ```php $container->autowire('app.twig_extension', AppExtension::class) ->addTag('twig.extension') ; ``` With #20264, this will be even nicer: ```php $container->autowire(AppExtension::class) ->addTag('twig.extension') ; ``` Commits ------- 6ef4ce8 Added a shortcut method for autowired definitions

…wired definitions (wouterj) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] Added a shortcut method for autowired definitions | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | todo This is a simple proposal to make adding autowired definitions to the `ContainerBuilder` a little easier. Registering autowired services in PHP code was quite verbose at the moment, while the whole point of autowiring is quick service registration. **Before** ```php $container->register('app.twig_extension', AppExtension::class) ->setAutowired(true) ->addTag('twig.extension') ; ``` **After** ```php $container->autowire('app.twig_extension', AppExtension::class) ->addTag('twig.extension') ; ``` With symfony/symfony#20264, this will be even nicer: ```php $container->autowire(AppExtension::class) ->addTag('twig.extension') ; ``` Commits ------- 6ef4ce8 Added a shortcut method for autowired definitions

nicolas-grekas · 2016-12-17T08:30:40Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -759,6 +759,10 @@ public function setDefinition($id, Definition $definition)
            throw new BadMethodCallException('Adding definition to a frozen container is not allowed');
        }

+        if (null === $definition->getClass() && class_exists($id)) {


The class_exists check will hide typos. Can't we throw instead when the class doesn't exist?

also: should we allow this for abstract classes? and for synthetic services? for ChildDefinition?

What about always calling something like Definition::resolve($id) and move some logic?

@nicolas-grekas The exception for an unknown (not-set) class is thrown in compiler pass.

nicolas-grekas · 2016-12-18T11:15:56Z

Rethinking about this one, shouldn't this logic be handled by PhpDumper instead?
Definition should have as low logic as possible. But PhpDumper is the right place to interpret what a definition means. Don't you think?

nicolas-grekas · 2016-12-18T11:17:19Z

I missed adding that ContainerBuilder shouldn't change the state of Definition objects IMHO.

ogizanagi · 2016-12-19T18:05:03Z

@nicolas-grekas I think the implementation cannot be changed to handle this logic in the PhpDumper actually. As mentioned in #20264 (comment), the original service id is lowercased by the ContainerBuilder::setDefinition() method. Which makes impossible to guess the class from the service id at this point.

On another hand, regarding #20943, @stof got a point in #20943 (comment). A compiler pass would be more suitable.

nicolas-grekas · 2016-12-27T22:13:49Z

@hason do you think you could have a look at this one in the coming weeks? It would be a nice addition for the feature set we're working on with @dunglas for 3.3

nicolas-grekas · 2017-01-02T13:53:35Z

Continued in #21133
Thank you @hason for the proposal.

@hason

…-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Optional class for named services | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Continues #20264: - makes the id-to-class mapping context-free (no `class_exist` check), - deals with ChildDefinition (which should not have this rule applied on them), - deprecates FactoryReturnTypePass as discussed on slack and reported in this comment: #19191 (comment) From @hason: > I prefer class named services (in applications) because naming things are too hard: ``` yaml services: Vendor\Namespace\Class: class: Vendor\Namespace\Class autowire: true ``` > This PR solves redundant parameter for class: ``` yaml services: Vendor\Namespace\Class: autowire: true ``` > Inspirations: https://laravel.com/docs/5.3/container, #18268, http://php-di.org/, Commits ------- a18c4b6 [DI] Add tests for class named services 71b17c7 [DI] Optional class for named services

carsonbot added Status: Needs Review DependencyInjection Feature labels Oct 21, 2016

hason force-pushed the class-service branch from 19b9e10 to 6627e27 Compare October 22, 2016 11:21

ro0NL reviewed Oct 22, 2016

View reviewed changes

hason mentioned this pull request Oct 26, 2016

[DependencyInjection] Better autowiring integration #20302

Closed

hason force-pushed the class-service branch from 6627e27 to ce92de2 Compare October 26, 2016 11:39

hason force-pushed the class-service branch 2 times, most recently from e1b86ab to 5f9f981 Compare October 27, 2016 08:12

hason mentioned this pull request Nov 3, 2016

Add support for FQCN service name Haehnchen/idea-php-symfony2-plugin#811

Closed

[DependencyInjection] Optional class for class named services

711fafc

hason force-pushed the class-service branch from 5f9f981 to 711fafc Compare November 25, 2016 11:56

wouterj mentioned this pull request Nov 26, 2016

[DependencyInjection] Added a shortcut method for autowired definitions #20648

Merged

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

nicolas-grekas reviewed Dec 17, 2016

View reviewed changes

ro0NL mentioned this pull request Dec 17, 2016

[DependencyInjection] Use current class as default class for factory declarations #20943

Merged

This was referenced Jan 2, 2017

[DependencyInjection] Automatically detect the definitions class when possible #19191

Merged

[DI] Optional class for named services #21133

Merged

nicolas-grekas closed this Jan 2, 2017

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

hason deleted the class-service branch May 25, 2017 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DependencyInjection] Optional class for class named services #20264

[DependencyInjection] Optional class for class named services #20264

hason commented Oct 21, 2016 •

edited

Loading

javiereguiluz commented Oct 21, 2016 •

edited

Loading

hason commented Oct 21, 2016 •

edited

Loading

dunglas commented Oct 22, 2016

ro0NL Oct 22, 2016 •

edited

Loading

hason Oct 22, 2016

ro0NL Oct 22, 2016 •

edited

Loading

hason Oct 22, 2016

ro0NL Oct 22, 2016

hason Oct 22, 2016

Tobion commented Oct 22, 2016 •

edited

Loading

ro0NL commented Oct 22, 2016 •

edited

Loading

ro0NL commented Oct 22, 2016 •

edited

Loading

theofidry commented Oct 26, 2016

hason commented Oct 26, 2016

nicolas-grekas commented Nov 25, 2016

theofidry commented Nov 25, 2016

stof commented Nov 25, 2016

stof commented Nov 25, 2016

hason commented Nov 25, 2016

nicolas-grekas Dec 17, 2016

nicolas-grekas Dec 17, 2016

ro0NL Dec 17, 2016

hason Dec 28, 2016

nicolas-grekas commented Dec 18, 2016

nicolas-grekas commented Dec 18, 2016

ogizanagi commented Dec 19, 2016

nicolas-grekas commented Dec 27, 2016

nicolas-grekas commented Jan 2, 2017

[DependencyInjection] Optional class for class named services #20264

[DependencyInjection] Optional class for class named services #20264

Conversation

hason commented Oct 21, 2016 • edited Loading

javiereguiluz commented Oct 21, 2016 • edited Loading

hason commented Oct 21, 2016 • edited Loading

dunglas commented Oct 22, 2016

ro0NL Oct 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Oct 22, 2016 • 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

Tobion commented Oct 22, 2016 • edited Loading

ro0NL commented Oct 22, 2016 • edited Loading

ro0NL commented Oct 22, 2016 • edited Loading

theofidry commented Oct 26, 2016

hason commented Oct 26, 2016

nicolas-grekas commented Nov 25, 2016

theofidry commented Nov 25, 2016

stof commented Nov 25, 2016

stof commented Nov 25, 2016

hason commented Nov 25, 2016

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

nicolas-grekas commented Dec 18, 2016

nicolas-grekas commented Dec 18, 2016

ogizanagi commented Dec 19, 2016

nicolas-grekas commented Dec 27, 2016

nicolas-grekas commented Jan 2, 2017

hason commented Oct 21, 2016 •

edited

Loading

javiereguiluz commented Oct 21, 2016 •

edited

Loading

hason commented Oct 21, 2016 •

edited

Loading

ro0NL Oct 22, 2016 •

edited

Loading

ro0NL Oct 22, 2016 •

edited

Loading

Tobion commented Oct 22, 2016 •

edited

Loading

ro0NL commented Oct 22, 2016 •

edited

Loading

ro0NL commented Oct 22, 2016 •

edited

Loading