@eltharin hi 👋🏻

nice to see that you are willing to take over this amount of work, incremented overtime by different coders and reviewers :)

i am still interested in such feature even though now I rarely use voter, still thinking that this can be a great addition in core (like workflow transition blocker message is available)

to be honest, I think I would probable split this in 2 separates PRs, just to ease review and reduce the friction already existing around such feature, as it needs to arrive on a simple/BC/evolutive/maintainable way in core

• add the feature of a voter message (main issue feature that seem to have lot of traction over the year regarding all PRs reactions)
• add the feature of a new decision strategy alone (perhaps even before the voter message)

in any way, I will add a reminder to make a review on this PR as well :)

( PS:@noniagriconomie is one old account of mine no more used since )

@94noni

you and other are the firsts to thank for the start of this work,

For the 2 PR's I thought about it but it's absolutlly linked, both needs Vote Object so getVote function so GetDecision functions,

Remove first or second feature but keep other one will only touch few files compared with all files changed.

At start, i saw your PR's without really needed, but when I wrote myine for scoring, I saw I made the same work as you, so I included your work in these to not have conflict when one will be accepted.

As it's a big update to add all these functions in symfony core I add a new benefit to this to have more chance to see this PR passed :)

Did anyone ever run performance checks on these changes? Considering that Voters can be called hundreds of times per request, any overhead created by hundreds of new Vote() instances or checks for existing methods should imo be avoided (e.g. add new interfaces so I can opt-in to the new approach, instead of forcing the code upon everyone).

Did anyone ever run performance checks on these changes? Considering that Voters can be called hundreds of times per request, any overhead created by hundreds of new Vote() instances or checks for existing methods should imo be avoided (e.g. add new interfaces so I can opt-in to the new approach, instead of forcing the code upon everyone).

I look with Symfony Profiler, with 3000 voters With 7.2 actual version VoteListener take 120MiB in 8ms with this PR, 118MiB for 8.4ms.
I don't made more performance test with a performance tool

friendly ping@chalasr as default reviewer

I think I nailed it: instead of passing by reference, we can pass pre-constructed objects.
Here is a patch that implements what I mean. (see last commit there for my specific contribution)
Note that I didn't update tests and that I removed the message on the AccessDecision object as I see it of low value.

I removed the message on the AccessDecision object as I see it of low value.

BTW, this makes me realize that the AccessDecision object is never used in the end. We pass it around, we collect votes in it, and we trash it. The profiler doesn't need that object. Maybe we can remove this part?

AccessDecision Object is send to Profiler AND Exception to retreive why this reason with votes.
I kept "int" votes and objects votes in this object to see whitch voter respond in int and whitch respond in object.
And I keep in mind my ScoringStragy need (witch I'll put in PR later or in a personal bundle) but with your solution it can be impossible to do that.
That's what I want with a VoteInterface instead directly Vote that if some more crazier than me (yes it can be possible) want more attributes in a vote It can have it own VoteObject with Own strategy.
I think your last commit lost very much of that

I'm sorry I don't understand your comment. What did my approach lose? The only thing I removed from a feature pov is adding a reason to the access decision itself.

sorry I miss something in your code...
Don't see you put Vote Object in adm

for allowMultipleAttributes argument, it's not from my PR, I just put it into comments in interface to rember it but It already been there.

And in the strategy we don't have Vote Object ?
Can't we keep accessdecision in second argument of decide and get Vote Object by end($accessDecision->votes) ?

Don't see you put Vote Object in adm

I don't get what this means sorry.

allowMultipleAttributes argument

this argument is an implementation detail, it shouldn't become an API. I moved it to the AccessDecision object but I suppose it can be kept as an argument, after the added $accessDecision. I'll see how this idea fits on by PR.

And in the strategy we don't have Vote Object ?
Can't we keep accessdecision in second argument of decide and get Vote Object by end($accessDecision->votes) ?

No, because it's not needed to achieve the purpose of this PR.
Why would we do this if we can make everything work without?
Said another way: what would this provide that'd not be possible with my approach?

What I say is the argument allowMultipleAttributes already exist now, it's not me who create it.

No, because it's not needed to achieve the purpose of this PR.

purpose is to open Vote process, by creating a VoteInterface, it allow developer to integrate a custom configuration with its own properties to have a custom strategy using it.

I don't think to make all thoses changes "just" to print some message is very useful...

I'm going to submit my patch as a separate PR, with only the "reason" feature. That's what people are asking for in all linked PRs/issues so many others do think it would be useful.

Then, I'll invite you to submit the scoring strategy as a separate and follow up PR. At the moment, I don't get what you want to achieve on this topic so having it split will help understand where we want to go, and why.

Follow up PR at#59771
I'm closing here as I'm quite convinced my approach works and fully accounts for BC issues.
As mentioned in my previous comment, I'll let you submit the scoring strategy as a separate PR.
Thanks a lot for helping this move forward!

I agree your approach works, I just said with a small patch (post below) we improve DX from "add a message in a vote" to make a full customizable Vote Process. I really think it's a shame to stop work for so little benefic

Now for your PR, I think it can be a BC when Dev create a custom AccesManager copying decide function (with existing $allowMultipleAttributes argument, they receive message :
App\Security\MyCustomAccessDecisionManager::decide(): Argument #4 ($allowMultipleAttributes) must be of type bool, Symfony\Component\Security\Core\Authorization\AccessDecision given

patch.patch

MyCustomAccessDecisionManager::decide(): Argument#4 ($allowMultipleAttributes)

Let's resolve this once for all: this argument is not part of any abstract API. It's only part of the concrete (Traceable)AccessDecisionManager, and if you extend it, BC is kept.

About your patch:

• I realized the by-ref approach I suggested doesn't work with fun_get_args(), so it's not BC.
• I removed thefinal modifier on AccessDecision and Vote to open things a bit for extensibility.
• I've yet to see why a strategy would need to get the $accessDecision. The challenge is on you, in a follow up PR ;)

OK.
please for my culture,
this line :
$vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote();
is used to avoid create $vote argument that whould have caused BC ?

Absolutely

* I've yet to see why a strategy would need to get the $accessDecision. The challenge is on you, in a follow up PR ;)

Here is :#60085