NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[HttpKernel] Add better error message 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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfrompierredup:controller-error

Sep 23, 2015

Merged

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

fabpot merged 1 commit intosymfony:masterfrompierredup:controller-error

Sep 23, 2015

Conversation

Copy link

Contributor

pierredup commentedApr 26, 2014

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

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

cordoval reviewed

Apr 26, 2014

View reviewed changes

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"'),

Copy link

Contributor

Choose a reason for hiding this comment

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

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

pierredup added the HttpKernel label

Apr 28, 2014

Copy link

ContributorAuthor

pierredup commentedJun 29, 2014

Please add the DX label for this PR.

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

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));
		}

Copy link

Member

Choose a reason for hiding this comment

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

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 theControllerResolver::createController method inside of the FrameworkBundle (as theAcmeDemoBundle:Default:new syntax is specific to the framework).

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

The first part of the message isExpected method "%s" on class "%s". So saying something likeExpected 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

Copy link

Member

weaverryan commentedSep 22, 2014

@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 fromline 92 toline 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 whichif block I'm in :).

Thanks!

pierredup force-pushed thecontroller-error branch from43c8666 to16743feCompare

September 22, 2014 15:46

weaverryan reviewed

Sep 22, 2014

View reviewed changes

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

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

Copy link

Member

weaverryanSep 22, 2014

Choose a reason for hiding this comment

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

Did you mean to make these changes?

Copy link

ContributorAuthor

pierredupSep 22, 2014

Choose a reason for hiding this comment

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

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?

Copy link

Member

weaverryanSep 22, 2014

Choose a reason for hiding this comment

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

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

Copy link

Member

fabpotSep 22, 2014

Choose a reason for hiding this comment

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

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

Copy link

Member

weaverryan commentedSep 22, 2014

@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 force-pushed thecontroller-error branch from8bc4fce to23b81a8Compare

September 22, 2014 19:39

Copy link

ContributorAuthor

pierredup commentedSep 22, 2014

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

fabpot reviewed

Sep 23, 2014

View reviewed changes

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)

Copy link

Member

fabpotSep 23, 2014

Choose a reason for hiding this comment

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

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

pierredup force-pushed thecontroller-error branch fromdd48191 to1cabe34Compare

September 23, 2014 19:07

Copy link

Member

weaverryan commentedSep 24, 2014

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

stof reviewed

Sep 25, 2014

View reviewed changes

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

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

Copy link

Member

stofSep 25, 2014

Choose a reason for hiding this comment

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

if is enough given that the previousif returns.

Copy link

Member

fabpot commentedFeb 11, 2015

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

pierredup force-pushed thecontroller-error branch from1cabe34 to875d342Compare

February 12, 2015 08:58

Copy link

ContributorAuthor

pierredup commentedFeb 12, 2015

@fabpot Done

pierredup force-pushed thecontroller-error branch from875d342 toa17ea7bCompare

February 12, 2015 09:04

Copy link

ContributorAuthor

pierredup commentedAug 31, 2015

ping@fabpot

pierredup force-pushed thecontroller-error branch froma17ea7b toe6d8bc5Compare

August 31, 2015 15:27

Copy link

Member

weaverryan commentedSep 3, 2015

Ping @symfony/deciders - failing tests seem unrelated

Copy link

Member

weaverryan commentedSep 4, 2015

👍 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.

Copy link

Member

weaverryan commentedSep 4, 2015

Status: Reviewed

Copy link

Contributor

aitboudad commentedSep 4, 2015

👍

fabpot reviewed

Sep 7, 2015

View reviewed changes

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

		$reflection = new \ReflectionClass($controller);

		if (!method_exists($controller, $method)) {
		$collection = array_map(

Copy link

Member

fabpotSep 7, 2015

Choose a reason for hiding this comment

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

Why not useget_class_methods() here?

pierredup force-pushed thecontroller-error branch frome6d8bc5 to40690ccCompare

September 8, 2015 20:08

Copy link

Member

fabpot commentedSep 14, 2015

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

fabpot reviewed

Sep 14, 2015

View reviewed changes

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


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

		throw new \InvalidArgumentException($message);

Copy link

Member

fabpotSep 14, 2015

Choose a reason for hiding this comment

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

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

pierredup force-pushed thecontroller-error branch from40690cc to33e4482Compare

September 14, 2015 09:46

Copy link

ContributorAuthor

pierredup commentedSep 16, 2015

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

stof reviewed

Sep 16, 2015

View reviewed changes

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

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

Copy link

Member

stofSep 16, 2015

Choose a reason for hiding this comment

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

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

pierredup force-pushed thecontroller-error branch 2 times, most recently from0fa35ae toc7ff100Compare

September 23, 2015 07:21

Add better error message when controller action isn't callable

e0e19f6

pierredup force-pushed thecontroller-error branch fromc7ff100 toe0e19f6Compare

September 23, 2015 08:24

pierredup reviewed

Sep 23, 2015

View reviewed changes

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)?/',

Copy link

ContributorAuthor

pierredupSep 23, 2015

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

Copy link

ContributorAuthor

pierredup commentedSep 23, 2015

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

Copy link

Member

fabpot commentedSep 23, 2015

Thank you@pierredup.

fabpot merged commite0e19f6 intosymfony:master

Sep 23, 2015

fabpot added a commit that referenced this pull request

Sep 23, 2015

feature#10788[HttpKernel] Add better error message when controller …

ab59d6b

…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 wrongCommits-------e0e19f6 Add better error message when controller action isn't callable

fabpot mentioned this pull request

Nov 16, 2015

Release v3.0.0-BETA1#16560

Merged

Labels

DX = Developer eXperience (anything that improves the experience of using Symfony)

HttpKernel Status: Reviewed

9 participants

Movatterモバイル変換

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

pierredup commentedApr 26, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierredup commentedJun 29, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commentedSep 22, 2014

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!

weaverryan commentedSep 22, 2014

Uh oh!

pierredup commentedSep 22, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commentedSep 24, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedFeb 11, 2015

Uh oh!

pierredup commentedFeb 12, 2015

Uh oh!

pierredup commentedAug 31, 2015

Uh oh!

weaverryan commentedSep 3, 2015

Uh oh!

weaverryan commentedSep 4, 2015

Uh oh!

weaverryan commentedSep 4, 2015

Uh oh!

aitboudad commentedSep 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedSep 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierredup commentedSep 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierredup commentedSep 23, 2015

Uh oh!

fabpot commentedSep 23, 2015

Uh oh!

Uh oh!