There is a drawback with this implementation: any such voter creates a circular object graph, which hurts garbage collection.
This will mainly affect tests where the kernel is shutdown in the process and then booted again, as the kernel is only booted once during a normal request handling.

👍

Does it make sense to add a DI tag instead of this configuration-through-implements?

@nicolas-grekas Do you mean give your voters an extra tag, and then a compiler pass will modify those service Definitions to have a setAccessDecisionManager "call" on them? Conceptually, I'm not against it - but I think we'll have a "circular reference", as I believe the AccessDecisionManager requires all the voters as a constructor arg, so the voters can't depend back on the AccessDecisionManager. Or were you thinking of something different?

That's true. An other idea trying to remove the circular dep: what about always injecting the AccessDecisionManager in a voting attribute? That would allow removing the new interface, and rely on the existing supportsAttribute instead? I don't know well the voter code so please forgive me if that's an obvious bad idea... :)

@nicolas-grekas That's really clever :). And actually, I think we can make it work technically. But, the "attributes" are what the user passes to isGranted() originally (i.e. the "permission" you're checking for). This would kind of change the meaning/purpose of the attributes, so I don't think it's a good idea unfortunately :/.

I see a few options:

a) Be ok with the circular reference. Afterall, you're opting into it
b) Pass the AccessDecisionManager into vote() as the 4th arg. Actually, we'd need to deprecate the old interface, create a new interface method (e.g. voteOnAccess) to do this, but very possible and there's a clear path to make this happen
c) Add a __destruct() method in AccessDecisionManager that unsets $this->voters to avoid the reference. I'm not 100% sure __destruct() will be called when you shutdown the kernel, but I could check. And there's no real precedence for this in core.

Would any of these 3 options be acceptable?

I see two valid options:

1. one is allowing the access decision manager to be injected in the voters: the patch is easy (see [Security] Add setVoters() on AccessDecisionManager #14733). It has the drawback of creating a circular object graph, and has the benefit of not requiring any deprecation.
2. your b) option: passing it as 4th argument to the vote() method. This prevents the circular object graph but requires a change in the interface, and makes every voter aware of the access decision manager, which is usually not required.

Since these are all services, my preference goes for 1.
If one wants to cut the circular graph, calling ->setVoters with an empty array would break it.

See #14733
@weaverryan I'd happily let your do the doc PR...

@nicolas-grekas Fast and great reply. I'm closing this - your approach is superior and simpler.

Thanks!