-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Double-autowiring can lead to bad outcomes #48019
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
Comments
By "autowire", you mean "autoconfigure" everywhere in your issue description, right? 🤓 |
Yes I guess I do |
IIRC, this is something to be solved in the compiler pass, to better handle case where you have a specific tag alongside a tag with no attribute. Otherwise, you have the same issue when the interface is autoconfigured and the dev adds an explicit tag. |
It can definitely be "fixed"/worked around in the compiler pass. I think it'd be worth having a good think whether this can be fixed more centrally though because that's just a nasty foot-gun lying around in the framework. Is there a valid case for having multiple of the same tag on a service (at least when some of the tags have no attributes)? If not we could automatically clean all those duplicates before calling the other compiler passes. |
See #48027 for tentative fix. |
…ready set with attributes (nicolas-grekas) This PR was merged into the 5.4 branch. Discussion ---------- [DependencyInjection] Don't autoconfigure tag when it's already set with attributes | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #48019 | License | MIT | Doc PR | - Commits ------- f27ed9b [DependencyInjection] Don't autoconfigure tag when it's already set with attributes
The change in #48027 causes the following error for us:
Does this mean we now have to add tags for every service that is returned by |
Please open an issue describing your problem instead of commenting on a closed ticket |
#48027 has been reverted because it caused a regression. People rely on the behavior that is described here so we cannot change it. I'd suggest fixing the compiler pass instead. Keeping as closed since the pass is not in this repo. |
Is this something that could be deprecated for future removal? |
Symfony version(s) affected
6.1.x (probably 5.4 and 4.4 perhaps as well)
Description
If a class gets autowired twice the effects can be unpredictable and consequences can be pretty bad.
How to reproduce
The case which triggered this find was that I made a monolog processor using the new AsMonologProcessor (symfony/monolog-bundle#428) attribute, which looks like this:
Now as you can see I specified this should only apply to the
my_channel
channel in theAsMonologProcessor
attribute. Given what the processor does, having it applied to all channels would be pretty bad as it turns all records into debug records.This in fact works well, the MonologExtension registers it correctly in https://github.com/symfony/monolog-bundle/blob/a41bbcdc1105603b6d73a7d9a43a3788f8e0fb7d/DependencyInjection/MonologExtension.php#L155-L166
The problem is that because I also implemented
ProcessorInterface
, and the MonologExtension defines this interface as autowired (https://github.com/symfony/monolog-bundle/blob/a41bbcdc1105603b6d73a7d9a43a3788f8e0fb7d/DependencyInjection/MonologExtension.php#L134-L135), the DIC also adds a plainmonolog.processor
tag to my processor class.Then in AddProcessorPass you end up with the ErrorSuppressingProcessor service with two tags, one containing the correct channel mapping and the other one being a blank
monolog.processor
tag without options, which means the processor suddenly gets registered to all Loggers/channels. Not at all what I wanted not expected :)Possible Solution
I am not entirely sure what can be done here, but I believe a solution should be in symfony/di and not in the bundle itself as that was just a symptom of the problem.
My instinct would be to say that whatever code handles
registerForAutoconfiguration
should probably not add a tag if the tag is already present. Not sure what the consequences of doing that would be though.Additional Context
No response
The text was updated successfully, but these errors were encountered: