[HttpKernel] Better exception page when the controller returns nothing #27138

lyrixx · 2018-05-03T13:49:23Z

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

Before:

after:

nicolas-grekas · 2018-05-03T14:39:12Z

src/Symfony/Component/HttpKernel/HttpKernel.php

+                    throw $e;
+                }
+
+                $r = new \ReflectionProperty(\Exception::class, 'trace');


What about using a rebound closure instead of reflection? It'd make things easier to read IMHO.

Warning: Cannot bind closure to scope of internal class Exception

nicolas-grekas · 2018-05-03T14:51:46Z

src/Symfony/Component/HttpKernel/HttpKernel.php

+                $r->setAccessible(true);
+                $r->setValue($e, array_merge(array(
+                    array(
+                        'line' => $e->getLine(),


This could be even more accurate, passing the line of the call_user_fun above (using __LINE__ next to it.)
Aren't they other keys we should put in the frame? How does it look when thrown from inside the controller?

nicolas-grekas · 2018-05-03T14:53:00Z

src/Symfony/Component/HttpKernel/HttpKernel.php

+        }
+
+        if (is_object($controller)) {
+            $r = new \ReflectionClass($controller);


method __invoke()?

yes

BTW, I adapted the code from

symfony/src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php

Lines 373 to 425 in 667924c

protected function parseController($controller)

{

if (is_string($controller) && false !== strpos($controller, '::')) {

$controller = explode('::', $controller);

}

if (is_array($controller)) {

try {

$r = new \ReflectionMethod($controller[0], $controller[1]);

return array(

'class' => is_object($controller[0]) ? get_class($controller[0]) : $controller[0],

'method' => $controller[1],

'file' => $r->getFileName(),

'line' => $r->getStartLine(),

);

} catch (\ReflectionException $e) {

if (is_callable($controller)) {

// using __call or __callStatic

return array(

'class' => is_object($controller[0]) ? get_class($controller[0]) : $controller[0],

'method' => $controller[1],

'file' => 'n/a',

'line' => 'n/a',

);

}

}

}

if ($controller instanceof \Closure) {

$r = new \ReflectionFunction($controller);

return array(

'class' => $r->getName(),

'method' => null,

'file' => $r->getFileName(),

'line' => $r->getStartLine(),

);

}

if (is_object($controller)) {

$r = new \ReflectionClass($controller);

return array(

'class' => $r->getName(),

'method' => null,

'file' => $r->getFileName(),

'line' => $r->getStartLine(),

);

}

return is_string($controller) ? $controller : 'n/a';

}

stof · 2018-05-03T15:19:56Z

src/Symfony/Component/HttpKernel/HttpKernel.php

@@ -146,7 +146,7 @@ private function handleRaw(Request $request, int $type = self::MASTER_REQUEST)
        $arguments = $event->getArguments();

        // call controller
-        $response = \call_user_func_array($controller, $arguments);
+        $response = \call_user_func_array($controller, $arguments); $line = __LINE__;


I would put this on the next line using $line = __LINE__ - 1; instead. The current code does not comply with our coding standards, and would force to exclude the whole HttpKernel.php file from the CS tooling to avoid having PHP-CS-Fixer breaking it on each run.

nicolas-grekas · 2018-05-03T15:39:23Z

src/Symfony/Component/HttpKernel/HttpKernel.php

+                    'line' => $r->getStartLine(),
+                );
+            } catch (\ReflectionException $e) {
+                return null;


try new \ReflectionClass($controller[0])?

To get only the file?

Yes, WDYT ? And start line of the class of course.

Why not, But I don't know how to "simulate" this fail :/ Do you know how to?

Looking at RequestDataCollector I'd say using __call?

I don't understand. Sorry.
If the method does not exist in the first place Symfony throw an error.
I don't want to add dead code to Symfony, so please give me a reproducer ;)

nicolas-grekas · 2018-05-03T17:26:44Z

WDYT of moving all the added logic to a new exception class, child of LogicException?

lyrixx · 2018-05-04T12:16:45Z

@nicolas-grekas I added the new Exception class.

Note: Exception::trace is private, So I need reflection. line and file are protected ;).
and get* methods are final

nicolas-grekas · 2018-05-04T17:35:32Z

src/Symfony/Component/HttpKernel/HttpKernel.php

@@ -162,7 +163,8 @@ private function handleRaw(Request $request, int $type = self::MASTER_REQUEST)
                if (null === $response) {
                    $msg .= ' Did you forget to add a return statement somewhere in your controller?';
                }
-                throw new \LogicException($msg);
+
+                throw new ControllerDoesNotReturnResponseException($msg, $controller, __FILE__, __LINE__ - 17);


we need a test case to ensure we won't miss updating this "17" in the future I suppose

Tests added

fabpot · 2018-05-09T05:57:30Z

Thank you @lyrixx.

… returns nothing (lyrixx) This PR was merged into the 4.2-dev branch. Discussion ---------- [HttpKernel] Better exception page when the controller returns nothing | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27137 | License | MIT | Doc PR | --- Before: ![before](https://user-images.githubusercontent.com/408368/39580501-9c8d5e0a-4ee9-11e8-84a9-aad8566da4b6.png) after: ![after](https://user-images.githubusercontent.com/408368/39580504-a23e3b76-4ee9-11e8-9a89-01e519b94a20.png) Commits ------- f1e9aa4 [HttpKernel] Better exception page when the controller returns nothing

…controller returns nothing (dimabory) This PR was merged into the 4.3-dev branch. Discussion ---------- [HttpKernel] Better exception page when the invokable controller returns nothing | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #27138 | License | MIT | Doc PR | --- __Prerequisites__ _Configure invokable controller_ ```php # config/routes.yaml index: path: / controller: App\Controller\Start ``` __Before:__ ![before](https://user-images.githubusercontent.com/11414342/53577698-ca739000-3b7e-11e9-98ac-8c8e27626fbe.png) __After:__ ![after](https://user-images.githubusercontent.com/11414342/53577733-df502380-3b7e-11e9-8377-a4a97ea73df8.png) --- Take a look for an enhancement/refactoring in `HttpKernel.php` Commits ------- f6c1622 [HttpKernel] Better exception page when the invokable controller returns nothing

carsonbot added Status: Needs Review HttpKernel Feature labels May 3, 2018

lyrixx force-pushed the http-kernel-exception-controller branch from 69dd0c0 to 31a63b9 Compare May 3, 2018 13:50

nicolas-grekas reviewed May 3, 2018

View reviewed changes

lyrixx force-pushed the http-kernel-exception-controller branch from 31a63b9 to 93ecc0b Compare May 3, 2018 15:05

stof requested changes May 3, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels May 3, 2018

lyrixx force-pushed the http-kernel-exception-controller branch from 93ecc0b to 36dacd7 Compare May 3, 2018 15:24

nicolas-grekas reviewed May 3, 2018

View reviewed changes

nicolas-grekas added this to the next milestone May 3, 2018

lyrixx force-pushed the http-kernel-exception-controller branch 2 times, most recently from 8c5d7b2 to c8f75e5 Compare May 4, 2018 12:15

nicolas-grekas reviewed May 4, 2018

View reviewed changes

[HttpKernel] Better exception page when the controller returns nothing

f1e9aa4

lyrixx force-pushed the http-kernel-exception-controller branch from c8f75e5 to f1e9aa4 Compare May 7, 2018 08:40

nicolas-grekas approved these changes May 7, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels May 7, 2018

fabpot approved these changes May 9, 2018

View reviewed changes

fabpot merged commit f1e9aa4 into symfony:master May 9, 2018

lyrixx deleted the http-kernel-exception-controller branch May 9, 2018 08:31

nicolas-grekas removed this from the next milestone Nov 1, 2018

nicolas-grekas added this to the 4.1 milestone Nov 1, 2018

This was referenced Nov 3, 2018

Release v4.2.0-BETA1 #29072

Closed

Release v4.2.0-BETA1 #29073

Merged

dimabory mentioned this pull request Feb 28, 2019

[HttpKernel] Better exception page when the invokable controller returns nothing #30408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HttpKernel] Better exception page when the controller returns nothing #27138

[HttpKernel] Better exception page when the controller returns nothing #27138

	protected function parseController($controller)
	{
	if (is_string($controller) && false !== strpos($controller, '::')) {
	$controller = explode('::', $controller);
	}

	if (is_array($controller)) {
	try {
	$r = new \ReflectionMethod($controller[0], $controller[1]);

	return array(
	'class' => is_object($controller[0]) ? get_class($controller[0]) : $controller[0],
	'method' => $controller[1],
	'file' => $r->getFileName(),
	'line' => $r->getStartLine(),
	);
	} catch (\ReflectionException $e) {
	if (is_callable($controller)) {
	// using __call or __callStatic
	return array(
	'class' => is_object($controller[0]) ? get_class($controller[0]) : $controller[0],
	'method' => $controller[1],
	'file' => 'n/a',
	'line' => 'n/a',
	);
	}
	}
	}

	if ($controller instanceof \Closure) {
	$r = new \ReflectionFunction($controller);

	return array(
	'class' => $r->getName(),
	'method' => null,
	'file' => $r->getFileName(),
	'line' => $r->getStartLine(),
	);
	}

	if (is_object($controller)) {
	$r = new \ReflectionClass($controller);

	return array(
	'class' => $r->getName(),
	'method' => null,
	'file' => $r->getFileName(),
	'line' => $r->getStartLine(),
	);
	}

	return is_string($controller) ? $controller : 'n/a';
	}

[HttpKernel] Better exception page when the controller returns nothing #27138

[HttpKernel] Better exception page when the controller returns nothing #27138

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment