Improve error handling #118

Closed
tobscure opened this Issue Jun 20, 2015 · 10 comments

Comments

Projects
None yet
2 participants
@tobscure
Member

tobscure commented Jun 20, 2015

Forum backend

  • Load Whoops vs. a custom error handler middleware depending on whether debug config setting is on or off
  • Design custom error pages - one for 404, one for 500
  • Build said custom error handler to render appropriate said custom error page

API

  • Load Whoops vs. a custom error handler middleware depending on whether debug config setting is on or off
  • Build said custom error handler to output errors in JSON-API spec format

Forum frontend

  • Design and handle error messages for empty sections (i.e. no discussion list results, no activity in a user's profile)
  • Design and handle error page for discussion not found, user not found, route not found, etc.
  • Better handling of general AJAX request errors (gracefully fail on unparseable JSON, timeouts, disconnect)
  • Better handling of API errors (make sure all AJAX requests have an appropriate error handler, this would be handy)
  • Nicer error message when the client fails to load due to a JS error
@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Jun 20, 2015

Member

I've taken care of only enabling whoops in debug mode. I also built error handlers for the frontend and the API. The frontend error pages are rather plain - I'll let you work your design magic on them, @tobscure. Maybe you're also more creative than me in making them unique and fun.

One thing I'm not sure about right now is the following: for some errors like validation errors etc., we probably do not want to show Whoops, even when in debug mode, right? (I guess it depends on how AJAX errors are handled exactly in the frontend.) Any good idea how we could take care of those cases?

Member

franzliedke commented Jun 20, 2015

I've taken care of only enabling whoops in debug mode. I also built error handlers for the frontend and the API. The frontend error pages are rather plain - I'll let you work your design magic on them, @tobscure. Maybe you're also more creative than me in making them unique and fun.

One thing I'm not sure about right now is the following: for some errors like validation errors etc., we probably do not want to show Whoops, even when in debug mode, right? (I guess it depends on how AJAX errors are handled exactly in the frontend.) Any good idea how we could take care of those cases?

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Jun 20, 2015

Member

Currently if the front-end doesn't receive JSON, it basically crashes the JS app. I will wrap the JSON parsing code with a try/catch so that it doesn't, and instead show a little red alert in the bottom left. So I think in debug mode any abnormal exception can trigger whoops and then you can use the browser inspector to see the error details.

But you're completely right about not wanting some errors (validation in particular) to be handled with whoops. We actually already have code in JsonApiAction@handle to catch some of those and output JSON-API spec errors. I think this could all be moved into a new error handler middleware. And if debug is not enabled, this needs to catch all exceptions and output a generic JSON-API spec error too.

Edit: the latter of which I see you've done! Nice!

Member

tobscure commented Jun 20, 2015

Currently if the front-end doesn't receive JSON, it basically crashes the JS app. I will wrap the JSON parsing code with a try/catch so that it doesn't, and instead show a little red alert in the bottom left. So I think in debug mode any abnormal exception can trigger whoops and then you can use the browser inspector to see the error details.

But you're completely right about not wanting some errors (validation in particular) to be handled with whoops. We actually already have code in JsonApiAction@handle to catch some of those and output JSON-API spec errors. I think this could all be moved into a new error handler middleware. And if debug is not enabled, this needs to catch all exceptions and output a generic JSON-API spec error too.

Edit: the latter of which I see you've done! Nice!

@tobscure tobscure added this to the 1.0 Beta 1 milestone Jun 25, 2015

@tobscure tobscure modified the milestone: 1.0 Beta 1 Jul 30, 2015

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Aug 27, 2015

Member

See #252 re: error pages

Member

tobscure commented Aug 27, 2015

See #252 re: error pages

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Aug 27, 2015

Member

What needs to be done:

  • Backend first. In accordance with the JSON-API spec, we need to make sure we're returning the right error responses when things go wrong on the backend.
    • We should probably set up some base Exception classes that Actions/Commands can throw which will then be translated into the correct JSON-API response.
  • Once that's good, we can tweak the front-end to handle these error responses in all instances.
    • Ideally we want a mechanism that allows callers of app.request to handle any error, but if they don't, it falls back to a default handler.
Member

tobscure commented Aug 27, 2015

What needs to be done:

  • Backend first. In accordance with the JSON-API spec, we need to make sure we're returning the right error responses when things go wrong on the backend.
    • We should probably set up some base Exception classes that Actions/Commands can throw which will then be translated into the correct JSON-API response.
  • Once that's good, we can tweak the front-end to handle these error responses in all instances.
    • Ideally we want a mechanism that allows callers of app.request to handle any error, but if they don't, it falls back to a default handler.
@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Sep 9, 2015

Member

This should also be an exception, right? @tobscure

Member

franzliedke commented Sep 9, 2015

This should also be an exception, right? @tobscure

franzliedke added a commit that referenced this issue Sep 9, 2015

franzliedke added a commit to flarum/flarum-ext-pusher that referenced this issue Sep 9, 2015

franzliedke added a commit to flarum/flarum-ext-tags that referenced this issue Sep 9, 2015

@tobscure

This comment has been minimized.

Show comment
Hide comment
Member

tobscure commented Sep 9, 2015

tobscure added a commit that referenced this issue Oct 20, 2015

Improve client XHR error handling
The default XHR error handler produce an alert which is appropriate to the response status code. It can be overridden per-request (by specifying the `errorHandler` option) so that the alert can be suppressed or displayed in a different position (e.g. inside a modal).

ref #118

@tobscure tobscure referenced this issue Oct 21, 2015

Open

Design nicer error/placeholder pages #596

1 of 7 tasks complete
@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Oct 21, 2015

Member

Closing this for beta 3. The rest is design stuff (polish) which isn't as high priority: #596

Member

tobscure commented Oct 21, 2015

Closing this for beta 3. The rest is design stuff (polish) which isn't as high priority: #596

@tobscure tobscure closed this Oct 21, 2015

@franzliedke

This comment has been minimized.

Show comment
Hide comment
@franzliedke

franzliedke Oct 21, 2015

Member

What about these three issues that were merged into this one?

At least the third one doesn't look like it's already fixed...

Member

franzliedke commented Oct 21, 2015

What about these three issues that were merged into this one?

At least the third one doesn't look like it's already fixed...

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Oct 21, 2015

Member

True, will take a look at them tomorrow.

Member

tobscure commented Oct 21, 2015

True, will take a look at them tomorrow.

tobscure added a commit that referenced this issue Oct 22, 2015

@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Oct 22, 2015

Member

#165 is fixed, #220 is fixed, and #552 will be tackled with #596.

Member

tobscure commented Oct 22, 2015

#165 is fixed, #220 is fixed, and #552 will be tackled with #596.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment