Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Make route params available during error handler (Fixes #860) #895

Open
wants to merge 1 commit into from

4 participants

@jeremyevans

This is kind of ugly, but I couldn't see a better way. One
potential issue is that the splat and captures route params
will be overwritten by the error handler. There's no way to
change that without breaking backwards compatibility. We
could potentially offer the route splat/captures under
different param names, though.

@jeremyevans jeremyevans Make route params available during error handler (Fixes #860)
This is kind of ugly, but I couldn't see a better way.  One
potential issue is that the splat and captures route params
will be overwritten by the error handler.  There's no way to
change that without breaking backwards compatibility.  We
could potentially offer the route splat/captures under
different param names, though.
10cc608
@rkh
Owner

I think I am in favour of this patch. Any opinions @kytrinyx, @zzak? Should the same behaviour be adapted for pattern-less before/after blocks?

@kytrinyx
Collaborator

I'm inclined to agree. It would be really nice to have those handy at that time!

What would be the use-case for having them in the pattern-less before/after blocks?

@rkh
Owner

What would be the use-case for having them in the pattern-less before/after blocks?

Personally, I've never run into a use case, since then I could usually use a pattern with the filter. People seem to assume it should work, see for instance #417. However, after giving this some thought, I don't think we should do it, since it would only work for after filters.

@kytrinyx
Collaborator

Agreed, let's not go down that path. I'm happy with this PR as is.

@rkh rkh added this to the 1.5.0 milestone
@zzak zzak added the feature label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 27, 2014
  1. @jeremyevans

    Make route params available during error handler (Fixes #860)

    jeremyevans authored
    This is kind of ugly, but I couldn't see a better way.  One
    potential issue is that the splat and captures route params
    will be overwritten by the error handler.  There's no way to
    change that without breaking backwards compatibility.  We
    could potentially offer the route splat/captures under
    different param names, though.
This page is out of date. Refresh to see the latest.
Showing with 23 additions and 0 deletions.
  1. +6 −0 lib/sinatra/base.rb
  2. +17 −0 test/routing_test.rb
View
6 lib/sinatra/base.rb
@@ -1006,6 +1006,9 @@ def process_route(pattern, keys, conditions, block = nil, values = [])
conditions.each { |c| throw :pass if c.bind(self).call == false }
block ? block[self, values] : yield(self, values)
end
+ rescue
+ @env['sinatra.error.params'] = @params
+ raise
ensure
@params = original if original
end
@@ -1088,6 +1091,9 @@ def dispatch!
# Error handling during requests.
def handle_exception!(boom)
+ if error_params = @env['sinatra.error.params']
+ @params = @params.merge(error_params)
+ end
@env['sinatra.error'] = boom
if boom.respond_to? :http_status
View
17 test/routing_test.rb
@@ -628,6 +628,23 @@ class RoutingTest < Test::Unit::TestCase
assert_equal 'Hello World', body
end
+ it "makes original request params available in error handler" do
+ mock_app {
+ disable :raise_errors
+
+ get '/:foo' do
+ raise ArgumentError, "foo"
+ end
+
+ error do
+ "Hello #{params['foo']}2"
+ end
+ }
+
+ get '/bar'
+ assert_equal 'Hello bar2', body
+ end
+
it "transitions to 404 when passed and no subsequent route matches" do
mock_app {
get '/:foo' do
Something went wrong with that request. Please try again.