-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
fix: exceptionHandler may return invalid HTTP status code #6228
fix: exceptionHandler may return invalid HTTP status code #6228
Conversation
I have doubts about the implementation. Something like this $statusCode = 500;
$exitStatus = EXIT_ERROR;
if (
$exception instanceof HttpException
&& $exception->getCode() >= 400
&& $exception->getCode() < 500
) {
$statusCode = $exception->getCode();
} There is no point in additional interfaces. |
Yes. This is global exception handler, so it should return 500 Response for all cases. |
Just a note that HTTP exceptions actually include 300s as well via |
There is
|
CodeIgniter4/system/CodeIgniter.php Lines 350 to 359 in bc688d9
|
I think this PR is ready for reviews. |
d414c1e
to
0581338
Compare
Why not make |
Actually I just read this:
So maybe the exception type is what defines these two special cases and we don't want more robust handling. |
404 is handled in the exceptionHandler() now. |
I've changed my mind. I sent another PR #6254 with minimum changes for the bug fix. |
With the merge of #6254 , is this still needed? |
Yes, #6254 is a workaround. I think the current behavior is broken. Because Exception code has nothing to do with HTTP status code/Exit code. |
a2277d7
to
e62b283
Compare
e62b283
to
c7bee50
Compare
It seems to me that it is impossible to use Exception code to represent Exit code. I feel it would be better to have |
c7bee50
to
dc51f1e
Compare
I renamed |
9468981
to
3bac9cb
Compare
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.
Good by me!
3bac9cb
to
5358b14
Compare
Added the docs. |
5358b14
to
cb1b570
Compare
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.
Looks good! Some proofing suggestions.
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Co-authored-by: MGatner <mgatner@icloud.com>
Description
The current implementation has undocumented behavior that use Exception code as HTTP status code,
and determine Exit code from Exception code.
But Exception code has nothing to do with HTTP status code or Exit code.
This PR introduces interfaces that explicitly show the Exception code is HTTP status code or Exit code
HTTPExceptionInterface
andHasExitCodeInterface
Exceptions::determineCodes()
Breaking Changes:
In that case, you need to implements
HTTPExceptionInterface
in the Exception.In that case, you need to implements
HasExitCodeInterface
in the Exception.Related: #6286
Checklist: