[DI] Add "inherit-tags" with configurable defaults + same for "public", "tags" & "autowire" #21071

nicolas-grekas · 2016-12-27T17:54:15Z

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

Instead of making services private by default (#20048), which is going to create a big migration burden that might not be worth it, I'd like to propose the idea of changing the default for the current file.

Having a place to redefine some defaults, this can also be used to enable autowiring for a file, making it much lighter in the end.

This PR handles defaults for "public", "autowired" and "tags". Not sure the other options need a configurable default. Please comment if you think otherwise.

Short example (the feat is more interesting with a longer list of services):

services:
    _defaults:
        public: false
        autowire: ['_construct', 'set*']

    foo:
        class: Foo

<?xml version="1.0" encoding="utf-8"?>
<container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
    <services>
        <defaults public="false">
            <autowire>__construct</autowire>
            <tag name="foo"/>
        </defaults>
    </services>
</container>

ping @dunglas @weaverryan

ogizanagi · 2016-12-27T18:23:58Z

Looks like a good idea to me. What about aliases, though?
Should we provide a way to let them public by default while services will be private?

service_defaults:
    public: ['aliases'] # aliases are public whereas services are private

service_defaults:
    public: true # default: aliases and services are public <=> ['aliases', 'services']

service_defaults:
    public: false # aliases and services are private <=> []

(It'll need a special treatment for the service_defaults.public key, but as we won't generalize the service_defaults for every definition keys, I'd rather see this as a loader_config key than a "parent definition" one)

linaori · 2016-12-27T18:27:39Z

What about adding tags like this? I often find myself with a specific service definition file that contains a specific type of services. Think of a file for form types, they all have the same tag. It would be really nice if I could use this feature to automatically tag all definitions in that file by this implicit parent definition.

GuilhemN · 2016-12-27T18:35:06Z

👍, you should add this new section here or the ContainerBuilder will throw an exception.

javiereguiluz · 2016-12-27T18:44:14Z

Minor comment: could we please take some time to think about this proposal before merging it? This kind of changes can have a deep impact in how developers use Symfony. Personally I don't have an opinion about the idea yet ... but I don't like the implementation. It doesn't feel "natural" for Symfony.

ogizanagi · 2016-12-27T18:49:48Z

As soon as it doesn't behaves blindly as a pseudo parent definition, but rather as a context/configuration exploitable by the loaders, it looks good to me. Hence, I don't really like the service_defaults naming, but rather see something like loader_config/context or whatever.

GuilhemN · 2016-12-27T18:58:23Z

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

@@ -146,9 +146,10 @@ private function parseDefinitions($content, $file)
        if (!is_array($content['services'])) {
            throw new InvalidArgumentException(sprintf('The "services" key should contain an array in %s. Check your YAML syntax.', $file));
        }
+        $defaults = isset($content['service_defaults']) ? $content['service_defaults'] : array();


I think unsupported keys should throw an exception to be consistent with the rest of the loader.

nicolas-grekas · 2016-12-27T19:00:57Z

@javiereguiluz don't worry we have a process for that, this PR is no exception.

@ogizanagi I'm 👎 on loader_config because this is just leaking internal vocabulary that users don't necessarily know about.

nicolas-grekas · 2016-12-27T19:27:21Z

PR completed for YamlLoader. If someone would like to work on XmlLoader, I'd happily accept a PR against my branch!

ro0NL

👍

ro0NL · 2016-12-27T19:49:21Z

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

@@ -176,7 +188,7 @@ private function parseDefinition($id, $service, $file)
        static::checkDefinition($id, $service, $file);

        if (isset($service['alias'])) {
-            $public = !array_key_exists('public', $service) || (bool) $service['public'];
+            $public = array_key_exists('public', $service) ? (bool) $service['public'] : !empty($defaults['public']);


Any reason to define this variable twice? One doing array_key_exists other isset?

these are not on the same path, and don't have the same semantic (don't forget about ChildDefinition on the other path.)

I had services defined: (Symfony 3.1.8):

foo: class: My\FooService bar: alias: foo

I could access bar service in my controller because it was public by default. After this change here my aliased service is private by default, thus the service is not accessible in controller.

Shouldn't this be mentioned somewhere? I couldn't find any info about it.

Good catch. That's a bug. See #21219

ro0NL · 2016-12-27T20:51:50Z

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

+            throw new InvalidArgumentException(sprintf('Service defaults must be an array, "%s" given in "%s".', gettype($defaults), $file));
+        }
+        foreach ($defaults as $key => $default) {
+            if (!in_array($key, $defaultKeys)) {


Perhaps go strict here?

done this way for consistency with the many other in_array in this file, good enough anyway

dunglas · 2016-12-28T09:15:00Z

I like the idea!

What about using just defaults as key? service_defaults is a bit redundant with service:

defaults:
    public: false
    autowire: ['_construct', 'set*']

services:
    foo:
        class: Foo

The best looking config is:

services:
   defaults:
        public: false
        autowire: ['_construct', 'set*']

    'Foo\Bar': {}

But I'm not sure that it's ok to deprecate some services names like default for this example.

ro0NL · 2016-12-28T09:17:18Z

The best looking config is:

Really? The one above looks more intuitive to me...

defaults: #...
services: #...

that is.

Looking at it like this looks good... then again; if parameters are involved (parameters: #...) it may work counter-intuitive.

Meaning either service_defaults (:+1:), or indeed service.defaults.

nicolas-grekas · 2016-12-28T09:29:28Z

The root namespace is used for container extensions configuration, that means there is a potential for collision with an existing bundle that uses this "defaults" name.
This leads to another reason pro-service_defaults: the defaults we're talking about are related to services only.
If you have eg:

framework: #...
services: #...
defaults: #...

it's not intuitive at all that defaults applies only to services.
That's why I agree with @dunglas that defaults inside services is the best of the two proposals.
But is it worth the potential collision?

nicolas-grekas · 2016-12-28T09:45:54Z

What about adding an underscore in this way?

services:
    _defaults:
        #...

ro0NL · 2016-12-28T09:48:38Z

Perhaps * works out as well? It looks nicer then _defaults at least :) and seems to fit the case perfectly?

nicolas-grekas · 2016-12-28T09:49:42Z

I doubt * would be friendly to anyone not familiar with what it does.

ro0NL · 2016-12-28T09:51:15Z

Think you're right.. just typed it out to visualize in fact. It looks good (imo.) but indeed.. you need to know it.

service._defaults 👍

GuilhemN · 2016-12-28T10:18:33Z

What about using something that sounds more specific to yaml such as !defaults (as a key of services)?
It would also decrease the risk of conflicts.

nicolas-grekas · 2016-12-28T10:29:52Z

!defaults must be quoted in yaml, not very friendly. And we already use _ prefixes for special keys in eg routing yaml.

GuilhemN · 2016-12-28T10:40:06Z

@nicolas-grekas I was thinking about a tag returning something similar to the JavaScript Symbol but yes custom tags aren't supported yet.
Currently, unknown tags are considered as part of the scalar (e.g. !defaults remains !defaults as it is not known) so it's possible to use them and we could later migrate to a better tags system, wdyt?

dunglas · 2016-12-28T10:51:53Z

+1 for services._defaults. Btw we should take this occasion to deprecate any service name starting by a _.

TomasVotruba · 2016-12-30T00:15:22Z

Just for inspiration from @nette, there is similar tool, that decorates specific classes/types of classes:

See this article - jump to "Decorator Extension to the Rescue" headline.

And this Tweet: https://twitter.com/VotrubaT/status/812802092356276224

I'm amazed, how similar these tools are :)

grachevko · 2016-12-30T23:34:36Z

What about add factory to list of _default options?
It can be useful to define doctrine repositories as service for example.

TomasVotruba · 2016-12-31T01:30:20Z

@grachevko How would that look like? From example I understand it's global setup for all services.

grachevko · 2016-12-31T09:01:25Z

@TomasVotruba not a global. This PR add _default options which have affect only on file where it.
We can create subscribers.yml and set _default.tags: [{ name: kernel.event_subscriber }] for all subscribers in this file without typing same tag to each.
Just the same we could create repositories.yml and set _default.factory: ['@doctrine', 'getRepository'] without typing same factory.

TomasVotruba · 2016-12-31T12:53:17Z

@grachevko Ah. That's a pity. I was looking forward to get rid of Console, EventSubscriber, FormType etc. tags.

I'll work on that Decorator feature then :)

fabpot · 2017-01-07T14:37:50Z

👍

dunglas · 2017-01-07T14:48:22Z

👍

fabpot · 2017-01-07T17:31:09Z

Thank you @nicolas-grekas.

@dunglas

…ame for "public", "tags" & "autowire" (nicolas-grekas, ogizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Add "inherit-tags" with configurable defaults + same for "public", "tags" & "autowire" | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20048 | License | MIT | Doc PR | - Instead of making services private by default (#20048), which is going to create a big migration burden that might not be worth it, I'd like to propose the idea of changing the default for the current file. Having a place to redefine some defaults, this can also be used to enable autowiring for a file, making it much lighter in the end. This PR handles defaults for "public", "autowired" and "tags". Not sure the other options need a configurable default. Please comment if you think otherwise. Short example (the feat is more interesting with a longer list of services): ```yaml services: _defaults: public: false autowire: ['_construct', 'set*'] foo: class: Foo ``` ```xml <?xml version="1.0" encoding="utf-8"?> <container xmlns="http://symfony.com/schema/dic/services" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd"> <services> <defaults public="false"> <autowire>__construct</autowire> <tag name="foo"/> </defaults> </services> </container> ``` ping @dunglas @weaverryan Commits ------- beec1cf [DI] Allow definitions to inherit tags from parent context 05f24d5 [DI] Add "defaults" tag to XML services configuration 7b4a18b [DI] Add "_defaults" key to Yaml services configuration

chalasr · 2017-01-08T12:13:24Z

The new inherit-tags option would have deserved its own line in the changelog imho, it's just great! Thanks

…gizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Fixes aliases visibility with and without defaults | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21071 (comment) | License | MIT | Doc PR | N/A Commits ------- f1cc090 [DI] Fixes aliases visibility with and without defaults

@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 Dec 27, 2016

carsonbot added Status: Needs Review DependencyInjection Feature labels Dec 27, 2016

nicolas-grekas force-pushed the di-pragma branch 2 times, most recently from ac66912 to cd74550 Compare December 27, 2016 18:40

GuilhemN reviewed Dec 27, 2016

View reviewed changes

nicolas-grekas force-pushed the di-pragma branch from cd74550 to 93664c8 Compare December 27, 2016 19:24

ro0NL reviewed Dec 27, 2016

View reviewed changes

ogizanagi mentioned this pull request Dec 28, 2016

Allow XmlFileLoader to support service_defaults nicolas-grekas/symfony#9

Closed

nicolas-grekas force-pushed the di-pragma branch from 93664c8 to b7ccc56 Compare December 28, 2016 13:20

xabbuh mentioned this pull request Jan 7, 2017

[RFC][Yaml] Add custom tags support #21185

Closed

nicolas-grekas force-pushed the di-pragma branch 2 times, most recently from 9545899 to 88b99fb Compare January 7, 2017 16:39

nicolas-grekas and others added 3 commits January 7, 2017 17:40

[DI] Add "_defaults" key to Yaml services configuration

7b4a18b

[DI] Add "defaults" tag to XML services configuration

05f24d5

[DI] Allow definitions to inherit tags from parent context

beec1cf

nicolas-grekas force-pushed the di-pragma branch from 88b99fb to beec1cf Compare January 7, 2017 16:43

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

javiereguiluz mentioned this pull request Jan 8, 2017

Services can now define default options per file symfony/symfony-docs#7339

Closed

Haehnchen mentioned this pull request Jan 9, 2017

[DI] Configurable defaults + same for "public", "tags" & "autowire" Haehnchen/idea-php-symfony2-plugin#848

Open

nicolas-grekas deleted the di-pragma branch January 9, 2017 14:01

ogizanagi mentioned this pull request Jan 9, 2017

[DI] Fixes aliases visibility with and without defaults #21219

Merged

ogizanagi mentioned this pull request Jan 16, 2017

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

Merged

dunglas mentioned this pull request Jan 19, 2017

[DependencyInjection] Yaml: check if $tags is an array before using it #21348

Closed

nicolas-grekas mentioned this pull request Feb 1, 2017

[DI] Deprecate underscore-services in YamlFileLoader #21484

Merged

nicolas-grekas mentioned this pull request Mar 27, 2017

[DI] Enhance DX by throwing instead of triggering a deprecation notice #22185

Merged

curry684 mentioned this pull request Mar 27, 2017

[DI] Enable factory attribute in service defaults #22189

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

[DI] Add "inherit-tags" with configurable defaults + same for "public", "tags" & "autowire" #21071

[DI] Add "inherit-tags" with configurable defaults + same for "public", "tags" & "autowire" #21071

Conversation

nicolas-grekas commented Dec 27, 2016 • edited Loading

ogizanagi commented Dec 27, 2016 • edited Loading

linaori commented Dec 27, 2016

GuilhemN commented Dec 27, 2016

javiereguiluz commented Dec 27, 2016

ogizanagi commented Dec 27, 2016 • edited Loading

GuilhemN Dec 27, 2016

Choose a reason for hiding this comment

nicolas-grekas Dec 27, 2016

Choose a reason for hiding this comment

nicolas-grekas commented Dec 27, 2016 • edited Loading

nicolas-grekas commented Dec 27, 2016 • edited Loading

ro0NL left a comment

Choose a reason for hiding this comment

ro0NL Dec 27, 2016

Choose a reason for hiding this comment

nicolas-grekas Dec 27, 2016

Choose a reason for hiding this comment

takeit Jan 9, 2017

Choose a reason for hiding this comment

ogizanagi Jan 9, 2017

Choose a reason for hiding this comment

ro0NL Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas Dec 27, 2016 • edited Loading

Choose a reason for hiding this comment

dunglas commented Dec 28, 2016 • edited Loading

ro0NL commented Dec 28, 2016 • edited Loading

nicolas-grekas commented Dec 28, 2016 • edited Loading

nicolas-grekas commented Dec 28, 2016

ro0NL commented Dec 28, 2016

nicolas-grekas commented Dec 28, 2016

ro0NL commented Dec 28, 2016

GuilhemN commented Dec 28, 2016

nicolas-grekas commented Dec 28, 2016 • edited Loading

GuilhemN commented Dec 28, 2016

dunglas commented Dec 28, 2016

TomasVotruba commented Dec 30, 2016 • edited Loading

grachevko commented Dec 30, 2016

TomasVotruba commented Dec 31, 2016

grachevko commented Dec 31, 2016 • edited Loading

TomasVotruba commented Dec 31, 2016

fabpot commented Jan 7, 2017

dunglas commented Jan 7, 2017

fabpot commented Jan 7, 2017

chalasr commented Jan 8, 2017

nicolas-grekas commented Dec 27, 2016 •

edited

Loading

ogizanagi commented Dec 27, 2016 •

edited

Loading

ogizanagi commented Dec 27, 2016 •

edited

Loading

nicolas-grekas commented Dec 27, 2016 •

edited

Loading

nicolas-grekas commented Dec 27, 2016 •

edited

Loading

ro0NL Dec 27, 2016 •

edited

Loading

nicolas-grekas Dec 27, 2016 •

edited

Loading

dunglas commented Dec 28, 2016 •

edited

Loading

ro0NL commented Dec 28, 2016 •

edited

Loading

nicolas-grekas commented Dec 28, 2016 •

edited

Loading

nicolas-grekas commented Dec 28, 2016 •

edited

Loading

TomasVotruba commented Dec 30, 2016 •

edited

Loading

grachevko commented Dec 31, 2016 •

edited

Loading