-
Notifications
You must be signed in to change notification settings - Fork 11k
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
[5.5] Ensure uncaught exceptions are returned in JSON if required #18732
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment. The security, the exception code, the message translation is important.
'message' => $e->getMessage(), | ||
'file' => $e->getFile(), | ||
'line' => $e->getLine(), | ||
'trace' => $e->getTrace(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't think we should response a different json in debug mode and the production mode.
Further more, the exception code (getCode()) may have much more valuable.
May be we should just hide the trace for some security consideration. Also we should consider the translation for the message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't think we should response a different json in debug mode and the production mode.
Taylor already added JSON stuff for debug in the master branch. This PR is not adding it.
We are just putting JSON for production when it was requested via AJAX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should just hide the trace in production. Or replace it with the exception's getCode. I think the translation for the message is really important. Could you add these code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Translation of exceptions should probably be a different PR though - as that is a separate to what is occurring in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but it is relate these code, about the exception. It doesn't matter how the code work. Please consider the exception code, the translation. The file name and the exception line shown in production may not be safe for the server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file name and the exception line shown in production may not be safe for the server.
Correct - we only return the detailed information if debug
is true.
I looked around - there dont seem to be any framework tests for the Exceptions Handler? I've added some tests to start some coverage, at least for these changes. |
@m1guelpf - looks like we both created the PRs at similar times. This PR is a different approach and takes into account the upcoming changes in |
This is something I have to add to every Laravel app I write that has an API. It would be nice for something like this to be In core. |
'trace' => $e->getTrace(), | ||
]; | ||
} else { | ||
$response['message'] = $this->isHttpException($e) ? $e->getMessage() : 'Server Error'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this to be clearer:
$response = [
'message' => $this->isHttpException($e) ? $e->getMessage() : 'Server Error',
];
...since we're only now creating the response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JosephSilber - yeah, I was wondering about that. I've pushed an update. Thanks.
return $request->expectsJson() && config('app.debug') | ||
? $this->prepareJsonResponse($request, $e) | ||
: $this->prepareResponse($request, $e); | ||
return $request->expectsJson() ? $this->prepareJsonResponse($request, $e) : $this->prepareResponse($request, $e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave this on multiple lines how it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GrahamCampbell - ok - fixed
]; | ||
} | ||
|
||
return response(json_encode($response, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES), $status, array_merge($headers, [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we actually can only return JSON here, why not just:
return new JsonResponse($response, $status, $headers, JSON_PRETTY_PRINT | JSON_UNESCAPED_SLASHES);
@return
doblock should be changed accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasmichot - that line of code is basically what was already in the master branch. But I agree your way seems cleaner - so I've updated the PR.
@@ -212,22 +213,28 @@ protected function prepareResponse($request, Exception $e) | |||
* | |||
* @param \Illuminate\Http\Request $request | |||
* @param \Exception $e | |||
* @return \Symfony\Component\HttpFoundation\Response | |||
* @return \Illuminate\Http\JsonResponse | |||
*/ | |||
protected function prepareJsonResponse($request, Exception $e) | |||
{ | |||
$status = $this->isHttpException($e) ? $e->getStatusCode() : 500; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe only one condition could be used here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasmichot - I'm not sure what do you mean? If it is a HttpException - we will use the exception code we were given (i.e. 403
). But if it is not a HttpException - then we need to cast it as a 500
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that we can actually make this check only once:
if ($this->isHttpException($e)) {
$status = $e->getStatusCode();
$headers = $e->getHeaders();
} else {
$status = 500;
$headers = [];
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up to Taylor I guess? Those lines were what he wrote already in the master branch that currently exists. I'll leave it as is for this PR to keep changes to a minimum and we can review after?
Hello, |
You could override the exception handler - yes |
The current framework behavior is to return full html in uncaught exceptions (such as a 500 internal error).
In
5.5/master
there are already some changes which will forcibly return JSON ifexpectsJSON()
istrue
- but only ifapp.debug = true
.I propose that if
expectsJSON()
istrue
- then we should always be returning JSON regardless of the debug setting.