8000 Adding a parameter that allows you to globally set an autowiring map by weaverryan · Pull Request #18040 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Adding a parameter that allows you to globally set an autowiring map #18040

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

Conversation

weaverryan
Copy link
Member
Q A
Branch master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17783
License MIT
Doc PR todo

Hi guys!

This allows end-users to override an autowiring type - e.g.

parameters:
    autowiring.map:
        Psr\Log\LoggerInterface: 'my_logger'

Using parameters for this was by far the easiest implementation, but not the most user friendly - it's not my favorite to have random magic keys like this do things. There is no precedent (that I'm aware of) for using parameters in this special way. However, why not? If we wanted to deprecate or change it in the future, we can do that easily where we use the parameter.

Cheers!

@linaori
Copy link
Contributor
linaori commented Mar 7, 2016

In my opinion it's wrong to have bundles depend or use parameters that are not defined from within the bundle, this is exactly where the configuration comes into play. This is not a parameter but a configuration option for the DIC. The benefit of configuration is that you can enforce the format, whereas this allows users to get all kind of weird errors if the format isn't correct.

I would prefer to see something more like:

framework:
    autowiring-mapping:
        Psr\Log\LoggerInterface: my_logger

@xabbuh
Copy link
Member
xabbuh commented Mar 7, 2016

I agree with @iltar. Internally, such a config option could still just populate this parameter. But users should not have to rely on this implementation detail.

@dunglas
Copy link
Member
dunglas commented Mar 7, 2016

The downside with the config option is that it will not be available for people using the standalone component.

But, is this feature really necessary? Developers needing such tricks can already override the definition of the service and add the autowiring_types key.
IMO it's better to encourage developers to open PRs upstream to add autowiring_types than introducing such settings.

Btw, a new release of MonologBundle has been done and this fix isn't necessary anymore this specific case.

@weaverryan
Copy link
Member Author

@dunglas Is there any "easy enough" way to override a service to do this - using logger as an example? I do it so rarely, I can't think of a pure config way (avoiding a compiler pass).

If there is an easy way, then maybe we can do that. Otherwise, I can follow the advice of the other guys and move it to config (I don't know why I didn't do that in the first place).

@weaverryan
Copy link
Member Author

After talking with @wouterj, there is a way (almost) to do this without needing new config. To continue with the LoggerInterface example (even though this problem is now fixed):

services:
    logger.for.autowiring:
        alias: logger
        autowiring_types:
            - 'Psr\Log\LoggerInterface'

What do you think? It actually doesn't work now because AutowirePass doesn't look at alias services for autowiring_types (which could be considered a bug), but that can easily be fixed.

@dunglas
Copy link
Member
dunglas commented Mar 12, 2016

I much prefer this way.

@linaori
Copy link
Contributor
linaori commented Mar 12, 2016

that looks good, but what would happen with the following config?

# foo.yml
services:
    logger.for.autowiring:
        alias: logger
        autowiring_types:
            - 'Psr\Log\LoggerInterface'

# bar.yml
services:
    another.logger.for.autowiring:
        alias: logger
        autowiring_types:
            - 'Some\Implemenation\Of\LoggerInterface'

@dunglas
Copy link
Member
dunglas commented Mar 12, 2016

@iltar with the current implementation, if you typehint Some\Implemenation\Of\LoggerInterface another.logger.for.autowiring will be injected, and if you type hint the interface logger.for.autowiring will be.

@maryo
Copy link
Contributor
maryo commented Mar 12, 2016

The thing with the alias is exactly the same as what I originally proposed here #17783

@weaverryan
Copy link
Member Author

@maryo I see that now - it's a clever solution, so I think it didn't "sink in" when you first mentioned it - sorry about that :)

@weaverryan
Copy link
Member Author

Actually, this is a non-trivial solution to implement. The problem is that an "alias" service isn't represented as a full Definition object - it's just an Alias object: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Loader/YamlFileLoader.php#L179.

To support autowiring_types no an alias service, we'd need to update every loader and add a new property to Alias so we know what types it should map to.

I was hoping the alias idea would be a nice "shortcut" so that we wouldn't need to add much/any code. But it's not. I recommend we use @iltar's original proposal: #18040 (comment)

@weaverryan
Copy link
Member Author

Nevermind, I figured it out. It requires no coding, and is pretty horrible-looking. But, it works (right now, today) and would require not coding. For example, if you assume still that LoggerInterface could not be autowired, you could:

services:
    logger_for_autowiring:
        class: Psr\Logger\LoggerInterface
        public: false
        autowiring_types:
            - Psr\Log\LoggerInterface
        tags:
            - { name: auto_alias, format: 'logger' }

Yep, that does it - taking advantage of a little-known DI tag.

Should we just document this?

@linaori
Copy link
Contributor
linaori commented Mar 22, 2016

The funny thing is, I actually arrived on that page during the hack-day and I thought it wasn't what you were looking for :(

I think it would be an excellent idea to document the auto_alias option and refer to it in the auto-wire documentation. Imo would be best documented as "what to do when you want to autowire but symfony has no support for this case yet".

@maryo
Copy link
Contributor
maryo commented Mar 23, 2016

@weaverryan:
"we'd need to update every loader and add a new property to Alias so we know what types it should map to."
Yeah. I realized it too. I was not sure if it worth it or if there are better alternatives or a different proposals how such a feature should work or if it should be closed instead for some reason.
If only the amount of work was a problem then I could do it. I personally like it more than the parameter or config solution or even the auto_alias but thanks alot mentioning it is possible this way. I actually didn't know about this auto_alias feature.

What do others think? Maybe this workarround is acceptable because as Dunglas wrote "it's better to encourage developers to open PRs upstream to add autowiring_types than introducing such settings".
But maybe one could still need to set it to something else even it is already set (but maybe that could be considered as a bad practice and named services in such cases should be preferred).

@weaverryan
Copy link
Member Author

I think for now, we should stick with this solution. If we find that we're using this a lot (and not just because there is some "bug" upstream that needs to be fixed), we can re-visit adding a clearer solution. But I feel pretty good that there's a workaround, and it's only a few lines of code (though a bit ugly)

@TomasVotruba
Copy link
Contributor
TomasVotruba commented Jul 17, 2016

I would like this feature as well. It's very counter intuitive and hard to understand. 5 lines is way too muc 8000 h for simple class/interface_type => service_name association.

@TomasVotruba
Copy link
Contributor

I've added this feature to my bundle, so it's easy to use for everyone without deeper DI+yaml configuration knowledge:
https://github.com/Symplify/DefaultAutowire#multiple-service-of-one-type-set-preferred-one

fabpot added a commit that referenced this pull request Feb 1, 2017
…icolas-grekas)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] Deprecate autowiring-types in favor of aliases

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #21351, #19970, ~~#18040~~, ~~#17783~~
| License       | MIT
| Doc PR        | symfony/symfony-docs#7445

https://github.com/symfony/symfony/pull/21494/files?w=1
This PR deprecates autowiring-types and replaces them by plain aliases.
ping @dunglas @weaverryan

Eg instead of
```xml
<service id="annotations.reader" class="Doctrine\Common\Annotations\AnnotationReader" public="false">
    <autowiring-type>Doctrine\Common\Annotations\Reader</autowiring-type>
</service>
```

just do:
```xml
<service id="annotations.reader" class="Doctrine\Common\Annotations\AnnotationReader" public="false" />
<service id="Doctrine\Common\Annotations\Reader" alias="annotations.reader" public="false" />
```

Commits
-------

b11d391 [DI] Deprecate autowiring-types in favor of aliases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
0