Should we explore dedicated PHP8 class attributes?

IIUC eg.#[VotesOn(attributes, class)] enables compling a warm cache?

#[VotesOn(attributes, class)] enables compling a warm cache

I don't know if we need an attribute for that, but what about a method that exposes what a voter cares about@jderusse, instead of the current approach?

@jderusse what about voters that vote only onnull subjects (for instance the RoleVoter) ?

@nicolas-grekas i generally like attributes enforcing static/cachable rules to us :) but runtime api also fits yes. As an end user taking care of such issues, an attribute looks elegant IMHO.

An attribute might make the architecture of the AccessDecisionManager harder, if we want to avoid calling Reflection on eachdecide() call. A$voter instanceof CacheableAbstainVotesInterface check is much more efficient than reading attributes at runtime. Having to implement a caching layer for the metadata of the caching layer seems weird to me.

Understood. I meant levaraging DI at compile time.

Writing another set of support method implementations already bugs/confuses me somewhat. But it'll be the edge case... WFM.

what about a method that exposes what a voter cares about instead of the current approach?

Yeah, I hesitated with this approach: but it can not work for voters that use a pattern for attributes: ie RoleVoter =>ROLES_whatever (Which one of the much-used Voters)

@jderusse what about voters that vote only onnull subjects (for instance the RoleVoter) ?

This will work: the current approach is to let voters reporting "I will alwaysabstain for this attribute" and/or "I will alwaysabstain for this object", everything else will always be called.

If voters don't care about subjects (like RoleVoter) they can returnfalse tellingI will **not** always abstain... which meanscall me, in all case.

The code will still be optimized because the voter will not be called when the attribute does not start withROLE_

note: to be accurate RoleVoter does not vote "only on null subject", they vote on "whatever the content of the subject".

Cmments addressed, and tests added

Thank you@jderusse.

@jderusse@nicolas-grekas

With the renaming of the interface and methods, the semantics seem reversed when you first look at it.

Since the interface is now calledCacheableVoterInterface, the methodssupport*() returningfalse to enable feel inverted. My expectation would be that my voter becomes cacheable if it returnstrue (opts in), just like the serializer equivalent does.

support* is not about whether the voter is cacheable, but whether it supports that permission (and so must be called for it)

@stof in that case the interface is misnamed, IMO, the original names seem more appropriate.

The original names were confusing to me. Feels like double negation:CacheableAbstainVotesInterface::willAlwaysAbstainOnAttribute raises a "wtf I am supposed to implement inside that?!?". Supports* method are common and straightforward.

I'm not against renamingCacheableVoterInterface if you have a better proposal. Do you?

if you have a better proposal. Do you?

It depends who gets to define "better". As I said, the weird part here is returningfalse to opt-in to being cacheable.

thing is, the feature is about improving performance when somethingIS NOT able to process the payload.

Having a negation will always be confusing.

One could useCacheableVoterInterface::doesNotSupportAttribute orUnCacheableVoterInterface::supportAttribute that will always be a cognitive load.

In my opinionsupport* is the way to go: It's not a negation, and consistent with what we do in many other place (including theAbstractVoter)

If we need to change something it should be the name of the interface. Would be happy to make a PR if somebody have a suggestion.

I'm on a phone so I can't explain on detail, but to me the current implementation is very intuitive because I understand it as follows:

Symfony: Hey voter, do you support ARTICLE_EDIT attribute?
Voter: no (returns FALSE to say that)
Symfony: OK. I will never call you again for this attribute then.

This means the voter cacheability is a side-effect of this detection, it's inferred and not directly stated. Your votes is not becoming "cacheable", it's becoming "more capable of explaining what it knows how to work with".. which is the information then used to make it cacheable.

It basically has "extended / detailed supports" support.

I must say that I quite liked thewillAlwaysAbstainOnAttribute() andwillAlwaysAbstainOnObjectType(): it's crystal clear what information is needed from the voter in order to judge whether we should cache it.
I.e. I must only returntrue if I always 100% of the cases abstain on this attribute. We loose this a bit with the current generic name ofsupportsAttribute() imho.

That said, this little nuance can be fulfilled easily by documentation.