merged branch sli-systems/dc-1 (PR #6080)
This PR was squashed before being merged into the master branch (closes #6080).
Commits
-------
e477a2e Handle case of static controller method and controllers using magic __call() method
Discussion
----------
Handle case of static controller method and controllers using magic __call() method
Improve collecting controller details for edge cases where:
- controller is array, but contains class name and static method
- method doesn't exist, but is handled by magic __call() method
---------------------------------------------------------------------------
by fabpot at 2012-11-21T08:12:08Z
Can you add some unit tests?
---------------------------------------------------------------------------
by sli-systems at 2012-11-21T22:19:17Z
@pierredup
I disagree with the your comment about is_callable() only working with objects. The PHP docs state that the first argument is a callable, so it can be a string, array, closure, and perhaps more.
The test I added also shows that the code works as is :)
I've thought about your suggestion of adding reflection to look up the location of __call(). However, I think this doesn't really add a lot and only complicates matters. Also, as you can see in the new test, there is also __callStatic() to consider.
The fact that file/line are n/a is correct, because the most typical case will be that __call() and __callStatic() will delegate to some other method that might not even be in the same class/file (a subclass I would expect), IMHO.
@fabpot
Good catch about the '/'. I hope the test is complete enough. Looks more like an exercise on PHP callables than anything else, tho ;)
---------------------------------------------------------------------------
by pierredup at 2012-11-22T04:56:18Z
True that ````is_callable```` takes any callable argument, except in the one specific case where you have a ````__call()```` method, and pass an array with the first paramater as a string.
Take the following example:
class Controller {
public function __call($method, $arguments) {}
}
$controller = array('Controller', 'action');
var_dump(is_callable($controller));
Here ````is_callable($controller)```` will actually return ````false````, where if you have ````$controller = array(new Controller, 'action');```` it would return true.
Of course if you have a ````__callStatic```` method, then it would always return true.
Your tests doesn't seem to cover this use case
---------------------------------------------------------------------------
by sli-systems at 2012-11-22T20:27:05Z
Hmm, maybe. I have to admin that I do not know about this case. OTOH, if is_callable returns false is it really callable then? I would think this more of a PHP bug then?
I think I might have come across this case during coding, but then dismissed it because in that case FilterControllerEvent failed already before the data collector code is reached.
In FilterControllerEvent there is a check on is_callable and a LogicException is thrown if $controller is not callable.
So, is FilterControllerEvent wrong too then?
---------------------------------------------------------------------------
by pierredup at 2012-11-22T20:41:14Z
One would think that if is_callable returns false, then the controller isn't callable, but in the case I mentioned above, the controller is in fact callable. I also thought it was a bug with php, but the php-internals don't seem to think so.
The problem is, if you specify the class as a string, php looks for a static method, even if you have a __call method, it won't be registered.
I will have a look at the FilterControllerEvent to see if this use case applies there as well.
---------------------------------------------------------------------------
by sli-systems at 2012-11-22T20:50:32Z
Rather strange - if that is the case then using is_callable seems pretty pointless and the only way would be to try to execute the controller to find out if it is, in fact, callable...
---------------------------------------------------------------------------
by pierredup at 2012-11-22T20:51:07Z
Okay so it actually seems that the case above isn't callable after all. If the controller is specified as a string, then a static method need to exist. Hence why it works with __callStatic. Only when an instance of the class is specified, will it handle the __call method.
---------------------------------------------------------------------------
by sli-systems at 2012-11-22T20:57:55Z
So the tests are sufficient then?
---------------------------------------------------------------------------
by pierredup at 2012-11-22T20:59:22Z
Yes it is.
This happens when you just assume something without actually testing it :)
Sorry for the hassle
This commit is contained in:
Fabien Potencier
commit
c8ebc1e74b
@ -98,13 +98,25 @@ class RequestDataCollector extends DataCollector implements EventSubscriberInter
|
||||
if (isset($this->controllers[$request])) {
|
||||
$controller = $this->controllers[$request];
|
||||
if (is_array($controller)) {
|
||||
$r = new \ReflectionMethod($controller[0], $controller[1]);
|
||||
$this->data['controller'] = array(
|
||||
'class' => get_class($controller[0]),
|
||||
'method' => $controller[1],
|
||||
'file' => $r->getFilename(),
|
||||
'line' => $r->getStartLine(),
|
||||
);
|
||||
try {
|
||||
$r = new \ReflectionMethod($controller[0], $controller[1]);
|
||||
$this->data['controller'] = array(
|
||||
'class' => is_object($controller[0]) ? get_class($controller[0]) : $controller[0],
|
||||
'method' => $controller[1],
|
||||
'file' => $r->getFilename(),
|
||||
'line' => $r->getStartLine(),
|
||||
);
|
||||
} catch (\ReflectionException $re) {
|
||||
if (is_callable($controller)) {
|
||||
// using __call or __callStatic
|
||||
$this->data['controller'] = array(
|
||||
'class' => is_object($controller[0]) ? get_class($controller[0]) : $controller[0],
|
||||
'method' => $controller[1],
|
||||
'file' => 'n/a',
|
||||
'line' => 'n/a',
|
||||
);
|
||||
}
|
||||
}
|
||||
} elseif ($controller instanceof \Closure) {
|
||||
$this->data['controller'] = 'Closure';
|
||||
} else {
|
||||
|
||||
@ -11,10 +11,14 @@
|
||||
|
||||
namespace Symfony\Component\HttpKernel\Tests\DataCollector;
|
||||
|
||||
use Symfony\Component\HttpKernel\HttpKernel;
|
||||
use Symfony\Component\HttpKernel\HttpKernelInterface;
|
||||
use Symfony\Component\HttpKernel\DataCollector\RequestDataCollector;
|
||||
use Symfony\Component\HttpKernel\Event\FilterControllerEvent;
|
||||
use Symfony\Component\HttpFoundation\Request;
|
||||
use Symfony\Component\HttpFoundation\Response;
|
||||
use Symfony\Component\HttpFoundation\Cookie;
|
||||
use Symfony\Component\EventDispatcher\EventDispatcher;
|
||||
|
||||
class RequestDataCollectorTest extends \PHPUnit_Framework_TestCase
|
||||
{
|
||||
@ -50,6 +54,95 @@ class RequestDataCollectorTest extends \PHPUnit_Framework_TestCase
|
||||
$this->assertEquals('application/json',$c->getContentType());
|
||||
}
|
||||
|
||||
/**
|
||||
* Test various types of controller callables.
|
||||
*
|
||||
* @dataProvider provider
|
||||
*/
|
||||
public function testControllerInspection(Request $request, Response $response)
|
||||
{
|
||||
// make sure we always match the line number
|
||||
$r1 = new \ReflectionMethod($this, 'testControllerInspection');
|
||||
$r2 = new \ReflectionMethod($this, 'staticControllerMethod');
|
||||
// test name, callable, expected
|
||||
$controllerTests = array(
|
||||
array(
|
||||
'"Regular" callable',
|
||||
array($this, 'testControllerInspection'),
|
||||
array(
|
||||
'class' => 'Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest',
|
||||
'method' => 'testControllerInspection',
|
||||
'file' => __FILE__,
|
||||
'line' => $r1->getStartLine()
|
||||
),
|
||||
),
|
||||
|
||||
array(
|
||||
'Closure',
|
||||
function() { return 'foo'; },
|
||||
'Closure',
|
||||
),
|
||||
|
||||
array(
|
||||
'Static callback as string',
|
||||
'Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest::staticControllerMethod',
|
||||
'Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest::staticControllerMethod',
|
||||
),
|
||||
|
||||
array(
|
||||
'Static callable with instance',
|
||||
array($this, 'staticControllerMethod'),
|
||||
array(
|
||||
'class' => 'Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest',
|
||||
'method' => 'staticControllerMethod',
|
||||
'file' => __FILE__,
|
||||
'line' => $r2->getStartLine()
|
||||
),
|
||||
),
|
||||
|
||||
array(
|
||||
'Static callable with class name',
|
||||
array('Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest', 'staticControllerMethod'),
|
||||
array(
|
||||
'class' => 'Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest',
|
||||
'method' => 'staticControllerMethod',
|
||||
'file' => __FILE__,
|
||||
'line' => $r2->getStartLine()
|
||||
),
|
||||
),
|
||||
|
||||
array(
|
||||
'Callable with instance depending on __call()',
|
||||
array($this, 'magicMethod'),
|
||||
array(
|
||||
'class' => 'Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest',
|
||||
'method' => 'magicMethod',
|
||||
'file' => 'n/a',
|
||||
'line' => 'n/a'
|
||||
),
|
||||
),
|
||||
|
||||
array(
|
||||
'Callable with class name depending on __callStatic()',
|
||||
array('Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest', 'magicMethod'),
|
||||
array(
|
||||
'class' => 'Symfony\Component\HttpKernel\Tests\DataCollector\RequestDataCollectorTest',
|
||||
'method' => 'magicMethod',
|
||||
'file' => 'n/a',
|
||||
'line' => 'n/a'
|
||||
),
|
||||
),
|
||||
);
|
||||
|
||||
$c = new RequestDataCollector();
|
||||
|
||||
foreach ($controllerTests as $controllerTest) {
|
||||
$this->injectController($c, $controllerTest[1], $request);
|
||||
$c->collect($request, $response);
|
||||
$this->assertEquals($controllerTest[2], $c->getController(), sprintf('Testing: %s', $controllerTest[0]));
|
||||
}
|
||||
}
|
||||
|
||||
public function provider()
|
||||
{
|
||||
if (!class_exists('Symfony\Component\HttpFoundation\Request')) {
|
||||
@ -71,4 +164,39 @@ class RequestDataCollectorTest extends \PHPUnit_Framework_TestCase
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* Inject the given controller callable into the data collector.
|
||||
*/
|
||||
protected function injectController($collector, $controller, $request)
|
||||
{
|
||||
$resolver = $this->getMock('Symfony\\Component\\HttpKernel\\Controller\\ControllerResolverInterface');
|
||||
$httpKernel = new HttpKernel(new EventDispatcher(), $resolver);
|
||||
$event = new FilterControllerEvent($httpKernel, $controller, $request, HttpKernelInterface::MASTER_REQUEST);
|
||||
$collector->onKernelController($event);
|
||||
}
|
||||
|
||||
/**
|
||||
* Dummy method used as controller callable
|
||||
*/
|
||||
public static function staticControllerMethod()
|
||||
{
|
||||
throw new \LogicException('Unexpected method call');
|
||||
}
|
||||
|
||||
/**
|
||||
* Magic method to allow non existing methods to be called and delegated.
|
||||
*/
|
||||
public function __call($method, $args)
|
||||
{
|
||||
throw new \LogicException('Unexpected method call');
|
||||
}
|
||||
|
||||
/**
|
||||
* Magic method to allow non existing methods to be called and delegated.
|
||||
*/
|
||||
public static function __callStatic($method, $args)
|
||||
{
|
||||
throw new \LogicException('Unexpected method call');
|
||||
}
|
||||
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user