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

pierredup · 2014-04-26T00:16:39Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR

In the ControllerResolver, if a controller isn't callable, try to give a better description of what went wrong

cordoval · 2014-04-26T00:40:53Z

src/Symfony/Component/HttpKernel/Tests/Controller/ControllerResolverTest.php

+            array('Symfony\Component\HttpKernel\Tests\Fixtures\ControllerTest::staticsAction', 'The controller for URI "/" is not callable. Expected method "staticsAction" on class "Symfony\Component\HttpKernel\Tests\Fixtures\ControllerTest", did you mean "staticAction"?'),
+            array('Symfony\Component\HttpKernel\Tests\Fixtures\ControllerTest::privateAction', 'The controller for URI "/" is not callable. Method "privateAction" on class "Symfony\Component\HttpKernel\Tests\Fixtures\ControllerTest" should not be private'),
+            array('Symfony\Component\HttpKernel\Tests\Fixtures\ControllerTest::protectedAction', 'The controller for URI "/" is not callable. Method "protectedAction" on class "Symfony\Component\HttpKernel\Tests\Fixtures\ControllerTest" should not be protected'),
+            array('Symfony\Component\HttpKernel\Tests\Fixtures\ControllerTest::undefinedAction', 'The controller for URI "/" is not callable. Expected method "undefinedAction" on class "Symfony\Component\HttpKernel\Tests\Fixtures\ControllerTest". Available methods: "publicAction", "staticAction"'),


if the controller test class is used only in this file then it would be in the same file right?

pierredup · 2014-06-29T17:08:06Z

Please add the DX label for this PR.

weaverryan · 2014-07-10T02:53:41Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

+                            $message .= sprintf(', did you mean "%s"?', implode('", "', array_keys($alternatives)));
+                        } else {
+                            $message .= sprintf('. Available methods: "%s"', implode('", "', $collection));
+                        }


In my opinion, the alternatives should be full _controller strings.

So, don't just say, "Did you mean newAction"? If possible, I think we should say, did you mean "AcmeDemoBundle:Default:new"?

This would need to live actually on the ControllerResolver::createController method inside of the FrameworkBundle (as the AcmeDemoBundle:Default:new syntax is specific to the framework).

The first part of the message is Expected method "%s" on class "%s". So saying something like Expected method "newAction" on class "DefaultController", did you mean AcmeDemoBundle:Default:news? doesn't feel right.
This checks for mistakes in the action name, if E.G you have a newsAction, but reference it just as new, then this message will give you an idea of the correct method name to use.
This is useful for any class and method, not just the Bundle:Controller:Action syntax in the FrameworkBundle

weaverryan · 2014-09-22T14:58:27Z

@pierredup I like this PR, so let's see if we can get it merged. This is the last week before the Symfony 2.6 feature freeze, so if we can work quickly, then this could get in for 2.6 :). The biggest issue I see is that there is a lot of logic here to get the message right, which makes it look disorganized. Perhaps if we moved a lot (or all) of this logic into different private functions, it'll help make things look more clear. Specifically, if all the logic from line 92 to line 137 were in a private function, then all the nested if's would be easier to read. I actually just have a hard time following which if block I'm in :).

Thanks!

weaverryan · 2014-09-22T16:16:05Z

src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerResolverTest.php

        $container->expects($this->once())
            ->method('get')
            ->with('foo')
-            ->will($this->returnValue($this))
-        ;
+            ->will($this->returnValue($this));


Did you mean to make these changes?

This was done according to fabbot.io (http://fabbot.io/report/symfony/symfony/10788/16743fe4ac4b5929409b9f79e8349bb2a3460a9a, but the page doesn't exist anymore). I just applied the patch, and assumed the changes is correct?

ah, then you're fine! Thanks for the clarification!

@pierredup Sorry about that but fabbot (and actually PHP-CS-Fixer) is sometimes wrong, and here it is definitely wrong.

weaverryan · 2014-09-22T16:18:56Z

@pierredup There are a few CS changes that I think you made accidentally and the PR needs to be rebased. But after that, 👍 from me - I think it's certainly a big improvement.

pierredup · 2014-09-22T19:44:55Z

I've simplified the check a bit more and squashed the commits, so I think this PR is ready now

fabpot · 2014-09-23T06:04:14Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

@@ -155,4 +159,52 @@ protected function createController($controller)

        return array(new $class(), $method);
    }
+
+    private function getControllerError(array $callable)


That's wrong. The createController() method can return any PHP callable. It just happens that the default implementation returns an array, but that's not a requirement.

weaverryan · 2014-09-24T16:26:22Z

@pierredup I see you made the change fabpot suggested - awesome! Can you rebase if you have a chance to fix the conflict?

stof · 2014-09-25T17:06:38Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

+                $method,
+                $reflection->getName()
+            );
+        } elseif (is_string($callable) && !function_exists($callable)) {


if is enough given that the previous if returns.

fabpot · 2015-02-11T13:53:15Z

@pierredup Do you have some time to finish this PR?

pierredup · 2015-02-12T09:00:45Z

@fabpot Done

pierredup · 2015-08-31T15:17:37Z

ping @fabpot

weaverryan · 2015-09-03T20:50:14Z

Ping @symfony/deciders - failing tests seem unrelated

weaverryan · 2015-09-04T10:53:52Z

👍 All the edge-cases for different types of controllers is quite complex, but I couldn't think of any other edge-cases, and if there are some, it will just cause a less-helpful error message that we can improve on.

weaverryan · 2015-09-04T10:54:19Z

Status: Reviewed

aitboudad · 2015-09-04T11:03:45Z

👍

fabpot · 2015-09-07T07:55:22Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

+            $reflection = new \ReflectionClass($controller);
+
+            if (!method_exists($controller, $method)) {
+                $collection = array_map(


Why not use get_class_methods() here?

fabpot · 2015-09-14T08:10:47Z

@pierredup Can you have a look at the tests. They are broken.

fabpot · 2015-09-14T08:11:17Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

+
+            $message .= $this->getControllerError($callable);
+
+            throw new \InvalidArgumentException($message);


Can you remove the intermediate $message var and do everything on one line?

pierredup · 2015-09-16T10:22:36Z

@fabpot I'm looking into the failing builds, will let you know once the builds are passing

stof · 2015-09-16T10:29:08Z

src/Symfony/Component/HttpKernel/Controller/ControllerResolver.php

+    private function getControllerError($callable)
+    {
+        if (is_string($callable)) {
+            if (!function_exists($callable)) {


this is broken for Foo::bar strings, as such functions won't exist and so will go there. the check for :: needs to be done first

pierredup · 2015-09-23T08:42:49Z

src/Symfony/Bundle/FrameworkBundle/Tests/Controller/ControllerResolverTest.php

            array(
                'Symfony\Component\HttpKernel\Tests\Controller\ControllerResolverTest::bar',
                '\InvalidArgumentException',
-                'Controller "Symfony\Component\HttpKernel\Tests\Controller\ControllerResolverTest::bar" for URI "/" is not callable.',
+                '/.?[cC]ontroller(.*?) for URI "\/" is not callable\.( Expected method(.*) Available methods)?/',


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

pierredup · 2015-09-23T08:51:35Z

Travis tests is now passing. The failures on appveyor looks unrelated to these changes.

fabpot · 2015-09-23T09:00:54Z

Thank you @pierredup.

…action isn't callable (pierredup) This PR was merged into the 3.0-dev branch. Discussion ---------- [HttpKernel] Add better error message when controller action isn't callable | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | In the `ControllerResolver`, if a controller isn't callable, try to give a better description of what went wrong Commits ------- e0e19f6 Add better error message when controller action isn't callable

cordoval reviewed Apr 26, 2014
View reviewed changes

pierredup added the HttpKernel label Apr 28, 2014

weaverryan added the DX label Jun 30, 2014

pierredup mentioned this pull request Jul 6, 2014

[DX] Improve bad controller class error #11304

Closed

weaverryan reviewed Jul 10, 2014
View reviewed changes

pierredup force-pushed the controller-error branch from 43c8666 to 16743fe Compare September 22, 2014 15:46

weaverryan reviewed Sep 22, 2014
View reviewed changes

pierredup force-pushed the controller-error branch from 8bc4fce to 23b81a8 Compare September 22, 2014 19:39

fabpot reviewed Sep 23, 2014
View reviewed changes

pierredup force-pushed the controller-error branch from dd48191 to 1cabe34 Compare September 23, 2014 19:07

stof reviewed Sep 25, 2014
View reviewed changes

pierredup force-pushed the controller-error branch from 1cabe34 to 875d342 Compare February 12, 2015 08:58

pierredup force-pushed the controller-error branch from 875d342 to a17ea7b Compare February 12, 2015 09:04

pierredup force-pushed the controller-error branch from a17ea7b to e6d8bc5 Compare August 31, 2015 15:27

fabpot reviewed Sep 7, 2015
View reviewed changes

pierredup force-pushed the controller-error branch from e6d8bc5 to 40690cc Compare September 8, 2015 20:08

fabpot reviewed Sep 14, 2015
View reviewed changes

pierredup force-pushed the controller-error branch from 40690cc to 33e4482 Compare September 14, 2015 09:46

stof reviewed Sep 16, 2015
View reviewed changes

pierredup force-pushed the controller-error branch 2 times, most recently from 0fa35ae to c7ff100 Compare September 23, 2015 07:21

Add better error message when controller action isn't callable

e0e19f6

pierredup force-pushed the controller-error branch from c7ff100 to e0e19f6 Compare September 23, 2015 08:24

pierredup reviewed Sep 23, 2015
View reviewed changes

fabpot merged commit e0e19f6 into symfony:master Sep 23, 2015

fabpot mentioned this pull request Nov 16, 2015

Release v3.0.0-BETA1 #16560

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!


		$message .= $this->getControllerError($callable);

		throw new \InvalidArgumentException($message);

Uh oh!

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

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

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!