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 asetAccessDecisionManager "call" on them? Conceptually, I'm not against it - but I think we'll have a "circular reference", as I believe theAccessDecisionManager requires all the voters as a constructor arg, so the voters can't depend back on theAccessDecisionManager. 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 existingsupportsAttribute 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 toisGranted() 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 intovote() 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 inAccessDecisionManager 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 andgreat reply. I'm closing this - your approach is superior and simpler.

Thanks!