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 #44

Merged
merged 1 commit into from Mar 7, 2012

Conversation

Projects
None yet
2 participants
Contributor

pminnieur commented Mar 7, 2012

cross-posted issue, see symfony/symfony#3522

@schmittjoh schmittjoh added a commit that referenced this pull request Mar 7, 2012

@schmittjoh schmittjoh Merge pull request #44 from pminnieur/patch-1
use SecurityContextInterface instead of SecurityContext
c26bd5d

@schmittjoh schmittjoh merged commit c26bd5d into schmittjoh:master Mar 7, 2012

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

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment