NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

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

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:masterfromlyrixx:http-kernel-exception-controller

May 9, 2018

Merged

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

fabpot merged 1 commit intosymfony:masterfromlyrixx:http-kernel-exception-controller

May 9, 2018

Conversation

Copy link

Member

lyrixx commentedMay 3, 2018•
edited
Loading

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:

carsonbot added Status: Needs Review HttpKernel Feature labels

May 3, 2018

lyrixx force-pushed thehttp-kernel-exception-controller branch from69dd0c0 to31a63b9Compare

May 3, 2018 13:50

nicolas-grekas reviewed

May 3, 2018

View reviewed changes

src/Symfony/Component/HttpKernel/HttpKernel.php Outdated

		throw$e;
		}

		$r =new \ReflectionProperty(\Exception::class,'trace');

Copy link

Member

nicolas-grekasMay 3, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

Choose a reason for hiding this comment

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

Warning: Cannot bind closure to scope of internal class Exception

nicolas-grekas reviewed

May 3, 2018

View reviewed changes

src/Symfony/Component/HttpKernel/HttpKernel.php Outdated

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

Copy link

Member

Choose a reason for hiding this comment

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

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?

Copy link

MemberAuthor

lyrixxMay 3, 2018

Choose a reason for hiding this comment

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

fixed

nicolas-grekas reviewed

May 3, 2018

View reviewed changes

src/Symfony/Component/HttpKernel/HttpKernel.php Outdated

		}

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

Copy link

Member

nicolas-grekasMay 3, 2018

Choose a reason for hiding this comment

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

method__invoke()?

Copy link

MemberAuthor

lyrixxMay 3, 2018•
edited
Loading

Choose a reason for hiding this comment

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

yes

BTW, I adapted the code from

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

Lines 373 to 425 in667924c

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

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

	returnarray(
	'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
	returnarray(
	'class' =>is_object($controller[0]) ?get_class($controller[0]) :$controller[0],
	'method' =>$controller[1],
	'file' =>'n/a',
	'line' =>'n/a',
	);
	}
	}
	}

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

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

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

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

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

lyrixx force-pushed thehttp-kernel-exception-controller branch from31a63b9 to93ecc0bCompare

May 3, 2018 15:05

stof requested changes

May 3, 2018

View reviewed changes

src/Symfony/Component/HttpKernel/HttpKernel.php Outdated


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

Copy link

Member

stofMay 3, 2018

Choose a reason for hiding this comment

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

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.

Copy link

MemberAuthor

lyrixxMay 3, 2018

Choose a reason for hiding this comment

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

done

carsonbot added Status: Needs Work and removed Status: Needs Review labels

May 3, 2018

lyrixx force-pushed thehttp-kernel-exception-controller branch from93ecc0b to36dacd7Compare

May 3, 2018 15:24

nicolas-grekas reviewed

May 3, 2018

View reviewed changes

src/Symfony/Component/HttpKernel/HttpKernel.php Outdated

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

Copy link

Member

nicolas-grekasMay 3, 2018

Choose a reason for hiding this comment

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

trynew \ReflectionClass($controller[0])?

Copy link

MemberAuthor

lyrixxMay 3, 2018

Choose a reason for hiding this comment

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

To get only the file?

Copy link

Member

nicolas-grekasMay 3, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

lyrixxMay 3, 2018

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasMay 3, 2018

Choose a reason for hiding this comment

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

Looking at RequestDataCollector I'd say using__call?

Copy link

MemberAuthor

lyrixxMay 3, 2018

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekas commentedMay 3, 2018

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

nicolas-grekas added this to thenext milestone

May 3, 2018

lyrixx force-pushed thehttp-kernel-exception-controller branch 2 times, most recently from8c5d7b2 toc8f75e5Compare

May 4, 2018 12:15

Copy link

MemberAuthor

lyrixx commentedMay 4, 2018•
edited
Loading

@nicolas-grekas I added the new Exception class.

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

nicolas-grekas reviewed

May 4, 2018

View reviewed changes

src/Symfony/Component/HttpKernel/HttpKernel.php

		}
		thrownew \LogicException($msg);

		thrownewControllerDoesNotReturnResponseException($msg,$controller,__FILE__,__LINE__ -17);

Copy link

Member

nicolas-grekasMay 4, 2018

Choose a reason for hiding this comment

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

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

Copy link

MemberAuthor

lyrixxMay 7, 2018

Choose a reason for hiding this comment

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

Tests added

[HttpKernel] Better exception page when the controller returns nothing

f1e9aa4

lyrixx force-pushed thehttp-kernel-exception-controller branch fromc8f75e5 tof1e9aa4Compare

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

Copy link

Member

fabpot commentedMay 9, 2018

Thank you@lyrixx.

fabpot merged commitf1e9aa4 intosymfony:master

May 9, 2018

fabpot added a commit that referenced this pull request

May 9, 2018

feature#27138[HttpKernel] Better exception page when the controller…

d264fce

… 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

lyrixx deleted the http-kernel-exception-controller branch

May 9, 2018 08:31

nicolas-grekas removed this from thenext milestone

Nov 1, 2018

nicolas-grekas added this to the4.1 milestone

Nov 1, 2018

This was referencedNov 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

fabpot added a commit that referenced this pull request

Mar 5, 2019

feature#30408[HttpKernel] Better exception page when the invokable …

c877cf8

…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.yamlindex:    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

Labels

Feature HttpKernel Status: Reviewed

Movatterモバイル変換

Uh oh!

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

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

Uh oh!

Conversation

lyrixx commentedMay 3, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

nicolas-grekasMay 3, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

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!

lyrixxMay 3, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

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!

nicolas-grekasMay 3, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

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!

nicolas-grekas commentedMay 3, 2018

Uh oh!

lyrixx commentedMay 4, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commentedMay 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lyrixx commentedMay 3, 2018•
edited
Loading

nicolas-grekasMay 3, 2018•
edited
Loading

lyrixxMay 3, 2018•
edited
Loading

nicolas-grekasMay 3, 2018•
edited
Loading

lyrixx commentedMay 4, 2018•
edited
Loading