-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Private services stay public when required by more than one service #12588
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
Comments
It looks like this problem is also occurs for |
Marking a service as private is simply giving a hint to the container builder that the service is not expected to be accessed directly, and therefore can be potentially inlined. See previous discussion on this in #2586. I think the problem is in documentation, which should emphasise more the fact that a private service is a hint for the container. It says "Now that the service is private, you cannot call: $container->get('foo')", but few paragraphs before there's a note mentioning that a service is only inlined if it's referenced by a single service. |
I created a doc issue to make this chapter clearer symfony/symfony-docs#4524. |
this is some kind of weird. the audience of the documentations are the developers, not the internals of the di container. so the question is why would someone like to give the container such a hint?
potentially is bad and if this doesn't belongs to the developer, i ask myself - why to use and teach about this feature. from my point of view the container is lacking a way of just exposing bunch of services. right now most applications are exposing a huge mess of useless internal services. |
I second the post of @timglabisch. I guess it isn't far fetched to think, that developers get the impression, the private attribute would help to clean up the 'huge mess of useless internal services'. But there is a bigger problem: You expose services, that might vanish some day without the developer using it (eg. by getting it inside a container aware controller) can do anything about it. E.g.: The developer sees a private service provided by the container in his/her controller and uses it. Later the bundle, that provided the service, drops one usage leaving just one usage in the container. Then the service magically disappears from the container. The developer of the bundle won't see a BC break, as he just changed the internal configuration and usage of a service he explicitly declared private. And the user won't recognize the break either until his/her application explodes at runtime when calling the action of the controller using the service. I see your point, that this is a documentation issue at the moment, but in my opinion, we should think about whether it wouldn't be better to hide private services from the public interface of the container in general to avoid the described problem. |
For anyone who might stumble upon this issue: It has been addressed in this PR: #19146 |
The documentation says, that a service, that you declare
private
, will not be available via theget
method of the compiled container (see: http://symfony.com/doc/current/components/dependency_injection/advanced.html)This is not true. A private service will only be removed by the
RemoveUnusedDefinitionsPass
compiler pass, if it is only used by one other service. In this case, the container doesn't need to keep track of the instance, as it can just instantiate one instance of the private service in the getter of the service in need and forget about it.But should the service be used by more than one service, the container keeps track of the lazily loaded instance by not removing the service.
This in itself isn't bad, but the problem is, that at this point, the container also provides the private service publicly to its users.
I've written two new test cases, which reproduce the problem. The first test case (
testPrivateServiceIsNotAccessibleWhenRequiredByOneOtherService
) is successful, the second one (testPrivateServiceIsNotAccessibleWhenRequiredByMoreThanOneOtherService
) fails, as the private service hasn't been removed.You can see the test cases here: https://github.com/bestform/symfony/compare/test-for-private-services
I would suggest that the container keeps track of the instances internally while forbidding the access from the outside to be consistent with the documentation and in general to be more intuitive. Private services should be private, no matter how the compilation pass of the container is able to optimize their usage.
The text was updated successfully, but these errors were encountered: