[DI] Optional class for named services #21133

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

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: [DependencyInjection] Automatically detect the definitions class when possible #19191 (comment)

From @hason:

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/,

stof · 2017-01-02T14:17:04Z

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

@@ -29,6 +42,9 @@ public function process(ContainerBuilder $container)
        if (!method_exists(\ReflectionMethod::class, 'getReturnType')) {
            return;
        }
+        if (null !== $this->resolveClassPass) {
+            $this->resolveClassPassChanges = $this->resolveClassPass->getChanges();


this property should be reset at the end of processing to release memory

Did you forget to push the change or do I miss something? I don't see where it is reset.

Here: https://github.com/symfony/symfony/pull/21133/files#diff-5652ba8338f1efd56fa1e0e6b6d88994R46

Saw it now.

in fact, I removed the property completely for a local variable instead

GuilhemN · 2017-01-02T16:56:32Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

+     *
+     * @return string
+     */
+    public function getCaseFullId($id)


What about getRawId() or getOriginalId()?

But does case full id really exists in english ?

Yeah, if anything it should be casefullId (or getCasefullId). But I like @theofidry's caseSensitiveId more 👍

updated to caseSensitive

theofidry · 2017-01-02T17:57:52Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php


        if ($this->isFrozen() && (isset($this->definitions[$id]) && !$this->definitions[$id]->isSynthetic())) {
            // setting a synthetic service on a frozen container is alright
            throw new BadMethodCallException(sprintf('Setting service "%s" for an unknown or non-synthetic service definition on a frozen container is not allowed.', $id));
        }

        unset($this->definitions[$id], $this->aliasDefinitions[$id]);
+        if ($id !== $caseFullId) {
+            $this->caseFullIds[$id] = $caseFullId;


caseSensitiveIds?

👍 seems more obvious what is meant

xabbuh · 2017-01-03T07:58:14Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -101,6 +101,11 @@ class ContainerBuilder extends Container implements TaggedContainerInterface
    private $envCounters = array();

    /**
+     * @var string[] A map of case less to case full ids


string[] does not cover arrays where keys are no integers, does it?

xabbuh · 2017-01-03T08:00:45Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -837,6 +854,20 @@ public function findDefinition($id)
    }

    /**
+     * Returns the case full id used at registration time if any.


I would remove the "if any" part because we always return the id that was used when defining the definition. This makes it look likenull might be returned in some cases too.

nicolas-grekas · 2017-01-03T08:48:38Z

src/Symfony/Component/DependencyInjection/Loader/XmlFileLoader.php

@@ -301,7 +301,7 @@ private function processAnonymousServices(\DOMDocument $xml, $file)
        if (false !== $nodes = $xpath->query('//container:argument[@type="service"][not(@id)]|//container:property[@type="service"][not(@id)]')) {
            foreach ($nodes as $node) {
                // give it a unique name
-                $id = sprintf('%s_%d', hash('sha256', $file), ++$count);
+                $id = sprintf('%d_%s', ++$count, hash('sha256', $file));


just made these changes so that anonymous classes won't have their class set to a random string (+ added corresponding regexp check in ResolveClassPass)

Why hashing it? Wouldn't it be more explicit with the plain file name?

not related to this PR :)

OskarStark · 2017-01-03T13:17:07Z

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

+            if ($definition instanceof ChildDefinition || $definition->isSynthetic() || null !== $definition->getClass()) {
+                continue;
+            }
+            if (preg_match('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*$/', $id)) {


shouldn't this be covered by a test?

covered now

thank you 👍

xabbuh · 2017-01-03T18:28:05Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -638,6 +648,9 @@ public function setAlias($alias, $id)
        }

        unset($this->definitions[$alias]);
+        if ($alias !== $caseSensitiveAlias) {
+            $this->caseSensitiveIds[$alias] = $caseSensitiveAlias;


Do we actually need the entry for aliases or could we just remove existing entries here?

For completeness yes: we track any id, wherever it comes from

xabbuh · 2017-01-03T19:54:43Z

👍

Status: Reviewed

fabpot · 2017-01-07T14:26:57Z

src/Symfony/Component/DependencyInjection/ContainerBuilder.php

@@ -364,14 +369,18 @@ public function getCompiler()
     */
    public function set($id, $service)
    {
-        $id = strtolower($id);
+        $caseSensitiveId = (string) $id;


Why do you add a cast here?

Probably to be BC with the previous behaviour: It was possible to pass e.g. an object with __toString() as $id as strtolower() cast everything to a string.

If that's the reason, I would revert this as $id is documented as being a string.

fabpot · 2017-01-07T16:15:37Z

Thank you @nicolas-grekas.

@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

wouterj · 2017-01-07T18:21:22Z

Fyi, created a doc issue for this great feature: symfony/symfony-docs#7329

…tainer pass list (fabpot) This PR was merged into the 3.3-dev branch. Discussion ---------- [DependencyInjection] moved up ResolveClassPass in the container pass list | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | bug from #21133 | License | MIT | Doc PR | n/a Some compiler passes need access to the service class names. But when using an empty class name (the service id being the class name), the resolution happens too late (during the optimization step). This PR fixes this by moving up the ResolveClassPass earlier in the stack. Commits ------- 2e5b69f [DependencyInjection] moved up ResolveClassPass in the container pass list

@baz

…izanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Add Yaml syntax for short services definition | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | todo In my experience, most (at least, a lot) of the service definitions in an end-application only require the class and constructor arguments. #21133 allows to get rid of the `class` attribute by using the service id. Which means only arguments remain for most use-cases. Hence, we could support this syntax: #### Before: ```yml services: App\Foo\Bar: arguments: ['@baz', 'foo', '%qux%'] ``` #### After: ```yml services: App\Foo\Bar: ['@baz', 'foo', '%qux%'] ``` It works especially well along with services `_defaults` introduced in #21071 : ```yml services: _defaults: public: false autowire: ['set*'] tags: ['serializer.normalizer'] App\Serializer\FooNormalizer: ['@baz', 'foo', '%qux%'] ``` Commits ------- 83b599c [DI] Add Yaml syntax for short services definition

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

carsonbot added Status: Needs Review DependencyInjection Feature Deprecation labels Jan 2, 2017

nicolas-grekas mentioned this pull request Jan 2, 2017

[DependencyInjection] Optional class for class named services #20264

Closed

nicolas-grekas force-pushed the class-service branch from 618d432 to 80e4aae Compare January 2, 2017 13:56

stof reviewed Jan 2, 2017

View reviewed changes

nicolas-grekas force-pushed the class-service branch 2 times, most recently from af7dfda to 5def4a7 Compare January 2, 2017 16:09

GuilhemN reviewed Jan 2, 2017

View reviewed changes

theofidry reviewed Jan 2, 2017

View reviewed changes

theofidry mentioned this pull request Jan 3, 2017

[RFC] Define services via annotations #21103

Closed

dunglas approved these changes Jan 3, 2017

View reviewed changes

xabbuh reviewed Jan 3, 2017

View reviewed changes

nicolas-grekas force-pushed the class-service branch 4 times, most recently from f18bbe7 to 1d766ab Compare January 3, 2017 08:18

nicolas-grekas changed the title ~~[DI] Optional class for class named services~~ [DI] Optional class for named services Jan 3, 2017

nicolas-grekas force-pushed the class-service branch 3 times, most recently from 4b7cd67 to 01afe39 Compare January 3, 2017 08:44

nicolas-grekas commented Jan 3, 2017

View reviewed changes

OskarStark reviewed Jan 3, 2017

View reviewed changes

nicolas-grekas force-pushed the class-service branch 2 times, most recently from 68e9588 to 405b151 Compare January 3, 2017 17:39

xabbuh reviewed Jan 3, 2017

View reviewed changes

carsonbot removed the Status: Needs Review label Jan 3, 2017

carsonbot added the Status: Reviewed label Jan 3, 2017

fabpot reviewed Jan 7, 2017

View reviewed changes

nicolas-grekas and others added 2 commits January 7, 2017 16:55

[DI] Optional class for named services

71b17c7

[DI] Add tests for class named services

a18c4b6

nicolas-grekas force-pushed the class-service branch from 405b151 to a18c4b6 Compare January 7, 2017 15:55

fabpot merged commit a18c4b6 into symfony:master Jan 7, 2017

nicolas-grekas deleted the class-service branch January 7, 2017 16:32

wouterj mentioned this pull request Jan 7, 2017

[3.3][DI] Service ids now default to the classname symfony/symfony-docs#7329

Closed

fabpot mentioned this pull request Jan 8, 2017

[DependencyInjection] moved up ResolveClassPass in the container pass list #21203

Merged

Haehnchen mentioned this pull request Jan 9, 2017

[DI] Support optional class for named services Haehnchen/idea-php-symfony2-plugin#847

Closed

TomasVotruba mentioned this pull request Jan 14, 2017

[DependencyInjection] Automatic registration of FQCN services #18268

Closed

This was referenced Jan 16, 2017

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

Merged

[DI] Add Yaml syntax for short services definition #21313

Merged

ogizanagi mentioned this pull request Jan 23, 2017

[DI][DX] Optional class for named services DX issue #21380

Closed

nicolas-grekas mentioned this pull request Mar 24, 2017

Anonymous services do not work for global classes #22146

Closed

pierredup mentioned this pull request Apr 8, 2017

[DI] Cannot instantiate abstract class #22345

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

hason mentioned this pull request Aug 28, 2018

Add compiler pass for omitted class symfonette/class-named-services#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Optional class for named services #21133

[DI] Optional class for named services #21133

nicolas-grekas commented Jan 2, 2017 •

edited

Loading

stof Jan 2, 2017

nicolas-grekas Jan 2, 2017

xabbuh Jan 3, 2017

nicolas-grekas Jan 3, 2017

xabbuh Jan 3, 2017

nicolas-grekas Jan 3, 2017

GuilhemN Jan 2, 2017

nicolas-grekas Jan 2, 2017

GuilhemN Jan 2, 2017

wouterj Jan 2, 2017

nicolas-grekas Jan 3, 2017

theofidry Jan 2, 2017

xabbuh Jan 3, 2017

xabbuh Jan 3, 2017

xabbuh Jan 3, 2017

nicolas-grekas Jan 3, 2017

dunglas Jan 3, 2017

nicolas-grekas Jan 3, 2017

OskarStark Jan 3, 2017

nicolas-grekas Jan 3, 2017

OskarStark Jan 4, 2017

xabbuh Jan 3, 2017

nicolas-grekas Jan 3, 2017 •

edited

Loading

xabbuh commented Jan 3, 2017

fabpot Jan 7, 2017

wouterj Jan 7, 2017 •

edited

Loading

fabpot Jan 7, 2017

nicolas-grekas Jan 7, 2017

fabpot commented Jan 7, 2017

wouterj commented Jan 7, 2017

[DI] Optional class for named services #21133

[DI] Optional class for named services #21133

Conversation

nicolas-grekas commented Jan 2, 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

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

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 Jan 3, 2017 • edited Loading

Choose a reason for hiding this comment

xabbuh commented Jan 3, 2017

Choose a reason for hiding this comment

wouterj Jan 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jan 7, 2017

wouterj commented Jan 7, 2017

nicolas-grekas commented Jan 2, 2017 •

edited

Loading

nicolas-grekas Jan 3, 2017 •

edited

Loading

wouterj Jan 7, 2017 •

edited

Loading