-
Notifications
You must be signed in to change notification settings - Fork 698
Throw user-friendly exception with the proper HTTP statuscode #2234
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## simplesamlphp-2.3 #2234 +/- ##
=======================================================
+ Coverage 44.90% 44.92% +0.01%
- Complexity 3895 3898 +3
=======================================================
Files 162 163 +1
Lines 12982 12998 +16
=======================================================
+ Hits 5830 5839 +9
- Misses 7152 7159 +7 |
|
Note to self: should fix the debug info box overflow |
thijskh
left a comment
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.
It makes it a lot friendlier, but I think the core issue remains and that is that OPTIONS and HEAD requests will still be logging exceptions and I doubt we want that. You get a lot of OPTIONS passing by from browsers etc that should just be silently responded to with a 405 but not generate a lot of logging. I think.
|
I'm not sure we can fix that entirely... One possible solution could be to add a parameter to |
20cbae4 to
ccf9cb8
Compare
|
I would only do this in debug mode indeed. Because it's very common for browsers etc to send OPTIONS requests which is spec compliant to do and the server just needs to respond with 405. Generating higher loglevel entries for this common and normal behaviour obscures the actual exceptions. |
|
So your OK with the changes I made last night? |
|
The changes seem to be about the email form? Or am I misunderstanding it. The browsers sending legitimate OPTIONS requests will not be submitting the email form anyway... |
|
The email form would still be generated and produce output to the logs. The OPTIONS request would still lead to: The end-result now is that these requests do not log anything at all, unless debug-level is set. |
823302f to
441b72e
Compare
monkeyiq
left a comment
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 see that in Error::show the old direct call to logError() is now moved to log($logLevel) which will dispatch the logBacktrace() call based on the level that is passed and the system settings.
Just a thinking out loud took a closer look comment :)
Yes, that's the trick to downgrade exceptions. Any exception we want to downgrade to debug-level can be dealt with this way in |
* Throw user-friendly exception with the proper HTTP statuscode for 'method not found' * Suppress traceback for MethodNotAllowed error, unless debug-logging is set * Be able to suppress error reporting for specific types of errors
* Throw user-friendly exception with the proper HTTP statuscode for 'method not found' * Suppress traceback for MethodNotAllowed error, unless debug-logging is set * Be able to suppress error reporting for specific types of errors
* Throw user-friendly exception with the proper HTTP statuscode for 'method not found' * Suppress traceback for MethodNotAllowed error, unless debug-logging is set * Be able to suppress error reporting for specific types of errors
* Throw user-friendly exception with the proper HTTP statuscode for 'method not found' * Suppress traceback for MethodNotAllowed error, unless debug-logging is set * Be able to suppress error reporting for specific types of errors

Closes #2233