8000 [HttpKernel] Add better error message when controller action isn't callable by pierredup · Pull Request #10788 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] Add better error me 8000 ssage when controller action isn't callable #10788

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

Merged
merged 1 commit into from
Sep 23, 2015
Merged

[HttpKernel] Add better error message when controller action isn't callable #10788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,21 +101,25 @@ public function testGetControllerInvokableService()
*/
public function testGetControllerOnNonUndefinedFunction($controller, $exceptionName = null, $exceptionMessage = null)
{
$this->setExpectedException($exceptionName, $exceptionMessage);
// All this logic needs to be duplicated, since calling parent::testGetControllerOnNonUndefinedFunction will override the expected excetion and not use the regex
$resolver = $this->createControllerResolver();
$this->setExpectedExceptionRegExp($exceptionName, $exceptionMessage);

parent::testGetControllerOnNonUndefinedFunction($controller);
$request = Request::create('/');
$request->attributes->set('_controller', $controller);
$resolver->getController($request);
}

public function getUndefinedControllers()
{
return array(
array('foo', '\LogicException', 'Unable to parse the controller name "foo".'),
array('foo::bar', '\InvalidArgumentException', 'Class "foo" does not exist.'),
array('stdClass', '\LogicException', 'Unable to parse the controller name "stdClass".'),
array('foo', '\LogicException', '/Unable to parse the controller name "foo"\./'),
array('foo::bar', '\InvalidArgumentException', '/Class "foo" does not exist\./'),
array('stdClass', '\LogicException', '/Unable to parse the controller name "stdClass"\./'),
array(
'Symfony\Component\HttpKernel\Tests\Controller\ControllerResolverTest::bar',
'\InvalidArgumentException',
8000 'Controller "Symfony\Component\HttpKernel\Tests\Controller\ControllerResolverTest::bar" for URI "/" is not callable.',
'/.?[cC]ontroller(.*?) for URI "\/" is not callable\.( Expected method(.*) Available methods)?/',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regular expression test is necessary because on travis when the tests are run using deps=low the tests failed because of the installed dependecies

),
);
}
Expand Down
63 changes: 62 additions & 1 deletion src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public function getController(Request $request)
$callable = $this->createController($co 10000 ntroller);

if (!is_callable($callable)) {
throw new \InvalidArgumentException(sprintf('Controller "%s" for URI "%s" is not callable.', $controller, $request->getPathInfo()));
throw new \InvalidArgumentException(sprintf('The controller for URI "%s" is not callable. %s', $request->getPathInfo(), $this->getControllerError($callable)));
}

return $callable;
Expand Down Expand Up @@ -167,4 +167,65 @@ protected function instantiateController($class)
{
return new $class();
}

private function getControllerError($callable)
{
if (is_string($callable)) {
if (false !== strpos($callable, '::')) {
$callable = explode('::', $callable);
}

if (class_exists($callable) && !method_exists($callable, '__invoke')) {
return sprintf('Class "%s" does not have a method "__invoke".', $callable);
}

if (!function_exists($callable)) {
return sprintf('Function "%s" does not exist.', $callable);
}
}

if (!is_array($callable)) {
return sprintf('Invalid type for controller given, expected string or array, got "%s".', gettype($callable));
}

if (2 !== count($callable)) {
return sprintf('Invalid format for controller, expected array(controller, method) or controller::method.');
}

list($controller, $method) = $callable;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should handle the case where $controller is neither a string nor an object (i.e. passing crappy things), as this code is precisely here to cover the case where we don't get a callable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise it would break below btw

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point all cases are handled. Even when passing crappy things like 1, you will still get the error Unable to find controller "1".


if (is_string($controller) && !class_exists($controller)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need an extension point for child classes extending the ControllerResolver and adding more supported case for strings (it can be a service id in FrameworkBundle, not only a class name)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is only called when a controller is not callable. The process of getting the callable for the controller happens in the createController method, so at that point the service id would be converted to a callable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the message will tell you Class "app.controller.typoed" does not exist. when you make a typo in your service id, which is not fine. We need an extension point to allow child classes to tweak the error message in this case to account for their additional features

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It shouldn't, because the service id won't ever be passed to this function. When you make a typo, the error will come from ControllerResolver.php#L69 in the FrameworkBundle when the service doesn't exist

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to improve the error message in this place though, otherwise they will still get a bad error message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may over-complicate things, but what if createController was responsible for checking is_callable in different situations and throwing the clear exception there. If we want to prefix all the messages with "The controller for URI..." We could catch the exception in getController and prepend the message.

return sprintf('Class "%s" does not exist.', $controller);
}

$className = is_object($controller) ? get_class($controller) : $controller;

if (method_exists($controller, $method)) {
return sprintf('Method "%s" on class "%s" should be public and non-abstract.', $method, $className);
}

$collection = get_class_methods($controller);

$alternatives = array();

foreach ($collection as $item) {
$lev = levenshtein($method, $item);

if ($lev <= strlen($method) / 3 || false !== strpos($item, $method)) {
$alternatives[] = $item;
}
}

asort($alternatives);

$message = sprintf('Expected method "%s" on class "%s"', $method, $className);

67ED if (count($alternatives) > 0) {
$message .= sprintf(', did you mean "%s"?', implode('", "', $alternatives));
} else {
$message .= sprintf('. Available methods: "%s".', implode('", "', $collection));
}

return $message;
}
}
37 changes: 30 additions & 7 deletions src/Symfony/Component/HttpKernel/Tests/Controller/ControllerResolverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,12 @@ public function testGetControllerWithFunction()
}

/**
* @dataProvider getUndefinedControllers
* @expectedException \InvalidArgumentException
* @dataProvider getUndefinedControllers
*/
public function testGetControllerOnNonUndefinedFunction($controller)
public function testGetControllerOnNonUndefinedFunction($controller, $exceptionName = null, $exceptionMessage = null)
{
$resolver = $this->createControllerResolver();
$this->setExpectedException($exceptionName, $exceptionMessage);

$request = Request::create('/');
$request->attributes->set('_controller', $controller);
Expand All @@ -125,10 +125,14 @@ public function testGetControllerOnNonUndefinedFunction($controller)
public function getUndefinedControllers()
{
return array(
array('foo'),
array('foo::bar'),
array('stdClass'),
array('Symfony\Component\HttpKernel\Tests\Controller\ControllerResolverTest::bar'),
array(1, 'InvalidArgumentException', 'Unable to find controller "1".'),
array('foo', 'InvalidArgumentException', 'Unable to find controller "foo".'),
array('foo::bar', 'InvalidArgumentException', 'Class "foo" does not exist.'),
array('stdClass', 'InvalidArgumentException', 'Unable to find controller "stdClass".'),
array('Symfony\Component\HttpKernel\Tests\Controller\ControllerTest::staticsAction', 'InvalidArgumentException', 'The controller for URI "/" is not callable. Expected method "staticsAction" on class "Symfony\Component\HttpKernel\Tests\Controller\ControllerTest", did you mean "staticAction"?'),
array('Symfony\Component\HttpKernel\Tests\Controller\ControllerTest::privateAction', 'InvalidArgumentException', 'The controller for URI "/" is not callable. Method "privateAction" on class "Symfony\Component\HttpKernel\Tests\Controller\ControllerTest" should be public and non-abstract'),
array('Symfony\Component\HttpKernel\Tests\Controller\ControllerTest::protectedAction', 'InvalidArgumentException', 'The controller for URI "/" is not callable. Method "protectedAction" on class "Symfony\Component\HttpKernel\Tests\Controller\ControllerTest" should be public and non-abstract'),
array('Symfony\Component\HttpKernel\Tests\Controller\ControllerTest::undefinedAction', 'InvalidArgumentException', 'The controller for URI "/" is not callable. Expected method "undefinedAction" on class "Symfony\Component\HttpKernel\Tests\Controller\ControllerTest". Available methods: "publicAction", "staticAction"'),
);
}

Expand Down Expand Up @@ -236,3 +240,22 @@ protected function controllerMethod5(Request $request)
function some_controller_function($foo, $foobar)
{
}

class ControllerTest
{
public function publicAction()
{
}

private function privateAction()
{
}

protected function protectedAction()
{
}

public static function staticAction()
{
}
}
0