Skip to content

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

Closed
Seldaek opened this issue Oct 28, 2022 · 9 comments
Closed

Double-autowiring can lead to bad outcomes #48019

Seldaek opened this issue Oct 28, 2022 · 9 comments

Comments

@Seldaek
Copy link
Member

Seldaek commented Oct 28, 2022

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:

use Monolog\Attribute\AsMonologProcessor;
use Monolog\Level;
use Monolog\LogRecord;

#[AsMonologProcessor(channel: 'my_channel')]
class ErrorSuppressingProcessor implements \Monolog\Processor\ProcessorInterface
{
    public function __invoke(LogRecord $record): LogRecord
    {
        return $record->with(level: Level::Debug);
    }
}

Now as you can see I specified this should only apply to the my_channel channel in the AsMonologProcessor 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 plain monolog.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

@derrabus
Copy link
Member

By "autowire", you mean "autoconfigure" everywhere in your issue description, right? 🤓

@Seldaek
Copy link
Member Author

Seldaek commented Oct 28, 2022

Yes I guess I do

@stof
Copy link
Member

stof commented Oct 28, 2022

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.
Core compiler passes are written in a way taking care of that for priorities for instance (but for that, they leverage a shared helper so it is implemented only once), but it is a bit more tricky to support when having custom attributes.

@Seldaek
Copy link
Member Author

Seldaek commented Oct 28, 2022

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.

@nicolas-grekas
Copy link
Member

See #48027 for tentative fix.

@fabpot fabpot closed this as completed Nov 1, 2022
fabpot added a commit that referenced this issue Nov 1, 2022
…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
@leofeyer
Copy link
Contributor

The change in #48027 causes the following error for us:

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException:
Service "Contao\CoreBundle\Controller\BackendController" misses a "container.service_subscriber"
tag with "key"/"id" attributes corresponding to entry "router" as returned by
"Contao\CoreBundle\Controller\BackendController::getSubscribedServices()".

Does this mean we now have to add tags for every service that is returned by getSubscribedServices()?

@stof
Copy link
Member

stof commented Nov 29, 2022

Please open an issue describing your problem instead of commenting on a closed ticket

@nicolas-grekas
Copy link
Member

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

@Seldaek
Copy link
Member Author

Seldaek commented Dec 5, 2022

Is this something that could be deprecated for future removal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants