Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Deprecate :after-handler value for show_exceptions #884

Open
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Contributor

huoxito commented Jun 14, 2014

It's very confusing for new users as they will write custom handlers
with error(*codes, &:block) but those would never run in development
because handle_exception! was raising a general exception before
checking for custom errors.

I believe the only advantage for that would be to display stack traces
on the browser for users but I think most of the times users are catching
custom errors they don't care much about the stack trace or even if they
care they can always raise the exception again

It's been a couple months I've tried to use sinatra built in error handler but would always drop it because I thought it didn't work till today when I took a closer look. I see README mentions the :after_handler option but it was not clear to me at all that I had to set that in order to make my error handler run in development. Initially I thought of submitting a README change to mention that here https://github.com/sinatra/sinatra#error-handling but after reading the code decided to take a chance on dropping that config option value and hear what you guys think?

In case you have strong options against it I can submit another PR to mention about the show_exceptions, :after_handler value on the error-handling section.

@huoxito huoxito Deprecate :after-handler value for show_exceptions
It's very confusing for new users as they will write custom handlers
with `error(*codes, &:block)` but those would never run in development
because `handle_exception!` was raising a general exception before
checking for custom errors.

I believe the only advantage for that would be to display stack traces
on the browser for users but I think most of the times users are catching
custom errors they don't care much about the stack trace or even if they
care they can always raise the exception again
53e99df
Owner

rkh commented Jun 17, 2014

The problem is that his would be a behaviour change in a minor release.

Contributor

huoxito commented Jun 17, 2014

@rkh I see, looks a valid concern to me. What about merging only once you guys decide sinatra 2.0 is coming next (whenever that is)? I could continue enhancements on this PR and submit another one to mention about the :after_handler option for error handlers in development.

I think there's at least one other issue in handler_exception. A general error handler, e.g. error { } would never run in development even if you set up show_exception to after_handler.

Owner

rkh commented Jun 17, 2014

👍 I would like the error page to become an exception handler rather than a middleware in 2.0.

@zzak zzak added the feature label Feb 6, 2015

Contributor

huoxito commented Jun 15, 2015

been a long while and just saw this one again

@rkh @zzak you think this is still worth it or you have other plans for this config? @rkh could you elaborate a bit more on your last comment pls? happy to help move this forward

Owner

zzak commented May 9, 2016

In order to move this forward, we need a way to support both :after_handler and the exception handler refactor.

Regarding rkh's comment, I believe his goal is to pull the error page out of the middleware stack so it can be removed entirely from production setups.

@zzak zzak added this to the Beyond milestone May 9, 2016

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