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

[3.0] [DependencyInjection] Remove the *.class parameters from core #11881

Closed
fabpot opened this issue Sep 8, 2014 · 27 comments
Closed

[3.0] [DependencyInjection] Remove the *.class parameters from core #11881

fabpot opened this issue Sep 8, 2014 · 27 comments
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@fabpot
Copy link
Member

fabpot commented Sep 8, 2014

All services in core define their classes as parameters. The convention is to create a foo.class parameter for the class of the foo service. This approach comes with several problems:

  • The parameters are dumped like any other one in the compiled class and it makes the file larger for no good reasons (read: it slows down your app for free).
  • When someone wants to change a service, just changing the class name is rarely enough: the new service probably have some different constructor arguments, ... Or put another way, overriding a service by just changing the class name of a service is a very rare use case.
  • Of course, if several bundles change the value of such a parameter, the "last" definition wins. Classic inheritance vs composition problem.

For all these reasons, I like to remove all those parameters for Symfony 3.0.

On a side note, it's now possible to easily replace a service by decorating it (see #9003).

@fabpot fabpot added DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Sep 8, 2014
@fabpot fabpot added this to the 3.0 milestone Sep 8, 2014
@pgodel
Copy link
Contributor

pgodel commented Sep 8, 2014

I like this. When dealing with large projects with many classes/services it becomes tedious and never really used this "feature".

@javiereguiluz
Copy link
Member

I love this proposal. Any given Symfony app has more than 300 *.class parameters!

@fabpot
Copy link
Member Author

fabpot commented Sep 8, 2014

By the way, I'm not even sure that this is a documented "feature" ;)

@stof
Copy link
Member

stof commented Sep 8, 2014

👍 as well.

Btw, I suggest we update the documentation ASAP to stop showing this kind of definition in the samples (IIRC, we have a few places showing it), or at least adding a note explaining it is discouraged

@stof
Copy link
Member

stof commented Sep 8, 2014

By the way, I'm not even sure that this is a documented "feature" ;)

IIRC, we even explicitly document that for any bundle exposing a semantic configuration, messing with the parameters defined by the bundle is an unsupported extension point

@sstok
Copy link
Contributor

sstok commented Sep 8, 2014

👍

@pborreli
Copy link
Contributor

pborreli commented Sep 8, 2014

👍 less magic, more speed for Symfony 3 🚤

@wouterj
Copy link
Member

wouterj commented Sep 8, 2014

👍 But this is currently documented in the conventions for service and parameter names, let's change that :)

@webmozart
Copy link
Contributor

👍

@derrabus
Copy link
Member

👍

Although it has never been best practice to override those *.class parameters for framework services, I've seen people doing that. I like the idea of removing those parameters, but there should be a good documentation pointing out a migration path away from this practice.

@lyrixx
Copy link
Member

lyrixx commented Sep 11, 2014

👍 But I have another approach. Keep everything like it's done now, but we could add a compiler pass to remove all *.class parameters from the compiled DIC.

When someone wants to change a service, just changing the class name is rarely enough

I disagree with that, I override some some part of symfony / monolog to tweak 2/3 things. I did that on private projects...

@kingcrunch
Copy link
Contributor

👍 For removal

@Koc
Copy link
Contributor

Koc commented Sep 11, 2014

@lyrixx I'm also using this for some classes. But you can using services decoration for this.

@Tobion
Copy link
Member

Tobion commented Sep 11, 2014

👍 I never found these params to be a good design.

@linaori
Copy link
Contributor

linaori commented Sep 13, 2014

👍 I've always wondered why this was done and people recommended doing this for open-source packages without being able to specify why.

This will however, break a lot of things. It might be an idea to explain service decoration more thoroughly in the documentation. There's 1 page I could find about it in the docs which doesn't even explain how to build your service (php class) if you want to do some decorating: http://symfony.com/doc/current/components/dependency_injection/advanced.html

@GrahamCampbell
Copy link
Contributor

👍

@mmoreram
Copy link
Contributor

👎 Agree with @lyrixx. If the problem is that the Container definition becomes too big, okay, they could be removed using a simple compiler pass (Like unused private services).

There is a point you should take in account when planning Symfony3. Big BC Breaks are very tedious to be migrated, and big migrations are a bad idea for any technology. If you remove all .class, a lot of projects overriding Symfony services using this strategy will be affected, and what you will will get is more and more legacy code.

@linaori
Copy link
Contributor

linaori commented Sep 14, 2014

If decided to not implement this change, a compiler pass could optimize the container by removing unused parameters ending with .class for the current state of symfony 2.*. For all new "feature" and "major" patches, you could already enforce this rule by fabbot, leaving the old untouched and to be removed in a later major than 3.

Enforcing better development options can make the code a lot better on the long term. A service decorator is a far more elegant solution than replacing the arguments.

@wouterj
Copy link
Member

wouterj commented Sep 14, 2014

We can also do this in the 2.x serie and include a compiler pass which changes the class of a service definition when a parameter with %service_id%.class exists. This way, we can notify people to update their code while keeping everything working.

@fabpot
Copy link
Member Author

fabpot commented Sep 14, 2014

@wouterj No, you cannot. What about someone using the parameter outside the DIC (something like $container->getParameter('foo.class'))?

@linaori
Copy link
Contributor

linaori commented Sep 14, 2014

You could make it configurable, off by default. If someone is using those parameters run-time, they are doing funky stuff either way. It doesn't justify a BC break, but it should not be considered a common use-case in my opinion.

framework:
    container:
        optimize_class_name_parameters: ~

@stof
Copy link
Member

stof commented Sep 15, 2014

@fabpot the advantage of being able to mark parameters as private (same meaning than with private services) would be that we could remove much more parameters from the runtime container than only the .class ones. Lots of parameters are used only in service definitions.
Then, we could introduce the feature in 2.x, start using it for new parameters we add, and apply it on existing parameters only in 3.x to maintain BC on $container->getParameter('router.class') (while documenting that these parameters should already be considered private and should not be accessed from the container, but the code refactored)

@acrobat
Copy link
Contributor

acrobat commented Sep 16, 2014

👍

@lyrixx
Copy link
Member

lyrixx commented Sep 19, 2014

I totally agree with @stof but I propose a "better" solution:

I prefer to introduce private parameter in symfony 2.6. All *.class parameter will be private. But to be BC, theses parameters will stay in the DIC. But, a new option in the framework bundle, like remove_private_parameters, could enable the removal of all private parameters. Like that, we can already remove all useless parameter in SF 2.6, for "optin" / aware users.

Then, IIRC (and @stof noticed that too), symfony itself uses parameters to configure some services from a compiler pass, and they not always remove theses param. I (we?) do the same thing in ours projects.

I can work on this feature, it seems quite easy.

@stof
Copy link
Member

stof commented Sep 19, 2014

Introducing private parameters without removing them from the DIC makes them useless. It would mean that private parameters are the same than public ones

boekkooi added a commit to boekkooi/TwigJackBundle that referenced this issue Dec 4, 2015
boekkooi added a commit to boekkooi/TwigJackBundle that referenced this issue Dec 4, 2015
fabpot added a commit to symfony/monolog-bundle that referenced this issue Oct 19, 2016
This PR was squashed before being merged into the 3.x-dev branch (closes #170).

Discussion
----------

remove class parameters

The idea was initially discussed in symfony/symfony/issues/11881.

If I understand things correctly, we should provide a service for each removed parameter to allow bundle users to decorate or override corresponding services.
But I'm not sure if services should be listed in DI config of bundle, or it is OK to add them during DI Extension load.

I'm using those services to map handler type to handler class (this is done [here](https://github.com/avant1/monolog-bundle/blob/remove-class-parameters/DependencyInjection/MonologExtension.php#L133-L134)). But those handler services can be replaced by hash mapping.

If anything else related to this changes should be done (like updating README or adding note to symfony logging cookbook) - I'm ready to work on this too.

Commits
-------

6398efc remove class parameters
e-moe added a commit to e-moe/guzzle6-bundle that referenced this issue Mar 20, 2017
lunetics added a commit to prooph/service-bus-symfony-bundle that referenced this issue Apr 21, 2017
Bad practice and error prone:
also see symfony/symfony#11881
and symfony/symfony#9003
ndench added a commit to ndench/symfony-docs that referenced this issue May 11, 2017
dmaicher added a commit to dmaicher/menu-bundle that referenced this issue Jun 12, 2017
codeliner pushed a commit to prooph/service-bus-symfony-bundle that referenced this issue Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests