Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

use SecurityContextInterface instead of SecurityContext #3522

Merged
merged 3 commits into from Mar 8, 2012

Conversation

Projects
None yet
4 participants
Contributor

pminnieur commented Mar 6, 2012

Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: /
Todo: /

Abstract: it's not possible to exchange the security.context with another implementation without this change. You may not be able to extend the SecurityContext because isGranted is final, so you may implement your own context.

Contributor

pminnieur commented Mar 6, 2012

PS: could you merge this back to 2.0 branch, too?

Member

stof commented Mar 6, 2012

@pminnieur send a pull request to the 2.0 branch then

Contributor

lsmith77 commented Mar 6, 2012

i guess this doesn't break BC as SecurityContext always implemented the SecurityContextInterface .. no?

Contributor

pminnieur commented Mar 6, 2012

this would not break BC, correct. I may identify additonal places where its not typed against the Interface but the implementation, which is really annoying. I will update the PR tomorrow morning and also do a PR for the 2.0 branch.

Member

stof commented Mar 6, 2012

As it is in the constructor, it is not a BC break indeed as overwritten constructors can have a different signature anyway. For other places, take care that it could be a BC issue for people extending the class

Contributor

pminnieur commented Mar 6, 2012

as the isGranted method in the SecurityContext implementation provided by Symfony is declared final, it's not really extendable at all - which ultimately leads to the problem: its indirectly hard coupled ;-)

Member

stof commented Mar 6, 2012

@pminnieur the BC break is not for people extending the SecurityContext but for people extending classes that typehint it

Contributor

pminnieur commented Mar 7, 2012

JFYI: the RememberMeListener also does not type hint the interface but the implementation itself (it's always a constructor argument). All the other Security\Http\Firewall listeners type hint against the interface. I will update the PR accordingly today and also create a second PR against the 2.0 branch.

Contributor

pminnieur commented Mar 7, 2012

JFYI: same issue w/ JMSSecurityExtraBundle schmittjoh/JMSSecurityExtraBundle#44

@fabpot fabpot added a commit that referenced this pull request Mar 8, 2012

@fabpot fabpot merged branch pminnieur/patch-1 (PR #3522)
Commits
-------

bfb5547 fixed docblock
bf75212 use SecurityContextInterface instead of SecurityContext
498b4b6 use SecurityContextInterface instead of SecurityContext

Discussion
----------

use SecurityContextInterface instead of SecurityContext

Bug fix: no
Feature addition: no
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: /
Todo: /

Abstract: it's not possible to exchange the `security.context` with another implementation without this change. You may not be able to extend the `SecurityContext` because `isGranted` is final, so you may implement your own context.

---------------------------------------------------------------------------

by pminnieur at 2012-03-06T17:37:27Z

PS: could you merge this back to 2.0 branch, too?

---------------------------------------------------------------------------

by stof at 2012-03-06T17:42:03Z

@pminnieur send a pull request to the 2.0 branch then

---------------------------------------------------------------------------

by lsmith77 at 2012-03-06T18:42:41Z

i guess this doesn't break BC as SecurityContext always implemented the SecurityContextInterface .. no?

---------------------------------------------------------------------------

by pminnieur at 2012-03-06T19:11:00Z

this would not break BC, correct. I may identify additonal places where its not typed against the Interface but the implementation, which is really annoying. I will update the PR tomorrow morning and also do a PR for the 2.0 branch.

---------------------------------------------------------------------------

by stof at 2012-03-06T22:04:09Z

As it is in the constructor, it is not a BC break indeed as overwritten constructors can have a different signature anyway. For other places, take care that it could be a BC issue for people extending the class

---------------------------------------------------------------------------

by pminnieur at 2012-03-06T22:11:28Z

as the `isGranted ` method in the `SecurityContext ` implementation provided by Symfony is declared `final`, it's not really extendable at all - which ultimately leads to the problem: its indirectly hard coupled ;-)

---------------------------------------------------------------------------

by stof at 2012-03-06T22:38:08Z

@pminnieur the BC break is not for people extending the SecurityContext but for people extending classes that typehint it

---------------------------------------------------------------------------

by pminnieur at 2012-03-07T10:45:55Z

JFYI: the `RememberMeListener ` also does not type hint the interface but the implementation itself (it's always a constructor argument). All the other `Security\Http\Firewall` listeners type hint against the interface. I will update the PR accordingly today and also create a second PR against the 2.0 branch.

---------------------------------------------------------------------------

by pminnieur at 2012-03-07T11:55:52Z

JFYI: same issue w/ JMSSecurityExtraBundle schmittjoh/JMSSecurityExtraBundle#44
369d7aa

@fabpot fabpot merged commit bfb5547 into symfony:master Mar 8, 2012

@craigmarvelley craigmarvelley pushed a commit to craigmarvelley/symfony that referenced this pull request Nov 26, 2013

@fabpot fabpot merged branch pminnieur/2.0 (PR #3537)
Commits
-------

0c9b2d4 use SecurityContextInterface instead of SecurityContext

Discussion
----------

[2.0][Security] use SecurityContextInterface instead of SecurityContext

see symfony#3522 (this is a fix for the 2.0 branch)

---------------------------------------------------------------------------

by pminnieur at 2012-03-21T13:25:59Z

*ping* it still missed the 2.0.12 release ...

---------------------------------------------------------------------------

by stof at 2012-03-21T16:41:28Z

@pminnieur you PR has been merged into master, not into 2.0, so it will only be in 2.1

---------------------------------------------------------------------------

by pminnieur at 2012-03-21T16:43:02Z

I know, and this is a second PR for 2.0 branch.
5ede111
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment