Skip to content
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

Public service, very vague #6959

Closed
appeltaert opened this issue Sep 11, 2016 · 4 comments
Closed

Public service, very vague #6959

appeltaert opened this issue Sep 11, 2016 · 4 comments

Comments

@appeltaert
Copy link
Contributor

http://symfony.com/doc/current/service_container/alias_private.html

What makes private services special is that, if they are only injected once, they are converted from services to inlined instantiations (e.g. new PrivateThing()). This increases the container's performance.

It seems that a private service is only inline instantiated if there is only one service depending on it which is not shared(shared:false), couldn't find any other use case..

Now that the service is private, you should not fetch the service directly from the container:
This may or may not work, depending on if the service could be inlined. Simply said: A service can be marked as private if you do not want to access it directly from your code

Whether the private service is inline instantiated or not, or even used or not, i couldn't find any use-case where i could get it manually from the container.

Services are by default public, but it's considered a good practice to mark as much services private as possible.

For something that's considered a good practise, it would be good to have a little insight on what's going on. You would expect a private service to simply not be callable from the container, let alone "it may or may not". It doesn't seem to have any use besides the small performance boost, and the vague behaviour you get in return doens't seem worth it.

It feels like poor design and not good practise at all, but maybe someone can improve the docs. I looked into the file that has the conditions, but they contain no inline docs with some sort of clarification.

https://github.com/symfony/dependency-injection/blob/master/Compiler/InlineServiceDefinitionsPass.php#L108

This would feel like a more solid feature doing only what it suggests, namely the only possibility for it to be injected into another service, no side-effects or performance boosts

@sstok
Copy link
Contributor

sstok commented Sep 29, 2016

The implementation is good as it is, but the description needs updating.

The most important thing: A private service is never exposed for direct usage ($container->get('')) after the Container is compiled. You can only reference it in other service definitions, which are then compiled.

Marking a service private gives a tip to the Compiler that it can be optimized (unless it's referenced more then once).

The inlining of a private service is a detail, which you should not rely on. There was a little leakage in the implementation were a private service that was referenced more then once, was still accessible but this is deprecated in Symfony 3.2 and will not be possible anymore in 4.0 (unless you can guess the internal service-id).

There is actually a proposal to mark services as private by default.
symfony/symfony#20048

wouterj added a commit that referenced this issue Oct 20, 2016
…vate services (lemoinem)

This PR was merged into the master branch.

Discussion
----------

[ServiceContainer] Remove implementation details of private services

Since symfony/symfony#19708, getting private services through `Container::get()` is deprecated in the cases where it worked up to now, and this will removed in 4.0. The ways a container optimizes private services instanciation are not useful information anymore and is confusing to some (see #6959). I removed the mentions of these implementations details to make the documentation clearer.

Commits
-------

a529157 [ServiceContainer] Remove any references to the implementation details of private services
@xabbuh
Copy link
Member

xabbuh commented Oct 21, 2016

Hey @appeltaert, thanks for opening this issue. We made some improvements to the description in #7021. Do you think they make it more clear or do you see room for further improvements? Thanks for your feedback!

@appeltaert
Copy link
Contributor Author

Hi @xabbuh, thanks for letting me know.

Looks good, can I suggest a simple change? It makes sense once you know the inner workings, but if I would read it with the knowledge of a few weeks ago, I would brainfart the same way..

It's mostly because of this line:

Now that the ... you *_should not_* fetch

With the following paragraph saying:

This _may or may not work_, depending ...

Would it not be good to add a tip box stating that it's removed in 4.0? This will divert most people from asking 'why, how, what' questions, because in the end it's in the docs, this must mean that it has it's use..

If i would've read a such a tip box i would stop questioning everything said, and simply not do it, yet stil--for those who really need the information, it it's still there. At the same time, it raises awareness that it's not good practise to do it even if it's possible.

@xabbuh
Copy link
Member

xabbuh commented Jan 28, 2018

@appeltaert It took me so long, but here we go now: #9157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants