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 #23822

Closed
nicolas-grekas opened this Issue Aug 7, 2017 · 23 comments

Comments

Projects
None yet
9 participants
@nicolas-grekas
Member

nicolas-grekas commented Aug 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Aug 8, 2017

Member

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.

Member

javiereguiluz commented Aug 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Simperfit

Simperfit Aug 8, 2017

Contributor

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

Contributor

Simperfit commented Aug 8, 2017

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

@javiereguiluz

This comment has been minimized.

Show comment
Hide comment
@javiereguiluz

javiereguiluz Aug 8, 2017

Member

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
Member

javiereguiluz commented Aug 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 8, 2017

Member

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)

Member

stof commented Aug 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 8, 2017

Member

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

Member

stof commented Aug 8, 2017

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 8, 2017

Member

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

Member

nicolas-grekas commented Aug 8, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Aug 8, 2017

Member

@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)
Member

stof commented Aug 8, 2017

@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 nicolas-grekas added this to the 3.4 milestone Aug 8, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Aug 8, 2017

Member

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

Member

nicolas-grekas commented Aug 8, 2017

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

@sstok

This comment has been minimized.

Show comment
Hide comment
@sstok

sstok Aug 8, 2017

Contributor

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

Contributor

sstok commented Aug 8, 2017

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

@chalasr

This comment has been minimized.

Show comment
Hide comment
@chalasr

chalasr Aug 14, 2017

Member

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

Member

chalasr commented Aug 14, 2017

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

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 5, 2017

Member

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)

Member

nicolas-grekas commented Sep 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Sep 5, 2017

Member

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.

Member

nicolas-grekas commented Sep 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Sep 5, 2017

Member

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

Member

stof commented Sep 5, 2017

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

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Sep 5, 2017

Member

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

Member

Tobion commented Sep 5, 2017

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

fabpot added a commit that referenced this issue Sep 5, 2017

minor #24103 [FrameworkBundle] Fix Di config to allow for more privat…
…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

nicolas-grekas added a commit that referenced this issue 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

@fabpot fabpot closed this Sep 14, 2017

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 2, 2017

Member

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? :)

Member

sroze commented Oct 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 2, 2017

Member

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.

Member

nicolas-grekas commented Oct 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 2, 2017

Member

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

stof commented Oct 2, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Oct 2, 2017

Member

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

Member

fabpot commented Oct 2, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 2, 2017

Member

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

Member

sroze commented Oct 2, 2017

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

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 2, 2017

Member

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

Member

stof commented Oct 2, 2017

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

@sroze

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 2, 2017

Member

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.

Member

sroze commented Oct 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Oct 2, 2017

Member

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.

Member

nicolas-grekas commented Oct 2, 2017

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

This comment has been minimized.

Show comment
Hide comment
@sroze

sroze Oct 3, 2017

Member

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:

  1. 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)
  2. 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
Member

sroze commented Oct 3, 2017

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:

  1. 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)
  2. 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment