-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] fix tests for the AbstractVoter
class
#15974
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
Conversation
{ | ||
$voter = new AbstractVoterTest_NothingImplementedVoter(); | ||
$voter->vote($this->token, new \stdClass(), array('EDIT')); | ||
} |
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.
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.
Ensuring that we don't let people creating a voter not implementing one of the voting methods, as we will turn voteOnAttribute
into an abstract one in 3.0
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.
Well, then the code is currently broken as this test would not pass. Can you point me into the direction of the code that would be responsible for this? Then I can readd the test and fix the code.
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.
this is because the supported class in AbstractVoterTest_NothingImplementedVoter
is wrong too, same bug than in AbstractVoterTest_LegacyVoter
(this is the issue when refactoring tests between branches, and not finishing things properly after the merge to newer branches)
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.
Never mind, I forgot to fix the getSupportedClasses()
method of the AbstractVoterTest_NothingImplementedVoter
class.
1d72bdd
to
f213b90
Compare
* The `LegacyAbstractVoterTest` class is not needed anymore, tests have been moved to the `AbstractVoterTest` class tagging them with the legacy group. * Tests are applied on `stdClass` object instances. Thus, the legacy voter fixture class must not support `AbstractVoterTest_Object` instances, but support `stdClass` objects instead.
f213b90
to
9fe3b76
Compare
👍 status: reviewed |
The failing tests are not related. |
Thank you @xabbuh. |
This PR was merged into the 2.8 branch. Discussion ---------- [Security] fix tests for the `AbstractVoter` class | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #15961, #15968 | License | MIT | Doc PR | * The `LegacyAbstractVoterTest` class is not needed anymore, tests have been moved to the `AbstractVoterTest` class tagging them with the legacy group. * Tests are applied on `stdClass` object instances. Thus, the legacy voter fixture class must not support `AbstractVoterTest_Object` instances, but support `stdClass` objects instead. * Remove a test that checked for a `BadMethodCallException` being thrown. This seems to have been added accidentally in #15961. Commits ------- 9fe3b76 fix tests for the `AbstractVoter` class
I'm sorry for the trouble my PR this morning caused. I didn't have the correct tools and the time to do it properly. I'm happy the tests are fixed now! |
LegacyAbstractVoterTest
class is not needed anymore, tests havebeen moved to the
AbstractVoterTest
class tagging them with thelegacy group.
stdClass
object instances. Thus, the legacyvoter fixture class must not support
AbstractVoterTest_Object
instances, but support
stdClass
objects instead.BadMethodCallException
beingthrown. This seems to have been added accidentally in [Security] Fix tests in 2.8 #15961.