Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.2k
[Cookbook][Security] Update Voters: add class properties#3211
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| private $request; | ||
| private $blacklistedIp; | ||
| public function __construct(Request $request, array $blacklistedIp = array()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
i wonder if we should start using the request_stack?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we can do that, but not for 2.2.
This can cause "not in scope"-errors. I haven't test this, let's wait what@weaverryan think about this. However, as of Symfony 2.3, we should use the request_stack here, as suggested by@cordoval |
Good point, i can update this PR to only add the class properties since they were missing. I was a bit over-enthusiastic with the rest of the changes ;). Thanks for the feedback. |
yeah, I think that's the best. And if you are enthousiast, you can create a PR for 2.3 using request_stack ;-) |
Otherwise, you should change the service definition as well. :) |
FYI, someone just submitted a change for 2.3 (I hope he'll correct his PR):#3216 |
Updated PR, only adds class properties now. |
| @@ -67,6 +67,9 @@ and compare the IP address against a set of blacklisted IP addresses: | |||
| class ClientIpVoter implements VoterInterface | |||
| { | |||
| private $container; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
add a blank line between the properties
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thought it was part of PSR-2. But it turns out that this seems to be only my personal taste.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
personal taste is personal 👶
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
my fault
Fixed the issues reported by@xabbuh, thanks |
[Cookbook][Security] Update Voters: add class properties
Hi guys! Nice work on this - I completely agree that we should use the container here and then handle it better in Symfony 2.4 with the request stack (#3220). Thanks! |
Added class properties.