[DependencyInjection] Private service definition documentation is not clear enough

[jakzal]

Ocasionally we're getting reports like symfony/symfony#12588 or symfony/symfony#2586.

I think the Advanced DI chapter should emphasise more the fact that a private service is a hint for the container, and the rule is not enforced when retrieving the service from container. The doc currently 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.

Partly related to #4228

[xabbuh]

@jakzal I agree with @timglabisch here that this behaviour is actually weird. Why should I as the developer care about the container internals? Marking a service as private to me means that it cannot be retrieved later from the container. What is the point in not checking this (besides maybe backward compatibility)?

[stof]

@jakzal "you cannot call" means you have no way to know whether calling it will work or no (except by looking at how many places the private service is referenced).

And we are not checking it at runtime, because it would force us to check whether ->get() was called from inside the container or outside, which would be a pain. Thus, it would add some overhead at runtime (forcing to do these checks). Currently, we don't even have the info available at runtime.
So to be clear, the main point for not checking it at runtime is performance.

Note however that checking this would be a perfect fit for static analysis tool like SensioLabsInsight or Scrutinizer (IIRC, the PhpStorm plugin already warns about this btw)

[jakzal]

@stof I know what it means, but since people keep getting confused, this could probably be phrased better.

[bestform]

@stof a possible solution for this problem would be to let the compiled (but not dumped) container check the requested service for its visibility at runtime (at the mentioned cost of performance). When dumping the container you can create private getters for the private services. When you create the call to the getter (as dependency for a public service) you know that you are requesting a private service and just generate a call to the private getter instead of the public one.
This way you won't have any performance impact when using the dumped container - which would be the case in nearly every situation.

[wouterj]

In my opinion, the current docs are indeed confusing (I didn't know this behaviour of private services and I've understand the concept incorrectly until this issue).

We should at least add a .. caution:: or .. sidebar:: directive, explaining what "private" actually means.

[timglabisch]

👍 but why not starting to deprecating the public flag in favour of something better?

[stof]

@timglabisch what could be something better ?

@bestform the issue is that this would require splitting the storage of service instances depending on the visibility too, making things more complex

[timglabisch]

@stof something better would be providing a strategy that allows you to just expose a bunch of services from a bundle. i did some research on this topic to enforce dependency rules between bundles and between software layers. like bundleA cant use services from bundleB. Or controllers can't use the data layer. this is a very complex topic.

[stof]

@timglabisch service definitions are not tied to a bundle (this would require introducing the concept of bundles into the container, which would be much more complex). Being public or private is related to being retrieved dynamically at runtime through $container->get() or being intended to be retrieved only through references between services. The advantage of private services is that the container knows all places needing to get the service, and so can perform some optimizations of the container based on that.
Restricting services to a given bundle has nothing to do with this feature. It is a totally different feature.

[timglabisch]

@stof thats correct, as @jakzal said "I think the Advanced DI chapter should emphasise more the fact that a private service is a hint for the container".

from my point of view private should documented as strategy for internal performance improvement. But if it's just up to performance improvements why not think about removing/renaming this feature or using a more natural api. people are getting confused by adding a public=false flags to services, most time they don't want to optimize the container, they want to hide/protect a service.