Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[FrameworkBundle] A controller must be callable #6798

Closed
wants to merge 1 commit into from

2 participants

@vicb

Am I right to say that a controller must be callable, ie method_exists() is a bug ?

No BC, tests pass.

The change is a one liner however I have rewritten some code & tests to the current standards (what a long way since the genesis of Sf2 !)

I have also added a FIXME for Sf3, because returning false is too much PHP !

@fabpot
Owner

Closing in favor of #7739

@fabpot fabpot closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 18, 2013
  1. @vicb
This page is out of date. Refresh to see the latest.
View
3  src/Symfony/Bundle/FrameworkBundle/Controller/ControllerResolver.php
@@ -48,6 +48,9 @@ public function __construct(ContainerInterface $container, ControllerNameParser
* @param string $controller A Controller string
*
* @return mixed A PHP callable
+ *
+ * @throws \LogicException When the controller format is not recognized
+ * @throws \InvalidArgumentException When the controller class does not exist
*/
protected function createController($controller)
{
View
42 src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php
@@ -72,13 +72,14 @@ public function getController(Request $request)
return new $controller;
}
- list($controller, $method) = $this->createController($controller);
+ $callable = $this->createController($controller);
- if (!method_exists($controller, $method)) {
- throw new \InvalidArgumentException(sprintf('Method "%s::%s" does not exist.', get_class($controller), $method));
+ if (!is_callable($callable)) {
+ list($controller, $method) = $callable;
+ throw new \InvalidArgumentException(sprintf('Method "%s::%s" is not callable.', get_class($controller), $method));
}
- return array($controller, $method);
+ return $callable;
}
/**
@@ -87,6 +88,8 @@ public function getController(Request $request)
* @param Request $request A Request instance
* @param mixed $controller A PHP callable
*
+ * @return array An array of parameters
+ *
* @throws \RuntimeException When value for argument given is not provided
*
* @api
@@ -112,21 +115,28 @@ protected function doGetArguments(Request $request, $controller, array $paramete
foreach ($parameters as $param) {
if (array_key_exists($param->name, $attributes)) {
$arguments[] = $attributes[$param->name];
- } elseif ($param->getClass() && $param->getClass()->isInstance($request)) {
+ continue;
+ }
+
+ if ($param->getClass() && $param->getClass()->isInstance($request)) {
$arguments[] = $request;
- } elseif ($param->isDefaultValueAvailable()) {
+ continue;
+ }
+
+ if ($param->isDefaultValueAvailable()) {
$arguments[] = $param->getDefaultValue();
+ continue;
+ }
+
+ if (is_array($controller)) {
+ $repr = sprintf('%s::%s()', get_class($controller[0]), $controller[1]);
+ } elseif (is_object($controller)) {
+ $repr = get_class($controller);
} else {
- if (is_array($controller)) {
- $repr = sprintf('%s::%s()', get_class($controller[0]), $controller[1]);
- } elseif (is_object($controller)) {
- $repr = get_class($controller);
- } else {
- $repr = $controller;
- }
-
- throw new \RuntimeException(sprintf('Controller "%s" requires that you provide a value for the "$%s" argument (because there is no default value or because there is a non optional argument after this one).', $repr, $param->name));
+ $repr = $controller;
}
+
+ throw new \RuntimeException(sprintf('Controller "%s" requires that you provide a value for the "$%s" argument (because there is no default value or because there is a non optional argument after this one).', $repr, $param->name));
}
return $arguments;
@@ -138,6 +148,8 @@ protected function doGetArguments(Request $request, $controller, array $paramete
* @param string $controller A Controller string
*
* @return mixed A PHP callable
+ *
+ * @throws \InvalidArgumentException When the controller does not exist
*/
protected function createController($controller)
{
View
2  src/Symfony/Component/HttpKernel/Controller/ControllerResolverInterface.php
@@ -41,6 +41,8 @@
* @return mixed|Boolean A PHP callable representing the Controller,
* or false if this resolver is not able to determine the controller
*
+ * FIXME: Symfony3 return null instead of false
+ *
* @throws \InvalidArgumentException|\LogicException If the controller can't be found
*
* @api
View
240 tests/Symfony/Tests/Component/HttpKernel/Controller/ControllerResolverTest.php
@@ -18,144 +18,238 @@
class ControllerResolverTest extends \PHPUnit_Framework_TestCase
{
- public function testGetController()
+ protected static $logger;
+ protected static $resolver;
+
+ public static function setUpBeforeClass()
{
- $logger = new Logger();
- $resolver = new ControllerResolver($logger);
+ self::$logger = new Logger();
+ self::$resolver= new ControllerResolver(self::$logger);
+ }
+
+ public static function tearDownAfterClass()
+ {
+ self::$logger = null;
+ self::$resolver = null;
+ }
+ public function test_controllerAttributeRequired()
+ {
$request = Request::create('/');
- $this->assertFalse($resolver->getController($request), '->getController() returns false when the request has no _controller attribute');
- $this->assertEquals(array('Unable to look for the controller as the "_controller" parameter is missing'), $logger->getLogs('warn'));
+ $this->assertFalse(self::$resolver->getController($request), '->getController() returns false when the request has no _controller attribute');
+ $this->assertEquals(array('Unable to look for the controller as the "_controller" parameter is missing'), self::$logger->getLogs('warn'));
+ }
- $request->attributes->set('_controller', 'Symfony\Tests\Component\HttpKernel\ControllerResolverTest::testGetController');
- $controller = $resolver->getController($request);
- $this->assertInstanceOf('Symfony\Tests\Component\HttpKernel\ControllerResolverTest', $controller[0], '->getController() returns a PHP callable');
+ public function testGetControllerReturnsACallable()
+ {
+ $request = Request::create('/');
+ $request->attributes->set('_controller', 'Symfony\Tests\Component\HttpKernel\Controller::testAction');
+ $controller = self::$resolver->getController($request);
+ $this->assertInstanceOf('Symfony\Tests\Component\HttpKernel\Controller', $controller[0], '->getController() returns a PHP callable');
+ $this->assertSame('testAction', $controller[1]);
+ }
- $request->attributes->set('_controller', $lambda = function () {});
- $controller = $resolver->getController($request);
- $this->assertSame($lambda, $controller);
+ public function testClosureAsController()
+ {
+ $request = Request::create('/');
+ $request->attributes->set('_controller', $closure = function () {});
+ $controller = self::$resolver->getController($request);
+ $this->assertSame($closure, $controller);
+ }
- $request->attributes->set('_controller', $this);
- $controller = $resolver->getController($request);
- $this->assertSame($this, $controller);
+ public function testClassInstanceAsController()
+ {
+ $request = Request::create('/');
+ $controller = new Controller();
+ $request->attributes->set('_controller', $controller);
+ $resolvedController = self::$resolver->getController($request);
+ $this->assertSame($controller, $resolvedController);
+ }
- $request->attributes->set('_controller', 'Symfony\Tests\Component\HttpKernel\ControllerResolverTest');
- $controller = $resolver->getController($request);
- $this->assertInstanceOf('Symfony\Tests\Component\HttpKernel\ControllerResolverTest', $controller);
+ public function testClassNameAsController()
+ {
+ $request = Request::create('/');
+ $request->attributes->set('_controller', 'Symfony\Tests\Component\HttpKernel\Controller');
+ $controller = self::$resolver->getController($request);
+ $this->assertInstanceOf('Symfony\Tests\Component\HttpKernel\Controller', $controller);
+ }
- $request->attributes->set('_controller', array($this, 'controllerMethod1'));
- $controller = $resolver->getController($request);
- $this->assertSame(array($this, 'controllerMethod1'), $controller);
+ public function testClassInstanceMethodNameAsController()
+ {
+ $controller = new Controller();
+ $request = Request::create('/');
+ $request->attributes->set('_controller', array($controller, 'controllerMethod1'));
+ $resolvedController = self::$resolver->getController($request);
+ $this->assertSame(array($controller, 'controllerMethod1'), $resolvedController);
+ }
- $request->attributes->set('_controller', array('Symfony\Tests\Component\HttpKernel\ControllerResolverTest', 'controllerMethod4'));
- $controller = $resolver->getController($request);
- $this->assertSame(array('Symfony\Tests\Component\HttpKernel\ControllerResolverTest', 'controllerMethod4'), $controller);
+ public function testClassNameMethodNameAsController()
+ {
+ $request = Request::create('/');
+ $request->attributes->set('_controller', array('Symfony\Tests\Component\HttpKernel\Controller', 'controllerMethod4'));
+ $controller = self::$resolver->getController($request);
+ $this->assertSame(array('Symfony\Tests\Component\HttpKernel\Controller', 'controllerMethod4'), $controller);
+ }
+ /**
+ * @expectedException \InvalidArgumentException
+ */
+ public function testInvalidControllerClass()
+ {
+ $request = Request::create('/');
$request->attributes->set('_controller', 'foo');
- try {
- $resolver->getController($request);
- $this->fail('->getController() throws an \InvalidArgumentException if the _controller attribute is not well-formatted');
- } catch (\Exception $e) {
- $this->assertInstanceOf('\InvalidArgumentException', $e, '->getController() throws an \InvalidArgumentException if the _controller attribute is not well-formatted');
- }
+ self::$resolver->getController($request);
+ }
+ /**
+ * @expectedException \InvalidArgumentException
+ */
+ public function testInvalidControllerClassAndMethod()
+ {
+ $request = Request::create('/');
$request->attributes->set('_controller', 'foo::bar');
- try {
- $resolver->getController($request);
- $this->fail('->getController() throws an \InvalidArgumentException if the _controller attribute contains a non-existent class');
- } catch (\Exception $e) {
- $this->assertInstanceOf('\InvalidArgumentException', $e, '->getController() throws an \InvalidArgumentException if the _controller attribute contains a non-existent class');
- }
+ self::$resolver->getController($request);
+ }
- $request->attributes->set('_controller', 'Symfony\Tests\Component\HttpKernel\ControllerResolverTest::bar');
- try {
- $resolver->getController($request);
- $this->fail('->getController() throws an \InvalidArgumentException if the _controller attribute contains a non-existent method');
- } catch (\Exception $e) {
- $this->assertInstanceOf('\InvalidArgumentException', $e, '->getController() throws an \InvalidArgumentException if the _controller attribute contains a non-existent method');
- }
+ /**
+ * @expectedException \InvalidArgumentException
+ */
+ public function testInvalidControllerMethod()
+ {
+ $request = Request::create('/');
+ $request->attributes->set('_controller', 'Symfony\Tests\Component\HttpKernel\Controller::bar');
+ self::$resolver->getController($request);
}
- public function testGetArguments()
+ /**
+ * @expectedException \InvalidArgumentException
+ */
+ public function testActionMustBePublic()
{
- $resolver = new ControllerResolver();
+ $request = Request::create('/');
+ $request->attributes->set('_controller', 'Symfony\Tests\Component\HttpKernel\Controller::protectedAction');
+ self::$resolver->getController($request);
+ }
+ public function testNoArguments()
+ {
$request = Request::create('/');
- $controller = array(new self(), 'testGetArguments');
- $this->assertEquals(array(), $resolver->getArguments($request, $controller), '->getArguments() returns an empty array if the method takes no arguments');
+ $controller = array(new Controller(), 'testAction');
+ $this->assertEquals(array(), self::$resolver->getArguments($request, $controller), '->getArguments() returns an empty array if the method takes no arguments');
+ }
+ public function testSingleArgument()
+ {
$request = Request::create('/');
$request->attributes->set('foo', 'foo');
- $controller = array(new self(), 'controllerMethod1');
- $this->assertEquals(array('foo'), $resolver->getArguments($request, $controller), '->getArguments() returns an array of arguments for the controller method');
+ $controller = array(new Controller(), 'controllerMethod1');
+ $this->assertEquals(array('foo'), self::$resolver->getArguments($request, $controller), '->getArguments() returns an array of arguments for the controller method');
+ }
+ public function testTwoArgumentsWithDefault()
+ {
$request = Request::create('/');
$request->attributes->set('foo', 'foo');
- $controller = array(new self(), 'controllerMethod2');
- $this->assertEquals(array('foo', null), $resolver->getArguments($request, $controller), '->getArguments() uses default values if present');
+ $controller = array(new Controller(), 'controllerMethod2');
+ $this->assertEquals(array('foo', null), self::$resolver->getArguments($request, $controller), '->getArguments() uses default values if present');
+ }
+ public function testTwoArgumentsAndOverrideDefaults()
+ {
+ $request = Request::create('/');
+ $request->attributes->set('foo', 'foo');
$request->attributes->set('bar', 'bar');
- $this->assertEquals(array('foo', 'bar'), $resolver->getArguments($request, $controller), '->getArguments() overrides default values if provided in the request attributes');
+ $controller = array(new Controller(), 'controllerMethod2');
+ $this->assertEquals(array('foo', 'bar'), self::$resolver->getArguments($request, $controller), '->getArguments() overrides default values if provided in the request attributes');
+ }
+ public function testClosureSingleArgument()
+ {
$request = Request::create('/');
$request->attributes->set('foo', 'foo');
$controller = function ($foo) {};
- $this->assertEquals(array('foo'), $resolver->getArguments($request, $controller));
+ $this->assertEquals(array('foo'), self::$resolver->getArguments($request, $controller));
+ }
+ public function testClosureTwoArgumentsWithDefaults()
+ {
$request = Request::create('/');
$request->attributes->set('foo', 'foo');
$controller = function ($foo, $bar = 'bar') {};
- $this->assertEquals(array('foo', 'bar'), $resolver->getArguments($request, $controller));
+ $this->assertEquals(array('foo', 'bar'), self::$resolver->getArguments($request, $controller));
+ }
+ public function test__invokeArgumentsWithDefaults()
+ {
$request = Request::create('/');
$request->attributes->set('foo', 'foo');
- $controller = new self();
- $this->assertEquals(array('foo', null), $resolver->getArguments($request, $controller));
- $request->attributes->set('bar', 'bar');
- $this->assertEquals(array('foo', 'bar'), $resolver->getArguments($request, $controller));
+ $controller = new Controller();
+ $this->assertEquals(array('foo', null), self::$resolver->getArguments($request, $controller));
+ }
+ public function test__invokeArgumentsOverrideDefaults()
+ {
$request = Request::create('/');
$request->attributes->set('foo', 'foo');
- $request->attributes->set('foobar', 'foobar');
- $controller = array(new self(), 'controllerMethod3');
+ $controller = new Controller();
+ $request->attributes->set('bar', 'bar');
+ $this->assertEquals(array('foo', 'bar'), self::$resolver->getArguments($request, $controller));
+ }
+ /**
+ * @expectedException \RuntimeException
+ */
+ public function testThreeArguments()
+ {
if (version_compare(PHP_VERSION, '5.3.16', '==')) {
$this->markTestSkipped('PHP 5.3.16 has a major bug in the Reflection sub-system');
- } else {
- try {
- $resolver->getArguments($request, $controller);
- $this->fail('->getArguments() throws a \RuntimeException exception if it cannot determine the argument value');
- } catch (\Exception $e) {
- $this->assertInstanceOf('\RuntimeException', $e, '->getArguments() throws a \RuntimeException exception if it cannot determine the argument value');
- }
}
+ $request = Request::create('/');
+ $request->attributes->set('foo', 'foo');
+ $request->attributes->set('foobar', 'foobar');
+ $controller = array(new Controller(), 'controllerMethod3');
+ self::$resolver->getArguments($request, $controller);
+ }
+ public function testInjectRequest()
+ {
$request = Request::create('/');
- $controller = array(new self(), 'controllerMethod5');
- $this->assertEquals(array($request), $resolver->getArguments($request, $controller), '->getArguments() injects the request');
+ $controller = array(new Controller(), 'controllerMethod5');
+ $this->assertEquals(array($request), self::$resolver->getArguments($request, $controller), '->getArguments() injects the request');
}
+}
+class Controller
+{
public function __invoke($foo, $bar = null)
{
}
- protected function controllerMethod1($foo)
+ public function testAction()
+ {
+ }
+
+ protected function protectedAction()
+ {
+ }
+
+ public function controllerMethod1($foo)
{
}
- protected function controllerMethod2($foo, $bar = null)
+ public function controllerMethod2($foo, $bar = null)
{
}
- protected function controllerMethod3($foo, $bar = null, $foobar)
+ public function controllerMethod3($foo, $bar = null, $foobar)
{
}
- protected static function controllerMethod4()
+ public static function controllerMethod4()
{
}
- protected function controllerMethod5(Request $request)
+ public function controllerMethod5(Request $request)
{
}
}
Something went wrong with that request. Please try again.