8000 [2.6] [SecurityBundle] use TokenStorageInterface instead of deprecated SecurityContextInterface in SecurityDataCollector and added unit tests suite. by hhamon · Pull Request #13045 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[2.6] [SecurityBundle] use TokenStorageInterface instead of deprecated SecurityContextInterface in SecurityDataCollector and added unit tests suite. #13045

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 21, 2014
Merged

[2.6] [SecurityBundle] use TokenStorageInterface instead of deprecated SecurityContextInterface in SecurityDataCollector and added unit tests suite. #13045

merged 1 commit into from
Dec 21, 2014

Conversation

hhamon
Copy link
Contributor
@hhamon hhamon commented Dec 19, 2014
Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

@hhamon hhamon changed the title [SecurityBundle] use TokenStorageInterface instead of deprecated SecurityContextInterface in SecurityDataCollector and added unit tests suite. [2.6] [SecurityBundle] use TokenStorageInterface instead of deprecated SecurityContextInterface in SecurityDataCollector and added unit tests suite. Dec 20, 2014
@xabbuh
Copy link
Member
xabbuh commented Dec 20, 2014

@hhamon The tests would make sense in 2.3 as well.

@hhamon
Copy link
Contributor Author
hhamon commented Dec 20, 2014

Indeed! I will backport the test on Symfony 2.3 as well.

@linaori
Copy link
Contributor
linaori commented Dec 20, 2014

How public is this class? When I made the split, I didn't change any classes using the SecurityContext as it would mean the constructor would become BC incompatible. It would mean you'd have to support both the new and old constructor arguments.

@xabbuh
Copy link
Member
xabbuh commented Dec 20, 2014

@iltar Given that SecurityContextInterface extends TokenStorageInterface, you can still use the class as before, can't you?

@linaori
Copy link
Contributor
linaori commented Dec 20, 2014

@xabbuh in theory nothing would break, but for people extending this class, things might change. Hence this should be done carefully depending on whether this is public or not.

@hhamon
Copy link
Contributor Author
hhamon commented Dec 20, 2014

The constructor is the only method that can be overriden safely. As I didn't change the argument order and considering the SecurityContext implements both TokenStorageInterface and AuthorizationCheckerInterface, you can still inject a SecurityContext instance as token storage. I can add a unit test to prove it's still BC. What do you think?

@hhamon hhamon force-pushed the securitybundle-security-collector branch from dd05b2a to cd4af9d Compare December 20, 2014 12:10
@hhamon
Copy link
Contributor Author
hhamon commented Dec 20, 2014

Just added an extra unit test to ensure injecting a SecurityContext ensures BC.

@hhamon hhamon force-pushed the securitybundle-security-collector branch from cd4af9d to 8000 875fda0 Compare December 20, 2014 12:13
@linaori
Copy link
Contributor
linaori commented Dec 20, 2014

Looking good to me 👍

[SecurityBundle] use TokenStorageInterface instead of deprecated Secu…
ea4cf33 
…rityContextInterface in SecurityDataCollector and added unit tests suite.
@hhamon hhamon force-pushed the securitybundle-security-collector branch from 875fda0 to ea4cf33 Compare December 20, 2014 21:36
@fabpot
Copy link
Member
fabpot commented Dec 21, 2014

Thank you @hhamon.

@fabpot fabpot merged commit ea4cf33 into symfony:2.6 Dec 21, 2014
fabpot added a commit that referenced this pull request Dec 21, 2014
@fabpot
minor #13045 [2.6] [SecurityBundle] use TokenStorageInterface instead…
188963f 
… of deprecated SecurityContextInterface in SecurityDataCollector and added unit tests suite. (hhamon)

This PR was merged into the 2.6 branch.

Discussion
----------

[2.6] [SecurityBundle] use TokenStorageInterface instead of deprecated SecurityContextInterface in SecurityDataCollector and added unit tests suite.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Commits
-------

ea4cf33 [SecurityBundle] use TokenStorageInterface instead of deprecated SecurityContextInterface in SecurityDataCollector and added unit tests suite.
@hhamon hhamon deleted the securitybundle-security-collector branch December 21, 2014 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hhamon @xabbuh @linaori @fabpot
0