New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make as many services private as possible #24104

Merged
merged 1 commit into from Sep 13, 2017

Conversation

@nicolas-grekas
Member

nicolas-grekas commented Sep 5, 2017

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 #24103
  • implement the behavior described in #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

Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated src/Symfony/Bundle/FrameworkBundle/Resources/config/assets.xml
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 7, 2017

Member

@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.

Member

nicolas-grekas commented Sep 7, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 8, 2017

Member

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

Member

nicolas-grekas commented Sep 8, 2017

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

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 8, 2017

Contributor

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...

Contributor

ro0NL commented Sep 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 8, 2017

Member

setPublic & setPrivate doing different things

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

Member

nicolas-grekas commented Sep 8, 2017

setPublic & setPrivate doing different things

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 8, 2017

Member

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

Member

nicolas-grekas commented Sep 8, 2017

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

@ro0NL

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 8, 2017

Contributor

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

Contributor

ro0NL commented Sep 8, 2017

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 8, 2017

Member

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.

Member

nicolas-grekas commented Sep 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 8, 2017

Member

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)

Member

stof commented Sep 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 8, 2017

Member

PR is now ready and tested.
Please help validate the list of still public services/aliases (see PR)

Member

nicolas-grekas commented Sep 8, 2017

PR is now ready and tested.
Please help validate the list of still public services/aliases (see PR)

Show outdated Hide outdated REMAINING.PUBLIC.md
Show outdated Hide outdated REMAINING.PUBLIC.md
@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 8, 2017

Member

All services are now either private, or public with a justification notice (see PR.)
Calling @symfony/deciders for review+votes before I remove that list of public services from the patch.

Member

nicolas-grekas commented Sep 8, 2017

All services are now either private, or public with a justification notice (see PR.)
Calling @symfony/deciders for review+votes before I remove that list of public services from the patch.

@chalasr

chalasr approved these changes Sep 9, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 9, 2017

Member

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

Member

nicolas-grekas commented Sep 9, 2017

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

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 9, 2017

Member

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

  1. 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

  1. Controllers: all these services

twig.controller.*
web_profiler.controller.*

Member

javiereguiluz commented Sep 9, 2017

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

  1. 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

  1. Controllers: all these services

twig.controller.*
web_profiler.controller.*

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Sep 9, 2017

Member

@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.

Member

chalasr commented Sep 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ro0NL

ro0NL Sep 9, 2017

Contributor

command related services can be made private in 4.0 :)

Contributor

ro0NL commented Sep 9, 2017

command related services can be made private in 4.0 :)

@mvrhov

This comment has been minimized.

Show comment
Hide comment
@mvrhov

mvrhov Sep 9, 2017

Contributor

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

Contributor

mvrhov commented Sep 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 9, 2017

Member

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.

Member

nicolas-grekas commented Sep 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@mvrhov

mvrhov Sep 9, 2017

Contributor

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.

Contributor

mvrhov commented Sep 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 9, 2017

Member

@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.

Member

nicolas-grekas commented Sep 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Sep 9, 2017

Member

@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.

Member

javiereguiluz commented Sep 9, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 9, 2017

Member

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.

Member

nicolas-grekas commented Sep 9, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 13, 2017

Member

ping @symfony/deciders, one vote missing :)

Member

nicolas-grekas commented Sep 13, 2017

ping @symfony/deciders, one vote missing :)

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

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

nicolas-grekas added a commit that referenced this pull request Sep 13, 2017

feature #24104 Make as many services private as possible (nicolas-gre…
…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

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:privates branch Sep 13, 2017

nicolas-grekas added a commit that referenced this pull request Sep 19, 2017

feature #24238 [DI] Turn services and aliases private by default, wit…
…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

This was referenced Oct 18, 2017

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 25, 2017

Contributor

@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:

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

Contributor

kbond commented Oct 25, 2017

@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:

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

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Oct 25, 2017

Member

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

Member

Tobion commented Oct 25, 2017

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

@kbond

This comment has been minimized.

Show comment
Hide comment
@kbond

kbond Oct 25, 2017

Contributor

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

Contributor

kbond commented Oct 25, 2017

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

leofeyer added a commit to contao/core-bundle that referenced this pull request Mar 2, 2018

Make all services public that we are retrieving directly from the con…
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment