-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Fix issue with custom error handler on bad request #1351
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for fixing this!
#1339 candidate? |
@zzak I think this fits a patch release. Include it! |
I agree, this is a backwards compatible bug fix 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with a nit.
Thank you for the patch!
lib/sinatra/base.rb
Outdated
@@ -1086,7 +1087,7 @@ def invoke | |||
|
|||
# Dispatch a request with error handling. | |||
def dispatch! | |||
force_encoding(@params = IndifferentHash[@request.params]) | |||
force_encoding(@params = @params.merge(@request.params)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason to not use the merge!
method? It would be something like:
force_encoding(@params.merge!(@request.params))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll make that update 👍
Good. Thanks! |
Make sure
@params
are initialized for an app, so that they exist if processing a request with a custom error handler. Currently if params are invalid an error is raised before@params
is set, causing aNoMethodError
fornil:NilClass
down the chain. Fixes #1350.