Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
Abstract voter tweaks#15921
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
Abstract voter tweaks#15921
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Duplicate of#15886 ? |
@wouterj it is - do you want to update your PR - i.e. add the trigger_error and remove the reflection stuff? I'm not convinced whether my BadMethodCallException is needed or not. |
528e2c6 to5961edbCompare👍 Status: reviewed @weaverryan the BadMethodCallException is a good idea: we will make |
| @@ -191,7 +178,8 @@ protected function getSupportedAttributes() | |||
| */ | |||
| protected function isGranted($attribute, $object, $user = null) | |||
| { | |||
| return false; | |||
| // forces isGranted() or voteOnAttribute() to be overridden | |||
| throw new \BadMethodCallException(sprintf('You must override either the voteOnAttribute() or isGranted() (deprecated) method in "%s"', get_class($this))); | |||
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 only mentionvoteOnAttribute() here, can't we? No need to talk about overriding a method that shouldn't be used anymore
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.
agreed
@weaverryan I think it's easier to finish this PR, I've closed mine. |
Great - PR updated - thanks! |
| @@ -191,7 +178,8 @@ protected function getSupportedAttributes() | |||
| */ | |||
| protected function isGranted($attribute, $object, $user = null) | |||
| { | |||
| return false; | |||
| // forces isGranted() or voteOnAttribute() to be overridden | |||
| throw new \BadMethodCallException(sprintf('You must override the voteOnAttribute() method in "%s"', get_class($this))); | |||
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.
very minor: All exceptions end with a full stop in Syfmony
I can't make any sense of the AbstractVoterTest.This is how I implemented it in a PR merged yesterday, while the code shown in the branch now (and in your PR) is just a copy of the test in a bad location... (seems like something went wrong in the merge:6f7aae9) |
@wouterj I don't see it. In this PR, the 2.8 branch and your linked PR, it all looks like its in |
bc7f342 to54811ecCompare@weaverryan yeah, but take a look at the contents. The 2.8 branch (and your PR) has the contents of the tests that was in |
@wouterj Ah, yep - I see the same thing (I thought the path was the problem). You should create a new PR to fix this, then I can rebase after your merged. You better to do that since that's your test code that needs to be re-added. |
As I explained in#15886 (comment) here won't be any deprecation message for voters that don't support any attribute even though they will break in 3.0 |
@Tobion good point - I'd vote to revert back to the Reflection stuff then - since the problem is with theclass itself (not how some code in the class behaves), so it would be most awesome if we added a deprecation warning as soon as the problematic class is used. |
@weaverryan can you rebase this branch so that tests of the Security component can actually run ? Your PR was opened at the time where the testsuite of the Security component was triggering a fatal error before reaching voters. |
54811ec toc03f5c2Comparedone - and thanks for fixing those tests |
@weaverryan see#15932 for fixing the tests |
Thank you@weaverryan. |
This PR was merged into the 2.8 branch.Discussion----------Abstract voter tweaks| Q | A| ------------- | ---| Bug fix? | yes (a little)| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aBased on suggestions from stof in#15870, this simplifies the BC and deprecation throwing code. This also adds a BadMethodCallException in case the user doesn't override `isGranted` *or* `voteOnAttribute`, because that's just plain wrong (as is calling `isGranted()` on the parent class directly, since that was formerly abstract).Commits-------c03f5c2 Massively simplifying the BC and deprecated-throwing code thanks to suggestions by stof in#15870
Based on suggestions from stof in#15870, this simplifies the BC and deprecation throwing code. This also adds a BadMethodCallException in case the user doesn't override
isGrantedorvoteOnAttribute, because that's just plain wrong (as is callingisGranted()on the parent class directly, since that was formerly abstract).