Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sort the internal list of voters by priority #197

Closed
wants to merge 1 commit into from
Closed

Sort the internal list of voters by priority #197

wants to merge 1 commit into from

Conversation

FlashBlack
Copy link

And piece of code in knp-menubundle
see: KnpLabs/KnpMenuBundle#268

@@ -13,9 +13,14 @@ class Matcher implements MatcherInterface
private $cache;

/**
* @var array[]
*/
private $voters = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must keep compatibility with PHP 5.3

@stof
Copy link
Collaborator

stof commented May 5, 2015

Does KnpMenu really need a concept of priority ? I think running voters in the order in which they are added is enough here. the concept of priority should be used by the code configuring the matcher if needed (it may not be needed depending of how the configuration is done).
KnpMenuBundle could then use the priority to sort tagged services (to give control over the order in which voters are added). Symfony does that in many places (the EventDispatcher is one of the only place where the priority needs to be known by the code rather than the configuration layer btw)

@stof
Copy link
Collaborator

stof commented May 5, 2015

so I'm -1 on this change in the library.

@akovalyov
Copy link
Contributor

@FlashBlack could you add a little bit of details why do you need this change?

@Nek-
Copy link
Contributor

Nek- commented May 24, 2015

@akovalyov notice that the implementation is wrong, and need a test.

Btw I'm 👎 too as voter don't need to be prioritized: if a voter say "no", then it's no (a voter can return null). No matter the order. The only point I see here is to authorize bad application design (things that the voter should not do in the voter).

@stof
Copy link
Collaborator

stof commented Sep 20, 2015

If you need a strict order for voters, just register them in the right order. I don't want to add the concept of priority in the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants