Skip to content

[RFC] Proposal to simplify the autowiring of container parameters #23718

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

Closed
javiereguiluz opened this issue Jul 30, 2017 · 18 comments
Closed

[RFC] Proposal to simplify the autowiring of container parameters #23718

javiereguiluz opened this issue Jul 30, 2017 · 18 comments
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@javiereguiluz
Copy link
Member

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

The Problem

These days I'm updating some legacy Symfony applications to use Flex and modern Symfony practices. The first step was autowiring services. I loved the change. Everything is much better now and the automation (not magic) added by autowiring is fantastic to improve your productivity and code quality.

However, there's a minor thing about autowiring that bugged me: autowiring scalars.

public function __construct(SomeClass $service1, $argument1, $argument2, $argument3)
{
    $this->service1 = $service1;
    $this->argument1 = $argument1;
    $this->argument2 = $argument2;
    $this->argument3 = $argument3;
}

I understand that this cannot be solved without adding magic. But in my case, 100% of scalar values are container parameters. And I have literally hundreds of lines of service config like this:

Acme\Blah\Blah\MyClass:
    arguments:
        $argument1: '%kernel.project_dir%'
        $argument2: '%app.container_param%'
        $argument3: '%app.another_param%'

It's so boring to do this. And so useless! I'd like to propose another way to solve this.


The Proposal

What if we allow to inject ParameterBag to inject all container parameters?

The above example would now look like this:

public function __construct(SomeClass $service1, ParameterBag $params)
{
    $this->service1 = $service1;
    $this->argument1 = $params->get('kernel.project_dir');
    $this->argument2 = $params->get('app.container_param');
    $this->argument3 = $params->get('app.another_param');
}

And no YAML/XML config would be needed for services like this.


Comments

  • What about performance? The number of container params has been reduced drastically in modern Symofny versions (by removing the *.class params) and params is an array which is easily cacheable by PHP 7. Impact in memory consumption should be minimal or null? To be confirmed...
  • What if you have thousands of container params? Don't use this and keep using the existing solution to inject params individually.
@javiereguiluz javiereguiluz added DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed) labels Jul 30, 2017
@GuilhemN
Copy link
Contributor

GuilhemN commented Jul 30, 2017

Another solution is merging #22187 which allows to inject named arguments in all services of a file :)

Edit: with your example, it could look like:

services:
    bind:
        $argument1: '%kernel.project_dir%'
        $argument2: '%app.container_param%'
        $argument3: '%app.another_param%'
    # ...

    App\:
        # ...

This way you don't end up with hundreds of useless lines but you keep the advantages of being explicit and you don't inject a class that imo belongs to sf internals.

@javiereguiluz
Copy link
Member Author

@GuilhemN yes, your proposal could work too ... although I don't like that it introduces yet another configuration option that users must learn.

@GuilhemN
Copy link
Contributor

There's not much to learn, the syntax is the same as arguments. The difference is that it can be used for arguments and method calls arguments and that its content doesn't need to be used in all services for resources/defaults.

This could replace arguments in most cases in fact so if you're worried there is too much to learn, we could reduce the explanations about arguments and method calls arguments to focus on it.

@javiereguiluz
Copy link
Member Author

I was referring to bind, which would be the new keyword/option to learn.

@theofidry
Copy link
Contributor

@javiereguiluz why can't you already inject the parameter bag?

@GuilhemN
Copy link
Contributor

For sure but that's not much to learn, there are options that are far less useful imo that we still introduced and documented (configurators, shared).

@javiereguiluz
Copy link
Member Author

@theofidry have you tested it? Because I can't make that work at all 😞

@GuilhemN
Copy link
Contributor

GuilhemN commented Jul 30, 2017

I guess you first have to register the ParameterBag as a service:

services:
    Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface:
        factory: ["@service_container", getParameterBag]

@javiereguiluz
Copy link
Member Author

@GuilhemN

Uncaught Symfony\Component\DependencyInjection\Exception\RuntimeException:
Unable to resolve service "Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface":
method "Symfony\Component\DependencyInjection\ContainerInterface::getParameterBag()" does not exist.

@GuilhemN
Copy link
Contributor

GuilhemN commented Jul 30, 2017

What if you explicitly don't autowire this service?

@franzose
Copy link

@javiereguiluz, by injecting ParameterBag or the whole container you make dependencies of your class unclear. If you feel that you repeat the same tasks of declaring some classes' arguments again and again, shouldn't you consider refactoring?

@NicoHaase
Copy link
Contributor

What about introducing a ConfigurationClass? It takes all scalars you need, delivers them through getters, and should be autowirable

@theofidry
Copy link
Contributor

theofidry commented Jul 31, 2017

@javiereguiluz to be honest that's one thing I don't like about auto-wiring: bending designs to make it work better. IMO auto-wiring is a tool to help, not something you base your application on. So in your case I would say that yes: you inject scalar values, you need to manually wire them. And Symfony is not alone: even in frameworks with full auto-wiring like Laravel you need to do that.

Now if you really want to go that way:

There may be other solutions like refactoring some pieces, using abstract service definitions, but hard to tell without your code.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 31, 2017

#22187 is exactly the solution we created to solve this issue, I think we just need everyone to focus on it so that it gets some support and is merged.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jul 31, 2017

#22187 is just the explicit way to code your own conventions in configuration. Contrary to a proposal like this one, or the annotation based ones, that force you to design/write your code in some way for the need of auto-configuration, which is doing things reverse side to me.

@javiereguiluz
Copy link
Member Author

Let's close this in favor of #22187. Thanks!

nicolas-grekas added a commit that referenced this issue Aug 9, 2017
This PR was squashed before being merged into the 3.4 branch (closes #22187).

Discussion
----------

[DependencyInjection] Support local binding

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes
| Fixed tickets | #22167, #23718
| License       | MIT
| Doc PR        |

> A great idea came out on Slack about local bindings.
> We could allow injecting services based on type hints on a per service/file basis:
> ```yml
> services:
>     _defaults:
>         bind:
>             BarInterface: '@usual_bar'
>
>     Foo:
>         bind:
>             BarInterface: '@alternative_bar'
>             $quz: 'quzvalue'
> ```
>
> This way, `@usual_bar` will be injected in any parameter type hinted as `BarInterface` (in a constructor or a method signature), but only for this service/file.
> Note that bindings could be unused, giving a better solution than #22152 to #21711.
>
> As named parameters are usable in arguments, bindings could be usable in arguments too:
> ```yml
> services:
>     Foo:
>         arguments:
>             BarInterface: '@bar'
> ```

~Named parameters aren't supported yet.~

Edit:

> Note that bindings could be unused

Current behavior is throwing an exception when a binding is not used at all, in no services of a file if it was inherited from `_defaults` or in no services created from a prototype.
It will pass if the bindings are all used in at least one service.

Commits
-------

81f2652 [DependencyInjection] Support local binding
@MGDSoft
Copy link
Contributor

MGDSoft commented Aug 12, 2017

I think this approach to improve development speed is important. Because to add an argument in a service I need to move to other file, search service (or create his definition), add argument, when I back to my service I lost my focus for this repetitive tasks and if the project is huge it is worst....

This kind of things is not magic is to be efficient...

I know all symfony programmers must know how services system works but in 90% of time we need to save our time.

This problem is the same to add a "TAG", "priorities" ... in a service. What do you think about the use of Annotations?

@nicolas-grekas
Copy link
Member

See #25288, which could be used instead.

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

7 participants