Make as many services private as possible #24104

nicolas-grekas · 2017-09-05T16:41:18Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	not yet
Fixed tickets	#23822
License	MIT
Doc PR	-

review the list of currently public services and validate which one should be turned private (noted as such with the container.private tag in this PR)
rebase on top of [FrameworkBundle] Fix Di config to allow for more private services #24103
implement the behavior described in Make as many services private as possible #23822 (comment)
add tests

List of services that will be kept public:

Remaining public aliases

cache.app_clearer <- for the cache:pool:clear command to clear app pools
$commandId <- for non-lazy commands which are get() at runtime
router <- for the base controller
templating <- for the base controller

Remaining public services

test.client <- for WebTestCase
security.password_encoder <- for use in ContainerAware classes
security.authentication_utils <- for use in ContainerAware classes
filesystem <- for use in ContainerAware classes
kernel <- for use in ContainerAware classes
translator <- for use in ContainerAware classes
validator <- for use in ContainerAware classes
cache_clearer <- for the cache:clear command
cache_warmer <- required to bootstrap the kernel
cache.app <- for userland only
cache.global_clearer <- for the cache:pool:clear command to clear all pools
cache.system <- for userland only
data_collector.dump <- required to have dump() work very early when booting the kernel
event_dispatcher <- required to wire console apps
form.factory <- for the base controller
http_kernel <- required to bootstrap the kernel
profiler <- used in tests
request_stack <- for the base controller
routing.loader <- used by routing
security.authorization_checker <- for the base controller
security.csrf.token_manager <- for the base controller
security.token_storage <- for the base controller
serializer <- for the base controller
session <- for the base controller
state_machine.abstract <- userland state machines
workflow.abstract <- userland workflows
twig <- for the base controller
twig.controller.exception <- controllers referenced by routing
twig.controller.preview_error <- controllers referenced by routing
var_dumper.cloner <- required to have dump() work very early when booting the kernel
web_profiler.controller.exception <- controllers referenced by routing
web_profiler.controller.profiler <- controllers referenced by routing
web_profiler.controller.router <- controllers referenced by routing

chalasr · 2017-09-07T11:22:08Z

REMAINING.PUBLIC.md

+security.acl.dbal.connection
+security.acl.provider
+security.encoder_factory
+security.firewall


it is an event subscriber, could be made private to me

chalasr · 2017-09-07T12:42:56Z

REMAINING.PUBLIC.md

+cache.app_clearer
+$commandId
+debug.templating.engine.php
+profiler.request_matcher


could be made private to me, only injected

chalasr · 2017-09-07T13:05:55Z

REMAINING.PUBLIC.md

+security.password_encoder
+session.handler
+session.storage
+session.storage.filesystem


flagged as "for BC" https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Resources/config/session.xml#L72..L73
should we deprecate it on 3.4? (can't find the PR)

chalasr · 2017-09-07T13:29:32Z

REMAINING.PUBLIC.md

+
+annotation_reader
+cache.app_clearer
+$commandId


must remain public, this is for non-lazy commands which are get() at runtime

stof · 2017-09-07T13:27:52Z

REMAINING.PUBLIC.md

+annotation_reader
+cache.app_clearer
+$commandId
+debug.templating.engine.php


the debug one does not need to be public IMO. You should never get it by its debug.* id (the templating.engine.php should stay public though)

stof · 2017-09-07T13:28:49Z

REMAINING.PUBLIC.md

+router
+security.access.decision_manager
+security.acl.cache
+security.acl.dbal.connection


this should not need to be public (if you need the DBAL connection yourselves dynamically, you should get it using the public ids given in DoctrineBundle)

stof · 2017-09-07T13:29:25Z

REMAINING.PUBLIC.md

+templating
+templating.loader
+translator
+twig.loader


even this one should become private IMO. There is no reason to access it dynamically at runtime IMO.

stof · 2017-09-07T13:38:10Z

REMAINING.PUBLIC.md

+cache.system
+cache_warmer <- required to bootstrap the kernel
+data_collector.cache
+data_collector.dump


Do collectors actually need to be public ? AFAIK, they are needed only by the profiler, and so could become private

stof · 2017-09-07T13:39:36Z

REMAINING.PUBLIC.md

+
+### Remaining public services
+
+cache.adapter.* <- pools are private by default when created by config so not a pb


I suggest adapters should become private in 4.0

stof · 2017-09-07T13:40:44Z

REMAINING.PUBLIC.md

+security.csrf.token_manager
+security.firewall
+security.token_storage
+security.validator.user_password


should become private, like other constraint validators

stof · 2017-09-07T13:41:38Z

REMAINING.PUBLIC.md

+serializer
+session
+state_machine.abstract <- state machines created by config are public
+workflow.abstract <- workflows created by config are public


can't we make them private instead ? The documented way to get them is to use the registry AFAIK, not to get the service from the container

stof · 2017-09-07T13:42:09Z

REMAINING.PUBLIC.md

+translator
+twig
+twig.controller.exception
+twig.controller.preview_error


these are required to be public due to the routing referencing them, right ?

stof · 2017-09-07T13:43:36Z

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

            <argument type="service" id="request_stack" />
        </service>

-        <service id="assets.path_package" class="Symfony\Component\Asset\PathPackage" abstract="true" public="true">
+        <service id="assets.path_package" class="Symfony\Component\Asset\PathPackage" abstract="true">
+            <tag name="container.private" />


the place where we actually need to apply this tag is in child services

nicolas-grekas · 2017-09-07T18:47:36Z

@chalasr @stof thanks for the first round of review, comments addressed.

In order to allow deprecating public aliases, I changed the implementation to not rely on a tag anymore. Instead, I added Alias/Definition::isPrivate(). The PR is ready for a second round review. It only needs tests now.

ro0NL

Awesome. Fixed rather elegant 👍 is setPrivate an internal detail now? should it?

nicolas-grekas · 2017-09-08T06:16:34Z

REMAINING.PUBLIC.md

+serializer <- for the base controller
+session <- for the base controller
+state_machine.abstract <- state machines created by config are public
+workflow.abstract <- workflows created by config are public


@lyrixx @Nyholm should we plan to make these services private in 4.0 and tell ppl that injecting workflow.registry is the way to go?

No. The registry is only here for convenience in twig.
Users should inject the right workflow in other services. Adding an extra layer will make the core harder to understand and to test. It's exactly like doctrine. People should inject the right repository, not the whole Doctrine Manager.

OK, then we can still make them private, isn't it?

@lyrixx if you want them to inject it in the service, it can be private. Public is here for dynamic get.

But if they are meant to be the main services of the workflow component, they should indeed stay public for now.

No, because we need to access it from a controller or a command.
Not everyone are using controller/command as a service.

nicolas-grekas · 2017-09-08T06:17:39Z

REMAINING.PUBLIC.md

+translation.extractor
+translation.loader
+translation.reader
+translation.writer


@Nyholm do you think we should plan making private these translation.*/translator services in 4.0?

translator must stay public. Otherwise, people using ContainerAware classes cannot use the translator layer anymore

IMHO, all theses services (above) could be private. except the security.authentication_utils (for use in ContainerAware classes)

nicolas-grekas · 2017-09-08T06:22:02Z

@ro0NL setPrivate is OK for eg bundles to me so they can do the same as we do here.

ro0NL · 2017-09-08T07:03:09Z

Guessed so. But are we sure about setPublic & setPrivate doing different things, which is here to stay for the long run. This requires documentation, but i think it needs a better name...

nicolas-grekas · 2017-09-08T07:07:34Z

setPublic & setPrivate doing different things

they don't need to do different things in 4.0, that's the solution.

nicolas-grekas · 2017-09-08T07:08:18Z

And in fact, they already don't do different things already - except for internal details.

ro0NL · 2017-09-08T07:17:22Z

Yeah but setPublic(false) wont enable isPrivate() === true.. right? That's unexpected.

nicolas-grekas · 2017-09-08T07:39:55Z

but setPublic(false) wont enable isPrivate() === true.. right

locally, that's correct - making it so would just discard any benefits of introducing it.
But the ResolveChildDefinitionsPass turns isPrivate() to setPublic(false) in the end, so semantically, they're the same.
The special behavior related to isPrivate() is 3.4-only and tailored to the purpose of making currently public services private in 4.0.
For this reason also, setPrivate() cannot be set via a (yaml/xml) loader - only a DI ext can. So only "advanced" users can use it.

stof · 2017-09-08T09:02:38Z

I think we need to improve the documentation about the difference between setPublic(false) and setPrivate(true) in the phpdoc (the phpdoc will be the place where advanced users will learn about it IMO, rather than learning it in symfony-docs, but the current phpdoc does not allow to understand the difference and why they should use one rather than the other)

nicolas-grekas · 2017-09-09T09:42:56Z

failures are false positives
list of remaining public services added to the PR's description and removed from patch
ready for vote+merge

javiereguiluz · 2017-09-09T09:50:58Z

Very nice work! Imagining myself as someone new to Symfony, I would have issues/doubts with these services:

Everything related to "cache clear": it looks like something internal to Symfony which I shouldn't care about

cache.app_clearer alias
cache_clearer <- for the cache:clear command
cache.global_clearer <- for the cache:pool:clear command to clear all pools

Controllers: all these services

twig.controller.*
web_profiler.controller.*

chalasr · 2017-09-09T10:31:43Z

@javiereguiluz We still have some places where we are forced to get() public services from the container internally, mostly places where we fetch unpredictable services which cannot be grouped by scope, like controllers which can be any service (no required tag), similar problem for those related to cache (have a look at CacheClearCommand/CachePoolClearCommand).
The services you listed must remain public.

ro0NL · 2017-09-09T11:12:32Z

command related services can be made private in 4.0 :)

mvrhov · 2017-09-09T14:00:18Z

If the controllers are the only big area which cannot be made private.. Maybe the controller tag should be introduced to make this possible?

nicolas-grekas · 2017-09-09T14:26:39Z

There is already a tag to flag services as controllers (controller.service_arguments).
BUT it works the other way around: the routing contains references to services.
So that the correct logic to do that would be to parse the routing config to then flag controllers as such.
That's not something we have right now, and I'm personally no planning to work on it at all: controllers as public services are just fine, there is nothing to "fix" here IMHO.

mvrhov · 2017-09-09T16:29:48Z

Just to complete this. But controller.service_arguments doesn't work for the controllers that are actually registered as services. And I find it weird that I'd have to tag with that when using "actions" for controllers e.g one class per action with __invoke and all dependencies injected via constructor.

nicolas-grekas · 2017-09-09T16:36:36Z

@mvrhov I don't know anymore what you're talking about :) You don't have to use that tag at all, that's what I said. You just have to make your controllers public.

javiereguiluz · 2017-09-09T16:41:20Z

@nicolas-grekas the thing that controllers must be public is more confusing than it looks. I can understand public services: you may inject this somewhere, so make it public. OK. But I don't inject or need or use or do anything with controllers. It's "a Symfony thing" and that's why I expect them to not be public.

nicolas-grekas · 2017-09-09T16:59:11Z

OK, I've no solution for that - and I don't think there is anything wrong in fact.

A service really needs to be public for one and only one reason: bootstrapping.
That's because the app must start with something.
There are at least three common bootstrapping contexts:

HTTP : here the entry point is routing thus controllers
CLI: the entry point is the Application thus commands
tests: the entry point is whatever you use in the body of your test cases

all other uses are the bad sort of service locator.

nicolas-grekas · 2017-09-13T08:02:10Z

ping @symfony/deciders, one vote missing :)

…kas) This PR was merged into the 3.4 branch. Discussion ---------- Make as many services private as possible | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | not yet | Fixed tickets | #23822 | License | MIT | Doc PR | - - [x] review the list of currently public services and validate which one should be turned private (noted as such with the `container.private` tag in this PR) - [x] rebase on top of #24103 - [x] implement the behavior described in #23822 (comment) - [x] add tests List of services that will be kept public: ### Remaining public aliases cache.app_clearer <- for the cache:pool:clear command to clear app pools $commandId <- for non-lazy commands which are get() at runtime router <- for the base controller templating <- for the base controller ### Remaining public services test.client <- for WebTestCase security.password_encoder <- for use in ContainerAware classes security.authentication_utils <- for use in ContainerAware classes filesystem <- for use in ContainerAware classes kernel <- for use in ContainerAware classes translator <- for use in ContainerAware classes validator <- for use in ContainerAware classes cache_clearer <- for the cache:clear command cache_warmer <- required to bootstrap the kernel cache.app <- for userland only cache.global_clearer <- for the cache:pool:clear command to clear all pools cache.system <- for userland only data_collector.dump <- required to have dump() work very early when booting the kernel event_dispatcher <- required to wire console apps form.factory <- for the base controller http_kernel <- required to bootstrap the kernel profiler <- used in tests request_stack <- for the base controller routing.loader <- used by routing security.authorization_checker <- for the base controller security.csrf.token_manager <- for the base controller security.token_storage <- for the base controller serializer <- for the base controller session <- for the base controller state_machine.abstract <- userland state machines workflow.abstract <- userland workflows twig <- for the base controller twig.controller.exception <- controllers referenced by routing twig.controller.preview_error <- controllers referenced by routing var_dumper.cloner <- required to have dump() work very early when booting the kernel web_profiler.controller.exception <- controllers referenced by routing web_profiler.controller.profiler <- controllers referenced by routing web_profiler.controller.router <- controllers referenced by routing Commits ------- 1936491 Make as many services private as possible

…h BC layer (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [DI] Turn services and aliases private by default, with BC layer | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #20048 | License | MIT | Doc PR | - With this PR, all services and aliases are made private by default. This is done in a BC way, thanks to the layer introduced in #24104. We will require bundles to explicitly opt-in for "public", either using "defaults", or stating `public="true"` explicitly. Same in DI extension, where calling `->setPublic(true)` will be required in 4.0. Commits ------- 9948b09 [DI] Turn services and aliases private by default, with BC layer

kbond · 2017-10-25T17:38:51Z

@nicolas-grekas After upgrading to 3.4, I'm getting this deprecation warning:

User Deprecated: The "routing.loader" service is private, getting it from the container is deprecated since Symfony 3.2 and will fail in 4.0. You should either make the service public, or stop using the container directly and use dependency injection instead.

This is the culprit:

symfony/src/Symfony/Bundle/FrameworkBundle/Routing/Router.php

Line 56 in 78426b7

    
           $this->collection = $this->container->get('routing.loader')->load($this->resource, $this->options['resource_type']);

Tobion · 2017-10-25T17:47:36Z

The routing.loader should be public. So that is strange.

kbond · 2017-10-25T17:50:17Z

Yeah, I discovered a bundle is at fault... Please ignore...

…tainer (see #1383). Description ----------- I searched the Contao source code for instances of `getContainer()->get('...');` occurrences, filtered out `contao` prefixed services and "uniquified" the resulting array. ``` assets.packages database_connection doctrine.dbal.default_connection event_dispatcher filesystem fragment.handler lexik_maintenance.driver.factory monolog.logger.contao request_stack router security.authentication_utils security.authorization_checker security.firewall.map security.logout_url_generator security.token_storage session swiftmailer.mailer translator ``` After intersecting this array to the one found in this ticket (future public services) [this ticket](symfony/symfony#24104), we end up with: ``` assets.packages database_connection doctrine.dbal.default_connection fragment.handler lexik_maintenance.driver.factory monolog.logger.contao security.firewall.map security.logout_url_generator swiftmailer.mailer ``` I added those to our `MakeServicesPublicPass` and made sure it will also find aliases instead of definitions only. Unfortunately I'm not sure if I really caught all direct service references. Do you have any other ideas? Commits ------- d6df8a8 Make all services public that we are retrieving directly from the container. 579c113 CS Fix a93083a Test all services in the unit test.

…the container (see contao#1383). Description ----------- I searched the Contao source code for instances of `getContainer()->get('...');` occurrences, filtered out `contao` prefixed services and "uniquified" the resulting array. ``` assets.packages database_connection doctrine.dbal.default_connection event_dispatcher filesystem fragment.handler lexik_maintenance.driver.factory monolog.logger.contao request_stack router security.authentication_utils security.authorization_checker security.firewall.map security.logout_url_generator security.token_storage session swiftmailer.mailer translator ``` After intersecting this array to the one found in this ticket (future public services) [this ticket](symfony/symfony#24104), we end up with: ``` assets.packages database_connection doctrine.dbal.default_connection fragment.handler lexik_maintenance.driver.factory monolog.logger.contao security.firewall.map security.logout_url_generator swiftmailer.mailer ``` I added those to our `MakeServicesPublicPass` and made sure it will also find aliases instead of definitions only. Unfortunately I'm not sure if I really caught all direct service references. Do you have any other ideas? Commits ------- d6df8a8a Make all services public that we are retrieving directly from the container. 579c1138 CS Fix a93083a2 Test all services in the unit test.

nicolas-grekas added this to the 3.4 milestone Sep 5, 2017

carsonbot added Status: Needs Review Feature Deprecation labels Sep 5, 2017

nicolas-grekas force-pushed the privates branch 5 times, most recently from 198fdbe to 28dac38 Compare September 7, 2017 12:44

chalasr reviewed Sep 7, 2017

View reviewed changes

nicolas-grekas force-pushed the privates branch from 28dac38 to 487a823 Compare September 7, 2017 13:43

stof reviewed Sep 7, 2017

View reviewed changes

nicolas-grekas force-pushed the privates branch 2 times, most recently from ae67484 to 0d55972 Compare September 7, 2017 18:45

nicolas-grekas force-pushed the privates branch 2 times, most recently from 4f8c41d to 89e8e28 Compare September 8, 2017 06:14

ro0NL reviewed Sep 8, 2017

View reviewed changes

nicolas-grekas commented Sep 8, 2017

View reviewed changes

nicolas-grekas force-pushed the privates branch from 89e8e28 to 39c36dd Compare September 8, 2017 06:20

nicolas-grekas force-pushed the privates branch from 39c36dd to 0706e20 Compare September 8, 2017 06:23

Make as many services private as possible

1936491

nicolas-grekas force-pushed the privates branch from a3b2743 to 1936491 Compare September 13, 2017 07:59

javiereguiluz approved these changes Sep 13, 2017

View reviewed changes

ogizanagi approved these changes Sep 13, 2017

View reviewed changes

nicolas-grekas merged commit 1936491 into symfony:3.4 Sep 13, 2017

nicolas-grekas deleted the privates branch September 13, 2017 15:05

nicolas-grekas mentioned this pull request Sep 17, 2017

[DI] Turn services and aliases private by default, with BC layer #24238

Merged

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

ogizanagi mentioned this pull request Nov 10, 2017

[FrameworkBundle][Routing] deprecation error when decorating routing.loader #24886

Closed

sheeep mentioned this pull request Feb 19, 2018

[RFC] Make all services public that we are retrieving directly from the container contao/core-bundle#1383

Merged

eugenekurasov mentioned this pull request Feb 28, 2018

#306. set public of 'translate' alias for compatibility with other components in SF4. lexik/LexikTranslationBundle#309

Merged


		### Remaining public services

		cache.adapter.* <- pools are private by default when created by config so not a pb

Make as many services private as possible #24104

Make as many services private as possible #24104

Conversation

nicolas-grekas commented Sep 5, 2017 • edited Loading

Remaining public aliases

Remaining public services

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 commented Sep 7, 2017

ro0NL left a 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 commented Sep 8, 2017

ro0NL commented Sep 8, 2017

nicolas-grekas commented Sep 8, 2017 • edited Loading

nicolas-grekas commented Sep 8, 2017 • edited Loading

ro0NL commented Sep 8, 2017

nicolas-grekas commented Sep 8, 2017 • edited Loading

stof commented Sep 8, 2017

nicolas-grekas commented Sep 9, 2017

javiereguiluz commented Sep 9, 2017

chalasr commented Sep 9, 2017

ro0NL commented Sep 9, 2017

mvrhov commented Sep 9, 2017

nicolas-grekas commented Sep 9, 2017 • edited Loading

mvrhov commented Sep 9, 2017 • edited Loading

nicolas-grekas commented Sep 9, 2017

javiereguiluz commented Sep 9, 2017

nicolas-grekas commented Sep 9, 2017 • edited Loading

nicolas-grekas commented Sep 13, 2017

kbond commented Oct 25, 2017

Tobion commented Oct 25, 2017

kbond commented Oct 25, 2017

nicolas-grekas commented Sep 5, 2017 •

edited

Loading

nicolas-grekas commented Sep 8, 2017 •

edited

Loading

nicolas-grekas commented Sep 8, 2017 •

edited

Loading

nicolas-grekas commented Sep 8, 2017 •

edited

Loading

nicolas-grekas commented Sep 9, 2017 •

edited

Loading

mvrhov commented Sep 9, 2017 •

edited

Loading

nicolas-grekas commented Sep 9, 2017 •

edited

Loading