Skip to content

Decode extra parameters when processing route#26

Open
voikya wants to merge 1 commit intopatriciomacadden:masterfrom
voikya:unescape_params
Open

Decode extra parameters when processing route#26
voikya wants to merge 1 commit intopatriciomacadden:masterfrom
voikya:unescape_params

Conversation

@voikya
Copy link
Copy Markdown
Contributor

@voikya voikya commented Dec 30, 2020

Updates Hobbit::Base#find_route to perform URI-decoding on any dynamic
URI components before storing them in request.params. This provides
greater consistency with both Rack's existing query params parsing
and with other server libraries like Sinatra (which similarly decode
dynamic URI segments).

This ensures that everything in request.params (both from Rack and from Hobbit) will be URI-decoded, rather than mixing decoded query params and encoded path captures.

For comparison:

Rack (query param decoding): https://github.com/rack/rack/blob/8d2e02abba44ac7e9d0ed2a4af9beb6063a81296/lib/rack/utils.rb#L54

Sinatra (unescaping captures like this pull request):
https://github.com/sinatra/sinatra/blob/a9649b4f18a9059de3161906c9c6e95ec06fdef9/lib/sinatra/base.rb#L1053

I added a test and confirmed tests pass (although some of the other development dependencies, such as the Code Climate integration, seem to now be defunct)

Note that this might result in backwards-incompatibility if someone was dependent on the previous behavior. However, I believe this arrangement is more consistent.

Updates `Hobbit::Base#find_route` to perform URI-decoding on any dynamic
URI components before storing them in `request.params`. This provides
greater consistency with both Rack's existing query params parsing
and with other server libraries like Sinatra (which similarly decode
dynamic URI components).
@voikya
Copy link
Copy Markdown
Contributor Author

voikya commented Dec 30, 2020

Regarding the failing CI tests, I opened up a separate pull request to fix them: #27 caused by broken development dependencies in the gemspec file. If you prefer, I can rebase this pull request over those changes if/when you merge them.

@voikya
Copy link
Copy Markdown
Contributor Author

voikya commented Jul 6, 2022

@patriciomacadden Are you still interested in maintaining this repo? Or would you just recommend us to fork and develop it further on our own?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant