Skip to content

[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

Merged
merged 1 commit into from
Sep 28, 2015

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Sep 28, 2015

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 [Security] Fix tests in 2.8 #15961.

{
$voter = new AbstractVoterTest_NothingImplementedVoter();
$voter->vote($this->token, new \stdClass(), array('EDIT'));
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wouterj I did not find a reason why this test was added in #15961. Was there a special reason for it?

Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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)

Copy link
Member Author

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.

* 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.
@stof
Copy link
Member

stof commented Sep 28, 2015

👍

status: reviewed

@xabbuh
Copy link
Member Author

xabbuh commented Sep 28, 2015

The failing tests are not related.

@fabpot
Copy link
Member

fabpot commented Sep 28, 2015

Thank you @xabbuh.

@fabpot fabpot merged commit 9fe3b76 into symfony:2.8 Sep 28, 2015
fabpot added a commit that referenced this pull request Sep 28, 2015
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
@wouterj
Copy link
Member

wouterj commented Sep 28, 2015

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!

@xabbuh xabbuh deleted the security-tests branch September 28, 2015 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants