Abstract voter tweaks #15921
Abstract voter tweaks #15921
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 to
5961edb
Compare
|
👍 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.
we can only mention voteOnAttribute() here, can't we? No need to talk about overriding a method that shouldn't be used anymore
|
@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.
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 to
54811ec
Compare
|
@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 the class 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. |
…uggestions by stof in symfony#15870
54811ec to
c03f5c2
Compare
|
done - 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/a 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 `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).