Add a priority to allow controlling the order of voters #281
Conversation
The order of voters can be important in 2 cases: - the first voter which does not abstain wins the decision. In case multiple voters can take opposite decisions on the same item, the order is important (this will never happen for matchers shipped in the library as they either match or abstain, but never reject a match) - the order of matchers can affect performance: if one of the matchers performs an heavy computation, running it after other matchers can avoid calling it when another matcher is taking a decision.
| $sortedVoters = call_user_func_array('array_merge', $voters); | ||
|
|
||
| foreach ($sortedVoters as $id) { | ||
| $definition->addMethodCall('addVoter', array(new Reference($id))); |
There was a problem hiding this comment.
in the cmf chain router for example we put the priority handling into the chain router. this allows to add more voters at runtime, at the cost of having to sort them at runtime. see https://github.com/symfony-cmf/Routing/blob/master/ChainRouter.php#L80-L93 and https://github.com/symfony-cmf/Routing/blob/master/ChainRouter.php#L124-L134
both have pro and con. is it a realistic scenario that i want to add my voter in a separate compiler pass or at runtime?
There was a problem hiding this comment.
ah ok, just read the discussion in KnpLabs/KnpMenu#197 so we keep as is.
There was a problem hiding this comment.
well, this is what I rejected in #268. I don't want to add the concept of priorities in the library itself.
If you want to add voters in your own compiler pass by modifying the definition directly, you can still manipulate the whole list of method calls of the Definition if you don't want to add it at the end of the list (same than what you would do today).
Do you really need to register a voter at runtime ? I never faced such scenario.
|
this is missing documentation. please add a short note where we explain how to tag voter services. |
|
@dbu we don't have any doc about registering voters currently. I will open a separate issue about that, but documenting the registration of voters should be its own PR. |
|
Actually, we already have such issue: #236 |
|
agreed. added a note in #236 travis seems rather confused with the push build. either its talking to the wrong repo or there is a caching issue. i tried restarting the build but same outcome. the pr build is green however. is that enough? imho we can merge this. |
|
@dbu I pushed to the wrong remote first and delete the branch and pushed back to my fork. but Travis triggered a build for the push to the main repo, and it was the same commit. |
Add a priority to allow controlling the order of voters
increase licence year
The order of voters can be important in 2 cases:
Replaces #268