bug #15921 Abstract voter tweaks (weaverryan)

fabpot · fabpot · commit 1e65f7189fde · 2015-09-27T11:19:34.000+02:00
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
diff --git a/src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php b/src/Symfony/Component/Security/Core/Authorization/Voter/AbstractVoter.php
@@ -70,12 +70,6 @@ public function vote(TokenInterface $token, $object, array $attributes)
         $vote = self::ACCESS_ABSTAIN;
         $class = get_class($object);
 
-        $reflector = new \ReflectionMethod($this, 'voteOnAttribute');
-        $isNewOverwritten = $reflector->getDeclaringClass()->getName() !== 'Symfony\Component\Security\Core\Authorization\Voter\AbstractVoter';
-        if (!$isNewOverwritten) {
-            @trigger_error(sprintf("The AbstractVoter::isGranted method is deprecated since 2.8 and won't be called anymore in 3.0. Override voteOnAttribute() instead.", $reflector->class), E_USER_DEPRECATED);
-        }
-
         foreach ($attributes as $attribute) {
             if (!$this->supports($attribute, $class)) {
                 continue;
@@ -84,16 +78,9 @@ public function vote(TokenInterface $token, $object, array $attributes)
             // as soon as at least one attribute is supported, default is to deny access
             $vote = self::ACCESS_DENIED;
 
-            if ($isNewOverwritten) {
-                if ($this->voteOnAttribute($attribute, $object, $token)) {
-                    // grant access as soon as at least one voter returns a positive response
-                    return self::ACCESS_GRANTED;
-                }
-            } else {
-                if ($this->isGranted($attribute, $object, $token->getUser())) {
-                    // grant access as soon as at least one voter returns a positive response
-                    return self::ACCESS_GRANTED;
-                }
+            if ($this->voteOnAttribute($attribute, $object, $token)) {
+                // grant access as soon as at least one voter returns a positive response
+                return self::ACCESS_GRANTED;
             }
         }
 
@@ -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)));
     }
 
     /**
@@ -211,6 +199,9 @@ protected function isGranted($attribute, $object, $user = null)
      */
     protected function voteOnAttribute($attribute, $object, TokenInterface $token)
     {
-        return false;
+        // the user should override this method, and not rely on the deprecated isGranted()
+        @trigger_error(sprintf("The AbstractVoter::isGranted() method is deprecated since 2.8 and won't be called anymore in 3.0. Override voteOnAttribute() in %s instead.", get_class($this)), E_USER_DEPRECATED);
+
+        return $this->isGranted($attribute, $object, $token->getUser());
     }
 }
diff --git a/src/Symfony/Component/Security/Core/Tests/Authorization/Voter/AbstractVoterTest.php b/src/Symfony/Component/Security/Core/Tests/Authorization/Voter/AbstractVoterTest.php
@@ -19,17 +19,10 @@
  */
 class AbstractVoterTest extends \PHPUnit_Framework_TestCase
 {
-    /**
-     * @var AbstractVoter
-     */
-    private $voter;
-
     private $token;
 
     protected function setUp()
     {
-        $this->voter = new VoterFixture();
-
         $tokenMock = $this->getMock('Symfony\Component\Security\Core\Authentication\Token\TokenInterface');
         $tokenMock
             ->expects($this->any())
@@ -44,7 +37,9 @@ protected function setUp()
      */
     public function testVote($expectedVote, $object, $attributes, $message)
     {
-        $this->assertEquals($expectedVote, $this->voter->vote($this->token, $object, $attributes), $message);
+        $voter = new VoterFixture();
+
+        $this->assertEquals($expectedVote, $voter->vote($this->token, $object, $attributes), $message);
     }
 
     /**
@@ -58,6 +53,16 @@ public function testVoteUsingDeprecatedIsGranted($expectedVote, $object, $attrib
         $this->assertEquals($expectedVote, $voter->vote($this->token, $object, $attributes), $message);
     }
 
+    /**
+     * @group legacy
+     * @expectedException \BadMethodCallException
+     */
+    public function testNoOverriddenMethodsThrowsException()
+    {
+        $voter = new DeprecatedVoterNothingImplementedFixture();
+        $voter->vote($this->token, new ObjectFixture(), array('foo'));
+    }
+
     public function getData()
     {
         return array(
@@ -113,6 +118,23 @@ protected function isGranted($attribute, $object, $user = null)
     }
 }
 
+class DeprecatedVoterNothingImplementedFixture extends AbstractVoter
+{
+    protected function getSupportedClasses()
+    {
+        return array(
+            'Symfony\Component\Security\Core\Tests\Authorization\Voter\ObjectFixture',
+        );
+    }
+
+    protected function getSupportedAttributes()
+    {
+        return array('foo', 'bar', 'baz');
+    }
+
+    // this is a bad voter that hasn't overridden isGranted or voteOnAttribute
+}
+
 class ObjectFixture
 {
 }
