Fix deprecated SecurityContextInterface usage #315
Conversation
|
Tests added. Ready for review. |
| @@ -6,6 +6,7 @@ | |||
|
|
|||
| <parameters> | |||
| <parameter key="fos_oauth_server.security.authentication.provider.class">FOS\OAuthServerBundle\Security\Authentication\Provider\OAuthProvider</parameter> | |||
| <parameter key="fos_oauth_server.security.authentication.listener.legacy.class">FOS\OAuthServerBundle\Security\Firewall\LegacyOAuthListener</parameter> | |||
There was a problem hiding this comment.
please don't introduce any new class parameters, given that Symfony discourages this usage
There was a problem hiding this comment.
Ok I'll put it directly on Symfony definition. I don't know that is discouraged also for external bundles.
soullivaneuh
force-pushed
the
security-context
branch
from
5d98b2d to
71969bc
Compare
|
@stof I following your recommendation and take profit to remove all parameters classes on Thanks for your advice, this PR is now ready for review and merge. |
soullivaneuh
force-pushed
the
security-context
branch
2 times, most recently
from
8acf3d7 to
fd49de0
Compare
|
@soullivaneuh did you find out why the tests fail? |
soullivaneuh
force-pushed
the
security-context
branch
2 times, most recently
from
cd72c3b to
d2851ea
Compare
|
Tests should be fixed. 👍 |
|
Don't know for the lowest deps failling test, can't reproduce it on my local. Any idea? |
|
@stof Could you please restart it? |
soullivaneuh
force-pushed
the
security-context
branch
from
d2851ea to
526d4b3
Compare
|
Seems to be an issue with php 5.3. Still investigating. |
|
Can't understand what happen. I got the same error on my local with PHP 5.3 but the ContainerBuilder class exists... |
|
I thought perhaps this error was due to the odd namespace (from a different bundle) on the new test. Tried to update this PR with it but looks like I can't do that. Maybe @soullivaneuh could do that and we'll see if it passes? Would be nice to get this merged in. |
|
Note: Test @sarcher commit: sarcher@89d96ef |
| @@ -16,6 +16,7 @@ | |||
| use Symfony\Component\DependencyInjection\ContainerBuilder; | |||
| use Symfony\Component\DependencyInjection\DefinitionDecorator; | |||
| use Symfony\Component\DependencyInjection\Reference; | |||
| use Symfony\Component\HttpKernel\Kernel; | |||
soullivaneuh
force-pushed
the
security-context
branch
2 times, most recently
from
76304be to
cbca50a
Compare
|
@stof Don't know why, but tests fail on php 5.3 becaue of symfony/dependency-injection 2.0. I added a conflict to bump it to 2.1. |
| * @var \Symfony\Component\Security\Core\Authentication\AuthenticationManagerInterface | ||
| * @var TokenStorageInterface|SecurityContextInterface | ||
| */ | ||
| protected $tokenStorage; |
There was a problem hiding this comment.
please make the new property private. Using private visibility makes maintenance much easier, as we don't need to care about BC on them
There was a problem hiding this comment.
Good idea, will work on it.
BTW, if we don't want override, class should be final later. ;-)
Add OAuthFactory tests
soullivaneuh
force-pushed
the
security-context
branch
from
cbca50a to
b3b103f
Compare
|
@stof review fixed. |
|
I'm sorry but why closing this @Ener-Getick? |
|
I closed it in favor of #374 which fixes all compatibility issues with symfony 3.0 ;-) |
|
OK great. 👍 |
This PR fix deprecated SecurityContextInterface usage, introduce since Symfony 2.6.
Symfony 2.3+ BC kept.
@willdurand @stof Have you any suggestion of how handle it easily? Thanks.