Skip to content

Loading…

Controller should be responsible for choice to show exceptions #3323

Closed
jeremy opened this Issue · 6 comments

2 participants

@jeremy
Ruby on Rails member

In Rails 2 we could override local_request? on the controller, so you had access to a wealth of info to decide whether to show exceptions like the current account, current user, etc.

In Rails 3 that moved to a middleware and the local_request? check moved to the Request itself, which can't see shit.

It'd be great to have a better way to tell ShowExceptions to actually show the exception. It's a patchwork of constructor args, rack env vars, and Request#local? right now. It shouldn't have to know about any of that except a rack env var, ideally.

Leave it up to the controller to tell it whether to kick in or not.

Example: on every request, set env['action_dispatch.show_exceptions'] = show_exceptions? and wrap up the typical local-request checks in a default #show_exceptions? that controllers can override and super to.

Targeting 3.2 for a fix. Pull requests welcome!

@jeremy jeremy was assigned
@lest

@jeremy, there are 3 ways in ShowExceptions:

  • raise exception (now controlled through env['action_dispatch.show_exceptions'])
  • rescue locally
  • rescue in public

Should we somehow change behaviour of env['action_dispatch.show_exceptions'] or add another one (e.g. env['action_dispatch.rescue_action_locally']) ?

@lest

I'll provide pull request for it.

@jeremy
Ruby on Rails member

@lest Thanks. It may help to split these into separate middleware, too.

@lest

@jeremy I've looked at sinatra sources (it has the same functionalily) and decided:

  • use action_dispatch.raise_errors to raise exceptions or not
  • use action_dispatch.show_exceptions to render detailed diagnostics for exception (as in your example)

PS. I've almost done this and added show_exceptions? method to ActionController::Metal (is it ok?), I have to reorganize tests and test it carefully.

@lest

Please see pull request #3717

@jeremy
Ruby on Rails member

Closed by #3717

@jeremy jeremy closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.