Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

yceruto
Copy link
Member

@yceruto yceruto commented Apr 3, 2019

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

exception_response

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

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

@fabpot
Copy link
Member

fabpot commented Apr 3, 2019

You're right, this is for master.

@yceruto yceruto force-pushed the exception_response_format branch from 3d66ef3 to aa6bc3d Compare April 3, 2019 14:47
@nicolas-grekas
Copy link
Member

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

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 3, 2019
@yceruto
Copy link
Member Author

yceruto commented Apr 3, 2019

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.

@yceruto
Copy link
Member Author

yceruto commented Apr 3, 2019

Status: Needs Work

@yceruto
Copy link
Member Author

yceruto commented Apr 3, 2019

Status: Needs Review

@yceruto yceruto force-pushed the exception_response_format branch from 94167b0 to 259fa20 Compare April 3, 2019 19:22
@yceruto
Copy link
Member Author

yceruto commented Apr 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:

throw new ProblemDetailsHttpException(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 by FlattenException and merge it into the final formatted content. WDYT?

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

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.

Copy link
Contributor

@dmaicher dmaicher left a comment

Choose a reason for hiding this comment

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

👍

} 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 without symfony/http-foundation installed would lead to this title/message? Might be a bit confusing 😄

$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?

@fabpot
Copy link
Member

fabpot commented Apr 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.

@yceruto
Copy link
Member Author

yceruto commented Apr 6, 2019

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

@fabpot
Copy link
Member

fabpot commented Apr 6, 2019

No, we should talk first

@yceruto
Copy link
Member Author

yceruto commented Apr 6, 2019

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

Later, the ExceptionListener can use it and also the Debug req can be moved to require-dev section in the future.

@yceruto
Copy link
Member Author

yceruto commented Apr 10, 2019

See #31065

@yceruto yceruto deleted the exception_response_format branch April 16, 2019 14:58
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants