-
-
Notifications
You must be signed in to change notification settings - Fork 191
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
Interoperability issue with 500 response code caused by ApplicationException #1106
Comments
It's not Winter's responsibility to care about what proxies are doing to the response. In the provided example of a repeater's minimum items not being reached then yes, that would probably be more correct to be handled with a ValidationException instead, but that's up to the developer writing the logic to decide which exception is most relevant. Do you have a suggestion for more relevant status codes / exceptions to use instead of 500 & an ApplicationException? The ApplicationException is intended for when the application cannot proceed with processing, but it's not because of a coding error or something that should be fixed by a developer. |
An application doesn't operate in a vacuum, there are 3rd parties and they all rely on some common standards that Web has to offer. When an app doesn't adhere to them for a good reason then it's hard to blame it, however when there's no good reason to give a 500 response for an error which is not fatal, then this behaviour should be changed. It's not only about the environment, it's about providing a logically correct behaviour.
I would like to know what led to that decision. On the surface it looks like the developer took an opportunity to come up with a quick error handling mechanism which is already there, and just inherited the specifics of it. But this issue is raised exactly for this reason - many developers may want to use it so more pressure is put on making it right and compatible.
Right, this class of errors in HTTP terms fall into 4xx category - client error (as opposed to 5xx when server fails to fulfil an apparently valid [i.e. inputs are correct and match the context] request). I did some custom implementation by redefining ErrorHandler via |
I wouldn't be opposed to a PR to specify the HTTP error code to be used along with providing the most common error codes as constants on the class because sometimes a 422 is more relevant.
I would argue that an ApplicationException is fatal. Unlike a stateful server application, the ApplicationException might not indicate that things are now permanently broken, but for the purposes of the request being made when the ApplicationException is thrown the code is saying "I've reached a state where I cannot help you anymore; here's why". What changes would you like to propose? I'm not opposed to an audit of the existing uses of ApplicationException vs SystemException in the core and improving our documentation / developer education resources to better communicate when each exception type should be used, we just need to figure out what changes specifically we want to make and then make them :) |
Winter CMS Build
dev-develop
PHP Version
8.1
Database engine
MySQL/MariaDB
Plugins installed
No response
Issue description
In certain setups that rely on a frontend web proxy (like CloudFront or CloudFlare) to deliver HTML generated by a Winter CMS application, it is possible for the proxy to intercept 500 response which is considered an application error. These proxies typically will show a custom HTML page when a 500 response is received, and for a good reason - not all applications behave correctly and are able to show a meaningful error page when an internal server error occurs.
Winter CMS has a mechanism based on throwing ApplicationException in the application in order for some admin UI JavaScript handlers to produce a non-browser-native alert (popup) with the error message matching the string used in ApplicationException constructor. This is simple an efficient in case a quick error message not related to any form is needed. Unfortunately, it doesn't work well with the above web proxy logic - the 500 response's message is ignored and replaced with an error page's HTML.
It is not always possible to override the proxy's behaviour. Also, logically, it's not clear why ApplicationException which is meant to be a quick (and yes, dirty) way of delivering a message even for normal error cases (like a field repeater's minimum number of items not reached), relies on code 500. If the server is able to produce a response that has a meaning then it's not an internal server error, but some kind of client error (4xx), For example, validation errors produce 416, so why an ApplicationException sticks to 500?
Steps to replicate
Expected result: an popup shows whatever message was passed as ApplicationException constructor argument
Actual result: the proxy thinks the app is defunct and substitutes the response with the custom 500 page.
Workaround
No response
The text was updated successfully, but these errors were encountered: