This is not the way you're supposed to define the decorator. In fact, I would expect this to inject itself and cause a recursive dependency. Maybe there's another bug at hand causing this or is this intended behavior?

@iltar, I'm not an expert of the DI component but this seems logical if we consider that the decoration is also "hardcoded", see here and here.

If I understand correctly, if the container handles the debug adm as a decorator it is automatically injected and those lines should not be needed, am I missing something ?

This is also how the decorated choice list factories are defined (without using the decorates parameter).

@HeahDude thanks for working on this.

I can confirm that it fixes the issue. (But I don't know if it is the "correct" way to fix it.)

This fix works fine here. Please merge it. 👍

The fix does not work fine. It simply means that the debug service will never be used, as it does not replace the original service with the decorated version, making it useless.

so this PR does not fix a bug. It breaks the feature entirely.

@stof please see the links of my comment above. This fix does not break the feature as tested with the fork linked in #18554.

@HeahDude but do you still get the votes in the profiler ? This is what the debug decorator is about.

Yes it works perfectly fine :)

Can anyone else (re-)confirm? ping @JHGitty @gharlan

Yes I can confirm it. It fixes the bug, and the new access decision log in profiler is still there.

Thank you @gharlan

@stof This works for the core as the injected access decision manager is changed manually in the extension class: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php#L100-L105

However, this will not work if someone injected the default access decision manager somewhere else as those places would now not use the decorated one anymore.

One solution could be the following delegator class in dev:

@Internal
final class VoterProxy implements VoterInterface
{
    private $voterId;
    private $container;

    public function __construct($voterId, ContainerInterface $container)
    {
        $this->voterId = $voterId;
        $this->container = $container;
    }

    public function vote(TokenInterface $token, $subject, array $attributes)
    {
        return $this->container->get($this->voterId)->vote($token, $subject, $attributes);
    }
}

Imo this is the only "clean" solution will will keep working in the future, possible even in prod (though you might want to cache the container get call then)

Ok I've reverted the first fix in a6fce2d and pushed 8e4f98b to fix it while keeping the debug adm injected properly, considering the good point of @xabbuh.

I've tested it was passed to the SecurityDataCollector when removing debug from its argument name in SecurityBundle/Resources/config/collectors.xml using the fork provided in #18554.

If some one could test it again, we should be good now :)

Actually, the test with the collector is extra safety, since the feature works without overriding security.authorization_checker argument in the config thanks to the last fix.

There was a major update on this PR wich actually fixes a BC break on 3.1. Anyone for a review?

ping @gharlan @JHGitty @iltar Could you please (re-)confirm it works for you?

Thanks.

Isn't the "critical" label needed when a BC break prevents the application from running?

ping @gharlan @JHGitty @iltar Could you please (re-)confirm it works for you?

It works!

Thank you @gharlan, again :)

Thank you @HeahDude.