[DI] Defaults to public=false in all service config files #22615

nicolas-grekas · 2017-05-02T19:23:27Z

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

This is what we call "eating your own dog food" :)
Made me realize that we need a tweak to the defaults<>ChildDefinition conflict we have now:
tags should be applied, and there should be no conflict when everything is set explicitly on the child definition.

nicolas-grekas · 2017-05-02T19:46:17Z

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

+                }
+                if (!$service->hasAttribute($k)) {
+                    throw new InvalidArgumentException(sprintf('Attribute "%s" on service "%s" cannot be inherited from "defaults" when a "parent" is set. Try moving your child definitions to a different file or defining this attribute explicitly.', $k, $service->getAttribute('id')));
+                }


this block is the main and only behavior change of this PR (same on YamlFileLoader)

fabpot · 2017-05-03T00:52:23Z

src/Symfony/Component/DependencyInjection/Tests/Loader/XmlFileLoaderTest.php

@@ -713,7 +713,7 @@ public function testAutoConfigureAndChildDefinitionNotAllowed()

    /**
     * @expectedException \Symfony\Component\DependencyInjection\Exception\InvalidArgumentException
-     * @expectedExceptionMessage The service "child_service" cannot use the "parent" option in the same file where "defaults" configuration is defined as using both is not supported. Try moving your child definitions to a different file.
+     * @expectedExceptionMessage Attribute "autowire" on service "child_service" cannot be inherited from "defaults" when a "parent" is set. Try moving your child definitions to a different file or defining this attribute explicitly.


I would say: Move your child definitions to a different file or define this attribute explicitly.

updated (and test case added)

stof · 2017-05-03T07:32:28Z

src/Symfony/Bundle/DebugBundle/Resources/config/services.xml

@@ -20,19 +22,19 @@
            <argument>null</argument><!-- var_dumper.cli_dumper when debug.dump_destination is set -->
        </service>

-        <service id="debug.dump_listener" class="Symfony\Component\HttpKernel\EventListener\DumpListener">
+        <service id="debug.dump_listener" class="Symfony\Component\HttpKernel\EventListener\DumpListener" public="true">


it would be great to be able to change this service to become private in 4.0 (as event listeners can now be private).
@nicolas-grekas I thnk a necessary feature for 3.4 would be a way to mark a service as "wanna-be-private", which would trigger a deprecation warning when getting it from the container through get, but not when injecting it inside another service (unlike deprecated services). Such feature would allow any bundle out there to provide gradual migration to private services (warning people if they try to access internal services that will become private)

100% agree, and this PR is on that path :)
No need for any new feature as there is already a trick to do the deprecation, see:

symfony/src/Symfony/Bundle/FrameworkBundle/Resources/config/form.xml

Line 192 in f2f9aa6

<service id="deprecated.form.registry" class="stdClass">

So, we should just turn these services as private, then register them in a public dummy registry, that will prevent inlining and will thus make them "public" (but in a deprecated manner).

no, the form types are deprecating the services entirely, because they won't be needed anymore in 4.0 (form types without any dependencies don't need to be registered explicitly anymore).

But we might have a case where we want to deprecate the runtime retrieval of services (to make them private) without deprecating the service itself (as you will want to keep it as a dependency of other services), which is what we need for listeners.

And making them public in a deprecated manner works for Symfony services in 3.4.x. but it does not work for the future of Symfony, because your proposal relies on the BC layer for accessing non-inlined private services. But Symfony 4 will not make such services public at all, even when not inlined, and so you cannot have a similar BC layer for the migration to private services.

Btw, if you want to be sure that the services stay public, you must create 2 dummy registries. If you create only one, the service may end up being inlined inside the dummy registry if other references get removed by some optimizations.

The form types I linked are the ones that are not deprecated because they need special instantiation.
You're right that this trick is valid only on 3.4. But maybe 3.4 is the last one that will have public services? So if using the trick is enough for 3.4, then maybe better not to work on a new feature for nothing?

Anyway, that's an interesting discussion, but should not be hidden in this PR :)

stof · 2017-05-03T07:33:26Z

src/Symfony/Bundle/DebugBundle/composer.json

@@ -23,9 +23,12 @@
    },
    "require-dev": {
        "symfony/config": "~2.8|~3.0",
-        "symfony/dependency-injection": "~2.8|~3.0",
+        "symfony/dependency-injection": "~3.3",


shouldn't it be a normal requirement ? The bundle is all about defining services anyway.

In fact, DebugBundle can and is used standalone on Silex, see
https://packagist.org/packages/jeromemacias/silex-debug

hmm indeed, they get it to get the template for the profiler, which is not in WebProfilerBundle unlike other collectors

stof · 2017-05-03T07:35:42Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/annotations.xml

@@ -18,27 +20,27 @@
                </argument>
            </call>
        </service>
-        <service id="Doctrine\Common\Annotations\Reader" alias="annotations.reader" public="false" />
+        <service id="Doctrine\Common\Annotations\Reader" alias="annotations.reader" />


@nicolas-grekas this autowiring alias should point to annotation_reader, which is the public name for it (even though it is an alias), so that people decorating the public name still get it applied in case of autowiring.

stof · 2017-05-03T07:37:23Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/cache_debug.xml

@@ -5,6 +5,8 @@
    xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

    <services>
+        <defaults public="false" />
+
        <!-- DataCollector -->
        <service id="data_collector.cache" class="Symfony\Component\Cache\DataCollector\CacheDataCollector">


this changes the visibility of this collector. Is it expected ?

yes, the service does not need to be public

stof · 2017-05-03T07:45:02Z

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -66,6 +66,7 @@
        "phpunit/phpunit": "<4.8.35|<5.4.3,>=5.0",
        "symfony/asset": "<3.3",
        "symfony/console": "<3.3",
+        "symfony/dependency-injection": "<=3.3-beta1",


As we have a requirement on it, just update the lower bound of the requirement. It is more efficient in the composer solver than adding a conflict rule.

stof · 2017-05-03T07:47:37Z

src/Symfony/Bundle/SecurityBundle/composer.json

@@ -44,6 +44,7 @@
        "twig/twig": "~1.28|~2.0"
    },
    "conflict": {
+        "symfony/dependency-injection": "<=3.3-beta1",


update the requirement lower bound instead

nicolas-grekas · 2017-05-03T15:57:11Z

PR should be ready: comments addressed, and integration test case from Ryan added.

weaverryan · 2017-05-03T17:43:24Z

I LOVE the idea of eating our own dog food.

👍 for the minor ChildDefinition -> _defaults behavior change. You can see the new behavior by comparing main.yml to expected.yml in the integration test: https://github.com/symfony/symfony/pull/22615/files#diff-e36b91c9d48d3617ee8a401577fcc603. Having a child definition in the same file as _defaults wasn't allowed at ALL anymore. Now it is, but only if you explicitly set any keys from _defaults on the child (i.e. we force you to override _defaults values (other than tags) so that there is no ambiguity).

And 👍 for the changes in general. I compiled an XML of the container on master versus this branch. They are identical, other than data_collector.cache, which was purposefully changed from public to private: https://gist.github.com/weaverryan/353a79c0e310b59d5eca9c922bc5cf5f

fabpot · 2017-05-03T19:17:03Z

Thank you @nicolas-grekas.

…(nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Defaults to public=false in all service config files | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is what we call "eating your own dog food" :) Made me realize that we need a tweak to the defaults<>ChildDefinition conflict we have now: tags should be applied, and there should be *no* conflict when everything is set *explicitly* on the child definition. Commits ------- 0656284 [DI] Defaults to public=false in all service config files

…ublic (ogizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [TwigBundle] service workflow.twig_extension should stay public | Q | A | ------------- | --- | Branch? | master (3.3) | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A While it's not really required for this service to be public AFAIK, it was not made private when introducing it in 3.2. Which means it should stay public in upper branches. But [since we now default to `public: false`](#22615) in every service config file, it's currently private in 3.3. I had a quick look on commits merged after this PR and didn't find any other similar case. Commits ------- 5d07c6a [TwigBundle] service workflow.twig_extension should stay public

alcaeus · 2017-05-25T17:40:24Z

@nicolas-grekas this change seems to have introduced a rather annoying deprecation warning:

Checking for the existence of the "debug.file_link_formatter" private service is deprecated since Symfony 3.2 and won't be supported anymore in Symfony 4.0: (383x)

This seems to come from the DebugBundle, where the service is injected in a method call using the on-invalid=ignore flag:

        <service id="var_dumper.html_dumper" class="Symfony\Component\VarDumper\Dumper\HtmlDumper">
            <argument>null</argument>
            <argument>%kernel.charset%</argument>
            <argument>0</argument> <!-- flags -->
            <call method="setDisplayOptions">
                <argument type="collection">
                    <argument key="fileLinkFormat" type="service" id="debug.file_link_formatter" on-invalid="ignore"></argument>
                </argument>
            </call>
        </service>

Apparently, when the compiled container is dumped to a file, this causes a $container->has call on the private service.

    protected function getTwigService()
    {
        // ...

        $d = new \Symfony\Component\VarDumper\Dumper\HtmlDumper(NULL, 'UTF-8', 0);
        if ($this->has('debug.file_link_formatter')) {
            $d->setDisplayOptions(array('fileLinkFormat' => $b));
        }

        // ...
    }

Should the debug.file_link_formatter service be made public or are you intending to ship 3.3 with this deprecation?

nicolas-grekas · 2017-05-25T17:52:36Z

See #22912

alcaeus · 2017-05-25T17:54:35Z

Ah, my search didn't turn up any results, so I wasn't aware if this was being tracked. Thanks!

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

carsonbot added Status: Needs Review DependencyInjection Bug labels May 2, 2017

nicolas-grekas force-pushed the di-defaults branch 2 times, most recently from d323232 to d80354d Compare May 2, 2017 19:45

nicolas-grekas commented May 2, 2017

View reviewed changes

fabpot reviewed May 3, 2017

View reviewed changes

nicolas-grekas force-pushed the di-defaults branch from d80354d to 78edb66 Compare May 3, 2017 07:09

stof reviewed May 3, 2017

View reviewed changes

stof requested changes May 3, 2017

View reviewed changes

nicolas-grekas force-pushed the di-defaults branch 3 times, most recently from ff6f36a to 259a606 Compare May 3, 2017 15:52

nicolas-grekas force-pushed the di-defaults branch from 259a606 to e2fe3f5 Compare May 3, 2017 16:51

[DI] Defaults to public=false in all service config files

0656284

nicolas-grekas force-pushed the di-defaults branch from e2fe3f5 to 0656284 Compare May 3, 2017 17:25

fabpot merged commit 0656284 into symfony:master May 3, 2017

nicolas-grekas deleted the di-defaults branch May 4, 2017 12:17

fabpot mentioned this pull request May 17, 2017

Release v3.3.0-RC1 #22731

Merged

sroze mentioned this pull request May 30, 2018

[Serializer][PropertyInfo] Missing PropertyTypeExtractorInterface alias #27139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI] Defaults to public=false in all service config files #22615

[DI] Defaults to public=false in all service config files #22615

nicolas-grekas commented May 2, 2017 •

edited

Loading

nicolas-grekas May 2, 2017 •

edited

Loading

fabpot May 3, 2017

nicolas-grekas May 3, 2017

stof May 3, 2017

nicolas-grekas May 3, 2017

stof May 3, 2017

nicolas-grekas May 3, 2017 •

edited

Loading

stof May 3, 2017

nicolas-grekas May 3, 2017

stof May 3, 2017

stof May 3, 2017

stof May 3, 2017

nicolas-grekas May 3, 2017

stof May 3, 2017

stof May 3, 2017

nicolas-grekas commented May 3, 2017

weaverryan commented May 3, 2017

fabpot commented May 3, 2017

alcaeus commented May 25, 2017

nicolas-grekas commented May 25, 2017

alcaeus commented May 25, 2017

[DI] Defaults to public=false in all service config files #22615

[DI] Defaults to public=false in all service config files #22615

Conversation

nicolas-grekas commented May 2, 2017 • edited Loading

nicolas-grekas May 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

nicolas-grekas May 3, 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

nicolas-grekas commented May 3, 2017

weaverryan commented May 3, 2017

fabpot commented May 3, 2017

alcaeus commented May 25, 2017

nicolas-grekas commented May 25, 2017

alcaeus commented May 25, 2017

nicolas-grekas commented May 2, 2017 •

edited

Loading

nicolas-grekas May 2, 2017 •

edited

Loading

nicolas-grekas May 3, 2017 •

edited

Loading