-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Add HttpErrorCodeInterface. #18110
Add HttpErrorCodeInterface. #18110
Conversation
e740154
to
de5e52a
Compare
|
||
use Throwable; | ||
|
||
interface HttpErrorCodeInterface extends Throwable |
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.
Ideally this should have been in the Http
package but couple of exceptions of the Datasource
package are also mapped to 404 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.
I think this is ok. Hard to create a generic map with one way dependencies with our split packages.
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.
Do we get much from extending Throwable
?
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.
Not really, I did it as this interface is only meant to be used by exception classes.
The static analysis job failure is because the composer.json of split packages use cakephp 5.1. |
We should be using dev branches I guess |
de5e52a
to
3e97a14
Compare
Even that doesn't help, since the new interface file is in this PR. The error will be gone only after the PR is merged. |
* @var array<string, int> | ||
* @psalm-var array<class-string<\Throwable>, int> | ||
*/ | ||
protected array $exceptionHttpCodes = [ |
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.
We could end up breaking userland exception rendering with this change. If we wanted to avoid the breakage, we could retain this as an empty list and also retain line 408 as a fallback for exceptions that don't implement HttpErrorCodeInterface
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.
Okay, I'll add it back for BC.
|
||
use Throwable; | ||
|
||
interface HttpErrorCodeInterface extends Throwable |
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.
Do we get much from extending Throwable
?
3e97a14
to
1e2087f
Compare
Using this interface allows exception classes to indicate that they use HTTP error codes and avoid having to maintain an error class to HTTP error code map in WebExceptionRenderer.
1e2087f
to
d5d72da
Compare
Looks good to me. The split package phpstan builds are failling. Those builds might clear up after we do a release though. |
Using this interface allows exception classes to indicate that they use HTTP error codes and avoid having to maintain an error class to HTTP error code map in WebExceptionRenderer.