[FrameworkBundle] Use security.token_storage service in Controller::getUser() #13214
Conversation
df722c5 to
8c5bd20
Compare
|
I'm sure that not everyone agrees, but can you add a test for this maybe? Other than that 👍 |
|
@iltar Tests are added |
|
Just noticed that the added tests wont work when merged into master, due to #12457. It must be probably fixed like #13217. What is the right workflow to handle this? Should I change this PR to include the changes made in #13217 or should this change be merged up to master first (providing that this PR is accepted for 2.6) and should a new PR be created to fix it then? |
|
You can already make the tests forward compatible by making a controller that extends the base controller and test that one. What you then could do is just call the protected methods as in any case. |
4cc0b2e to
982afbd
Compare
|
Thanks @iltar. I have updated the tests. They should be compatible with master now. |
| @@ -37,10 +41,99 @@ public function testForward() | |||
| $container->expects($this->at(0))->method('get')->will($this->returnValue($requestStack)); | |||
| $container->expects($this->at(1))->method('get')->will($this->returnValue($kernel)); | |||
|
|
|||
| $controller = new Controller(); | |||
| $controller = new TestController(); | |||
There was a problem hiding this comment.
Instead of creating a class, where we will add all methods progressively, what about using reflection to access the method?
$m = new \ReflectionMethod('...Controller', 'getUser');
$m->setAccessible(true);
$m->invoke(...);There was a problem hiding this comment.
When I see reflection, I usually notice that the code might not be correct. If you can't test it easily, you might have to rewrite so it is.
I think that the best solution would be if all the basic functionality of the Controller would move away to a loose class (or 2). The current Controller could then delegate the methods and you can test the public methods properly on that specific class. This also makes it easier to re-use certain functionality from the controller if you use 'Controller as a Service'.
I think this falls out of the scope of the PR though.
There was a problem hiding this comment.
Even if I agree with you that "non-easily-testable" code is a smell, in this specific case, this is perfectly valid IMO.
Please, don't overcomplicate things, this class is how we tie everything together, easy and simple.
There was a problem hiding this comment.
Can't this be handled with a magic __call() method in the TestController class?
There was a problem hiding this comment.
I haven't used reflection so far, so I am not very familiar with it. @fabpot Your suggestion sounds like it would avoid the need of the TestController class completely. If I am not wrong, it would only avoid the overriding of the methods. Maybe that was your point. In this case it would not simplify the testcode IMO, since I have to change
$this->assertSame($controller->getUser(), $user);to
$m = new \ReflectionMethod('Symfony\Bundle\FrameworkBundle\Controller\Controller', 'getUser');
$m->setAccessible(true);
$this->assertSame($m->invoke($controller), $user);where I still need
$controller = new TestController();
$controller->setContainer($this->getContainerWithTokenStorage($token));By avoiding adding all the tested methods of Controller progressively, the test methods itself would become unlike more verbose. Although it could be reduced with a helper method, for me, the current version is simpler (for readers, writers and runtime).
@xabbuh Using __call is indeed an option, but has the drawback that the IDE complains about calling the protected method then.
class TestController extends Controller
{
function __call($name, $arguments)
{
return call_user_func_array('parent::'.$name, $arguments);
}
}Let me know if I should change the PR though to either use reflection or the __call method.
To address @iltar's comment... What about capitalizing the abilities of traits in PHP 5.4 for SF 3.0 by providing traits with the shortcut methods, so developers can use the methods without extending a base Controller?
There was a problem hiding this comment.
I find it a bad practice to do that because it still requires me to have a container in my controller. As long as I don't see the container and it's abstracted away, I'm ok with it.
|
Thank you @xelaris. |
…Controller::getUser() (xelaris) This PR was squashed before being merged into the 2.6 branch (closes #13214). Discussion ---------- [FrameworkBundle] Use security.token_storage service in Controller::getUser() | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Replace deprecated `security.context` with `security.token_storage` service. Commits ------- f46ce9c [FrameworkBundle] Use security.token_storage service in Controller::getUser()
Replace deprecated
security.contextwithsecurity.token_storageservice.