Make as many services private as possible #23822

nicolas-grekas · 2017-08-07T20:16:00Z

Q	A
Bug report?	no
Feature request?	yes
BC Break report?	no
RFC?	no
Symfony version	3.4

There are many services which are public right now but have not reason to be.

javiereguiluz · 2017-08-08T13:58:44Z

Can we make a list of those services so the community can submit PRs making them private ... or do you prefer to work on this yourself? Thanks.

Simperfit · 2017-08-08T14:02:50Z

I think we can make a list, it will a lot of work for only one person :)

javiereguiluz · 2017-08-08T14:13:05Z

In the Symfony Demo upgraded to Symfony 3.3.6 and Symfony Flex I see a lot of services that could be removed. For example, most of these services related to translation:

Service ID	Service class
translation.dumper.csv	Symfony\Component\Translation\Dumper\CsvFileDumper
translation.dumper.ini	Symfony\Component\Translation\Dumper\IniFileDumper
translation.dumper.json	Symfony\Component\Translation\Dumper\JsonFileDumper
translation.dumper.mo	Symfony\Component\Translation\Dumper\MoFileDumper
translation.dumper.php	Symfony\Component\Translation\Dumper\PhpFileDumper
translation.dumper.po	Symfony\Component\Translation\Dumper\PoFileDumper
translation.dumper.qt	Symfony\Component\Translation\Dumper\QtFileDumper
translation.dumper.res	Symfony\Component\Translation\Dumper\IcuResFileDumper
translation.dumper.xliff	Symfony\Component\Translation\Dumper\XliffFileDumper
translation.dumper.yml	Symfony\Component\Translation\Dumper\YamlFileDumper
translation.extractor	Symfony\Component\Translation\Extractor\ChainExtractor
translation.extractor.php	Symfony\Bundle\FrameworkBundle\Translation\PhpExtractor
translation.loader	Symfony\Bundle\FrameworkBundle\Translation\TranslationLoader
translation.loader.csv	Symfony\Component\Translation\Loader\CsvFileLoader
translation.loader.dat	Symfony\Component\Translation\Loader\IcuDatFileLoader
translation.loader.ini	Symfony\Component\Translation\Loader\IniFileLoader
translation.loader.json	Symfony\Component\Translation\Loader\JsonFileLoader
translation.loader.mo	Symfony\Component\Translation\Loader\MoFileLoader
translation.loader.php	Symfony\Component\Translation\Loader\PhpFileLoader
translation.loader.po	Symfony\Component\Translation\Loader\PoFileLoader
translation.loader.qt	Symfony\Component\Translation\Loader\QtFileLoader
translation.loader.res	Symfony\Component\Translation\Loader\IcuResFileLoader
translation.loader.xliff	Symfony\Component\Translation\Loader\XliffFileLoader
translation.loader.yml	Symfony\Component\Translation\Loader\YamlFileLoader
translation.writer	Symfony\Component\Translation\Writer\TranslationWriter
translator	alias for "translator.data_collector"
translator.default	Symfony\Bundle\FrameworkBundle\Translation\Translator
translator_listener	Symfony\Component\HttpKernel\EventListener\TranslatorListener

stof · 2017-08-08T14:14:21Z

there is an issue about doing this in 3.4: if they get referenced only once or not at all, they will be inlined and removed, which forbids accessing them through get even through the BC layer. And this is a BC break.

Btw, we don't have any upgrade path for switching a service to private (be it a core service of an internal services). I suggest adding a feature for that.
Public services and aliases could get a new flag on them (no idea for the name though), which would signal Symfony that we plan to switch it to private in the future. Then, when doing a get on this service, the container would first check if the id is in the list of such services and trigger a deprecation warning.
Doing it in get would mean that it adds some overhead to all get calls. I don't know how to improve this (especially as getting a -to-become-private public service may not trigger the instantiation if already done)

stof · 2017-08-08T14:16:05Z

@javiereguiluz The translator alias should stay public IMO, as it is the main API of the translation system.
About translation.loader translation.writerandtranslation.extractor`, I'm not sure. All others can indeed become private (they were public for pre-3.3 lazy-loading)

nicolas-grekas · 2017-08-08T14:17:40Z

@stof there is another option, which is much lighter: we create two special public services, deprecated ones, that references them all.
Then we can safely make them private so that people will get a notice in 3.4.

stof · 2017-08-08T14:20:53Z

@nicolas-grekas note that the feature I'm talking about to deprecate public access to services should not go away in 4.0, as 4.x+ of Symfony may still want to do such changes in the future (and then, we don't even have the private get BC layer for private services referenced multiple times), and external bundles will have the same need too (once they start dropping support for Symfony < 3.4, most of their services won't need to be public either)

Your proposal about using a special deprecated public services is not fine:

it actually requires 2 such services (for cases of public services referenced nowhere that you want to make private)
it only works for Symfony 3.3 and 3.4. Once people are on Symfony 4, it does not work. so it is not a solution for bundles (bundles won't drop support for Symfony < 3.4 until after the Symfony 4 release, so they need a migration path compatible with Symfony 4)

nicolas-grekas · 2017-08-08T14:24:46Z

Of course, I was seeking for the laziest plan :)
If anyone wants to implement the feature, PR welcomed ;)

sstok · 2017-08-08T14:37:12Z

To really make translations loading lazy we'd need something like this #17563 right?

chalasr · 2017-08-14T15:30:10Z

👍 for introducing what @stof suggests, it would be useful for many third party bundles for sure.

nicolas-grekas · 2017-09-05T15:16:53Z

Here is the plan:

add a tag (container.private?) that express the private-in-next-major state
bundles would keep these services as public for <=3.3 compat, but we would turn them private because we can
add a compiler pass that turns these into private, with double registration trick to trigger the existing deprecation about private services no being fetchable anymore in 4.0
in 4.0, this compiler pass would just turn these services to private
in 4.1, we deprecate the container.private tag and the corresponding pass

That would allow bundles to still have the services public with <3.3, trigger the deprecation with 3.4, and have the services really private when used with 4.0.

(I'm still trying to not have to create and maintain a feature to move from public to private on the long term)

nicolas-grekas · 2017-09-05T15:20:16Z

Some comments posted on #23867 (which is now closed):

From @Tobion:

Just some thoughts: The whole concept of private services is only to shrink the container. So basically it's just implementation detail and ideally as developer I should not need to care about this.
So IMO in an ideal world there is no such thing as private and public services.

Can't we achieve that goal? Now that the container creates files per factory, the container should scale better with the amout of services, right? Can't we improve that even more so that the overhead per service is negligible? E.g. less internal tracking of serivce ID mapping to file name/method name.
Then we don't care about private or public at all.

I think the only difference are inlined services. If we'd say everything is public, there is no private anymore, the container would need to keep a reference to the inlined services. But does this really matter?

Then from @fabpot:

I like @Tobion thoughts a lot :)

Then me:

In Flex, we made the move of loading everything in the src/ folder because services where private by default - so they would be cleaned up anyway.

stof · 2017-09-05T15:24:08Z

The difference for private services is not only the inlining, but also the removal indeed (as the container is able to ensure that they won't ever be needed, as they are forbidden for dynamic retrieval).
So I don't think we can remove this distinction

Tobion · 2017-09-05T16:44:22Z

@stof if there was no performance difference between removing or not removing private services, then the distinction is not neccessary.

@lyrixx

…e services (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle] Fix Di config to allow for more private services | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This PR is a prelude to #23822: - allow cache pool clearers to be private by using IGNORE_ON_UNINITIALIZED_REFERENCE, allowing much more sane logic in the related passes - allow templating helpers to be private by using a service locator - replace an inline def by a reference in workflow's config (ping @lyrixx @Nyholm) - fix the Stopwatch alias, which is public in 3.4, but private in 3.3 - remove direct access to `->get('fragment.handler')` in tests so that the service could be made private Commits ------- 76c4217 [FrameworkBundle] Fix Di config to allow for more private services

…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

sroze · 2017-10-02T13:48:49Z

My thoughts after trying dev-master on the symfony-demo project... I think this is asking for troubles. On such a small project, I had do these 2 things to have the application working:

Update SymfonyBundle to have the doctrine service accessible from the ControllerTrait
Update pagerfanta's white_october_pagerfanta.view_factory service to public="true"

This means almost all the Symfony libraries will have to be updated for Symfony 4... Did we measure what is the performance improvement on such change that would justify this? :)

nicolas-grekas · 2017-10-02T13:54:15Z

It's not only about perf, it's also a lot about design. Bundles don't care about this public/private difference, so that everything is usually public, thus open to service location.
All bundles should have made most their services private from the beginning (as we try to do in core). But having everything public by default just makes this a dream.
Turning to private by default fixes this.
Time now to submit PRs to bundles and make them either explicitly opt-in for public on a case by case basis, or have them leverage (new DI ways of providing) regular injection.

stof · 2017-10-02T13:55:04Z

@sroze the fact that the default visibility of services is changing indeed means that bundles have to be updated. But they can be updated in 2 different ways (depending on the services):

marking them as public explicitly if it is actually part of the
use new Symfony DI 3.3+ feature to perform lazy-loading without requiring services to be public.

fabpot · 2017-10-02T13:55:57Z

@sroze I gave the same feedback to @nicolas-grekas a few days ago. Upgrading to Symfony 4 is almost impossible. I was luck enough to have write access to almost all bundles that had problems, but I'm in a very special position in this regard.

Moreover, the error message is very difficult to understand and I think people won't understand how to fix the issue.

@nicolas-grekas @sroze You're missing the point. First, it's not true that bundles are making services private. We never said that bundles should done that.

sroze · 2017-10-02T13:59:37Z

@fabpot I guess you wanted to tag @stof on the last sentence.

stof · 2017-10-02T13:59:42Z

Regarding the error message being difficult to understand, this is something we could work on during the stabilisation phase.

sroze · 2017-10-02T15:32:27Z

I agree with the idea that it would be nice to have libraries providing clear extension points (I guess that's what you are trying to solve with private services by default).

Still, if the main reasons are nice-to-have design-thinking and not-proven performances improvements, I guess that's still a debatable choice to force all the existing ecosystem to update. I'd go as far as saying that now that we have timely-based releases, it's not because we can break the BC at each major version that we should 😛

I'd follow on @fabpot's argument: private services were never a communicated best practice. I'd much more prefer a documentation approach (and also "blaming" libraries exposing too much) than enforcing it at the framework level, or we are going to have unhappy users all the way from 3.x to 4.x.

nicolas-grekas · 2017-10-02T15:59:48Z

Don't take me wrong, design is not a nice-to-have: it's something ppl expect their framework to help them do better.
About the performance argument, having services private allows 2 things: inlining, and removals. Both contribute to generate a very clean container, and that's also what I'd expect from a state of the art compiled container (let's say we pretend being one :))
But if nobody cares, let's drop the private/public distinction. The performance impact won't be high, and if ppl do $container->get() all over, that's their mistake. We can do that, I'd have no issue with that if that's what we decide. But we cannot stay in between, we have to move forward in one direction or another, and be core to our values (which ever they are).

it's not because we can break the BC at each major version that we should

I'm telling this all the day while reviewing PRs so I know that for sure. Here making services public in an explicit way is VERY easy, so the required fix is trivial. That's not an excuse, but a balanced argument: if it's easy to do while improving the architecture of all SF apps, that's worth it.
At least that's the motivation behind the current state.

sroze · 2017-10-03T08:34:54Z

Alright, I went a bit to far with "nice-to-have". It's definitely not right to say that "nobody cares" about the design so let's not drop the private/public distinction. There is still three points:

The difference between public and private services is blurry and not well-known by most of the Symfony developers (not only my PoV, highlighted before by @Tobion)
The best-practice of only having a few public services is unknown or at least not applied on almost all the Symfony Bundles out there

The point 1) makes me think that: maybe introducing another property, called private, is not the best we can do to clarify this to the community. Maybe we can find another name?
The point 2) makes me think that we shouldn't be as brutal as "let's make everything private by default" and maybe we should clarify more what this private/public mean?

What about the following:

We rename public/private to exports="true/false" ? That's a known keyword in the JavaScript community and might be easier for us to communicate. i.e. same as RequireJS modules, you can "exports" some of it to be accessible by the users of your library?
By default, we exports everything for Symfony 4 (with an opt-in framework configuration to not exports everything).
Once this mechanism of exports is sufficiently known, we make this by default in Symfony 5

nicolas-grekas added this to the 3.4 milestone Aug 8, 2017

ro0NL mentioned this issue Aug 11, 2017

Make as many services private as possible #23867

Closed

This was referenced Sep 5, 2017

[FrameworkBundle] Fix Di config to allow for more private services #24103

Merged

Make as many services private as possible #24104

Merged

fabpot closed this as completed Sep 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Make as many services private as possible #23822

Make as many services private as possible #23822

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Make as many services private as possible #23822

Make as many services private as possible #23822

Comments

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!