Skip to content
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

Refactor exception handling, rendering, and reporting #1641

Closed
tobyzerner opened this issue Nov 13, 2018 · 8 comments

Comments

@tobyzerner
Copy link
Member

commented Nov 13, 2018

From #1633 (review):

What I would like to see in beta.9:

  • One middleware (always enabled) that converts all known exception types with custom views to pretty error pages, always.
  • Another middleware (either Whoops or pretty pages) that a) displays the exceptions and b) logs them. (The logging could even be a separate middleware that re-throws the exceptions, unless that messes with the stack traces.)

IMO, there is no need for NotFound and similar exceptions to ever be handled with the fallback error handling (Whoops or pretty pages) - we are just using the exception mechanism to handle known error cases.

May be worth taking some inspiration from https://laravel.com/docs/5.7/errors

I would also like to see the ability to configure logging - we may as well make use of the power of Laravel's logging component.


Step-by-step TODO:

  • Return proper status codes for Whoops / debug mode
  • Token error message - notes from PR (#1528)
  • Pretty error pages should differ based on exception type, not status code (e.g. CSRF token mismatch)
  • Exceptions should expose whether they are loggable / reportable (see FriendsOfFlarum/sentry@8fc6928#r34287868)

@tobyzerner tobyzerner added this to the 0.1.0-beta.9 milestone Nov 13, 2018

@luceos

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

One thing I think is a must-have is the ability to easily enable Sentry. This would especially be helpful for taking immediate action when encountering unexpected errors on discuss. I will gladly provide an extension if need be.

@franzliedke

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

I'd be most interested in your opinion on this:

IMO, there is no need for NotFound and similar exceptions to ever be handled with the fallback error handling (Whoops or pretty pages) - we are just using the exception mechanism to handle known error cases.

@luceos

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

Catching NotFound exceptions allows for customising the returned response, eg with a decent error page or through extensions to list related discussions. Unless of course an extension could be configured to handle an uncaught exception (add to the handler stack, I think this is currently the case).

I would be very interested to know whether we can create decent error pages for any HttpException (with 500 probably being the 'exception').

@franzliedke

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

@luceos Yes, I want to keep that part. My question was about the Whoops error page, which is currently shown (in debug mode) for NotFound exceptions and the like. IMO, these exceptions should always be handled by the error handler stack.

Debug mode (a.k.a. Whoops error pages) would then only have an effect for generic 500 errors.

@luceos

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

My question was about the Whoops error page, which is currently shown (in debug mode) for NotFound exceptions and the like. IMO, these exceptions should always be handled by the error handler stack

When in debug mode you should always be able to get as much information as possible immediately (without diving into logs). I can imagine it would be easier to identify the cause of an incorrect 404 with whoops enabled for those kind of exceptions. Such exceptions can not only be caused by a firstOrFail but also by middleware, global scopes or random abort() calls.

@datitisev

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

If Whoops was kept for non-500 errors, which I agree with @luceos in that it would help debug, IMO it should still return the error code as the HTTP status.
Currently it only returns 500, which makes 404 errors look like server errors and can scare me a bit sometimes 😂

@franzliedke

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

If Whoops was kept for non-500 errors, which I agree with @luceos in that it would help debug, IMO it should still return the error code as the HTTP status.

That is a great suggestion and would alleviate most of my concerns.

@franzliedke

This comment has been minimized.

Copy link
Member

commented Jul 7, 2019

Now that #1648 is merged, what's left here is a rethink of how logging, exception handling and exception rendering fit together.

This also raises the question of how the new CSRF token mismatch handling fits in - generating the same status code from different exceptions is now possible, which means that we probably want to vary the returned pretty view based on the exception type, not the status code.

franzliedke referenced this issue in FriendsOfFlarum/sentry Jul 12, 2019

@franzliedke franzliedke referenced this issue Aug 9, 2019
1 of 2 tasks complete

franzliedke added a commit to flarum/lang-english that referenced this issue Aug 14, 2019

Update error keys to match based on exception type
...instead of status code. There are (or will be) multiple different
keys for similar errors with the same status code. In the future, we
will use the error's "type" (see flarum/core#1843) to distinguish them.

Refs flarum/core#1641.

franzliedke added a commit that referenced this issue Aug 20, 2019

@franzliedke franzliedke referenced this issue Aug 20, 2019
1 of 1 task complete

franzliedke added a commit to flarum/lang-english that referenced this issue Aug 21, 2019

franzliedke added a commit that referenced this issue Aug 21, 2019

Remove superfluous ForbiddenException
It has the same effect as the PermissionDeniedException, so let's
just use that.

Refs #1641.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.