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

Improve error handling #118

Closed
7 of 10 tasks
tobyzerner opened this issue Jun 20, 2015 · 10 comments
Closed
7 of 10 tasks

Improve error handling #118

tobyzerner opened this issue Jun 20, 2015 · 10 comments
Assignees
Milestone

Comments

@tobyzerner
Copy link
Contributor

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
Copy link
Contributor

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?

@tobyzerner
Copy link
Contributor Author

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!

@tobyzerner tobyzerner added this to the 1.0 Beta 1 milestone Jun 25, 2015
@tobyzerner tobyzerner modified the milestone: 1.0 Beta 1 Jul 30, 2015
@tobyzerner
Copy link
Contributor Author

See #252 re: error pages

@tobyzerner
Copy link
Contributor Author

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
Copy link
Contributor

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/pusher that referenced this issue Sep 9, 2015
franzliedke added a commit to flarum/tags that referenced this issue Sep 9, 2015
@tobyzerner
Copy link
Contributor Author

@franzliedke yep.

tobyzerner added a commit that referenced this issue Oct 20, 2015
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
@tobyzerner
Copy link
Contributor Author

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

@franzliedke
Copy link
Contributor

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

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

@tobyzerner
Copy link
Contributor Author

True, will take a look at them tomorrow.

tobyzerner added a commit that referenced this issue Oct 22, 2015
@tobyzerner
Copy link
Contributor Author

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

askvortsov1 pushed a commit to flarum/tags that referenced this issue Mar 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants