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

[HttpKernel][Debug] Get exception content according to request format#30851

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

Closed

yceruto wants to merge1 commit intosymfony:masterfromyceruto:exception_response_format

Closed

[HttpKernel][Debug] Get exception content according to request format#30851

yceruto wants to merge1 commit intosymfony:masterfromyceruto:exception_response_format

Conversation

Copy link

Member

yceruto commentedApr 3, 2019•
edited
Loading

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

Mainly for API-based apps that don't require TwigBundle to get the correct exception response according to the request format (aka_format attribute).

Let me know if this classify as a bugfix for 4.2, but IMHO this was never supported without TwigBundle, hencemaster branch.

Hide trace info for non-debug mode
Update according to RFChttps://tools.ietf.org/html/rfc7807 (JSON, XML)

thrownewNotFoundHttpException('Resource not found.',null,0, ['Content-Type' =>'application/problem+json']);

carsonbot added Status: Needs Review HttpKernel Debug Feature labels

Apr 3, 2019

yceruto mentioned this pull request

Apr 3, 2019

Symfony4 is not returning exceptions as JSON, even with _format: json explicitly defined.#25905

Closed

Copy link

Member

fabpot commentedApr 3, 2019

You're right, this is for master.

yceruto force-pushed theexception_response_format branch from3d66ef3 toaa6bc3dCompare

April 3, 2019 14:47

Copy link

Member

nicolas-grekas commentedApr 3, 2019

Do we follow any RFC regarding the formatting of the json/xml/etc payloads?
If not, what about doing it? See#30559

nicolas-grekas added this to thenext milestone

Apr 3, 2019

Copy link

MemberAuthor

yceruto commentedApr 3, 2019•
edited
Loading

Do we follow any RFC regarding the formatting of the json/xml/etc payloads?

Not really, I just copied the content format from TwigBundle exceptions.

If not, what about doing it? See#30559

Good idea! I'll check the RFC for each response format.

Later, I guess we should update the TwigBundle errors/exceptions as well.

Copy link

MemberAuthor

yceruto commentedApr 3, 2019

Status: Needs Work

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

Apr 3, 2019

yceruto force-pushed theexception_response_format branch fromaa6bc3d to94167b0Compare

April 3, 2019 18:36

Copy link

MemberAuthor

yceruto commentedApr 3, 2019

Status: Needs Review

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

Apr 3, 2019

Get exception content according to request format

259fa20

yceruto force-pushed theexception_response_format branch from94167b0 to259fa20Compare

April 3, 2019 19:22

Copy link

MemberAuthor

yceruto commentedApr 4, 2019

This is ready so far, because we don't have more info from HttpException, but if we wanted to complete the spec details...

https://tools.ietf.org/html/rfc7807#section-3
The ability to convey problem-specific extensions allows more than one problem to be conveyed.

... we might need a meta argument containing standard (e.g.type,instance) and/or non-standard (e.g.balance,invalid-params, etc) meta-information about the error.

Ideally something like this:

thrownewProblemDetailsHttpException(400,'Your request parameters didn't validate.', null, ['Content-Type' =>'application/problem+json'],0, ['type' =>'https://example.net/validation-error','invalid-params' => [         ['name' =>'age','reason' =>'must be a positive integer'        ],        ['name' =>'color','reason' =>'must be green, red or blue'        ],    ],]);

then, the last argument (named e.g. meta) could be handled byFlattenException and merge it into the final formatted content. WDYT?

carsonbot removed the Status: Needs Review label

Apr 4, 2019

nicolas-grekas approved these changes

Apr 4, 2019

View reviewed changes

Copy link

Member

nicolas-grekas left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Nice, thanks.
About your last comment, I wouldn't go further for a generic Debug component - for now at least.

carsonbot added the Status: Reviewed label

Apr 4, 2019

dmaicher reviewed

Apr 4, 2019

View reviewed changes

Copy link

Contributor

dmaicher left a comment

Choose a reason for hiding this comment

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

👍

src/Symfony/Component/Debug/ExceptionHandler.php

		}elseif (class_exists(Response::class) &&isset(Response::$statusTexts[$statusCode])) {
		$title = Response::$statusTexts[$statusCode];
		}else {
		$title ='Internal Server Error';

Copy link

Contributor

Choose a reason for hiding this comment

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

so a 403 error withoutsymfony/http-foundation installed would lead to this title/message? Might be a bit confusing 😄

src/Symfony/Component/Debug/ExceptionHandler.php

		$exception = FlattenException::create($exception);
		}

		if (404 ===$statusCode =$exception->getStatusCode()) {

Copy link

Contributor

Choose a reason for hiding this comment

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

this blocks seems repeated for all content-types? maybe move it to a private method?

Copy link

Member

fabpot commentedApr 4, 2019

@yceruto Thank you for working on this topic, which we should tackle. But I don't think this is the right approach. Let's talk about this this week-end during the Hackaton.

Copy link

MemberAuthor

yceruto commentedApr 6, 2019

@fabpot then I guess should I close here, right?

Copy link

Member

fabpot commentedApr 6, 2019

No, we should talk first

Copy link

MemberAuthor

As discussed, this code should be moved to a new component (maybeErrorException) that could be used in both "prod" (where a Debug component doesn't make sense) and dev as hard dep of theHttpKernel component.

Later, theExceptionListener can use it and also the Debug req can be moved torequire-dev section in the future.