Allow specifying a params hash class to use when parsing #820
Conversation
|
@jeremyevans just in case because my 👍 is to vague ... I agree with the current PR and I second the idea of moving this out of Utils and allowing to subclass. |
|
@spastorino Thanks. I should be able to work on that soon. I'll push some additional commits when it is ready. |
|
Travis failure is only for ruby 2.0 (all other implementations pass), and appears to be unrelated: |
|
👍 and having this a non-global setting would be amazing |
|
@spastorino @tenderlove I think this is ready to be committed, could one of you please review and merge or let me know if you'd like any changes? |
|
@jeremyevans thanks for the work you've done. I will try to review as soon as I have some free time. |
lib/rack/multipart/parser.rb
Outdated
There was a problem hiding this comment.
I don't really want to encourage sticking more stuff in env. Can we just change the signature for create to have a default like initialize below?
There was a problem hiding this comment.
I'm fine with changing the API, I just wanted to keep API changes to the minimum. I've pushed another commit that changes the API as you suggested.
|
Can you make |
|
Sounds good, I've pushed another commit that makes that change. Thanks! |
This allows you to subclass the query parser to change the behavior. The main reason behind this is to allow changing the hash used to store params, so that indifferent access to params can be done efficiently without monkey patching. This adds Request#query_parser which is hard coded to use the default query parser (QueryParser::Default). However, subclasses of Request can override this method to use an instance of a subclass of QueryParser for custom behavior. For backwards compatibility, Utils still defines the same methods as before, but now they delegate to the default query parser. The multipart code should use the same params class used by the query parser for the request, so the relevant APIs have been changed to support passing in the params to use as an argument.
b6829e2 to
7e7a389
Compare
|
@tenderlove There were commits merged recently that conflicted with this pull request, so I've fixed the merge conflicts and squash rebased the branch against master. It should be safe to merge now. |
|
Thanks @jeremyevans I haven't had the time to go through this will try to do ASAP. |
Allow specifying a params hash class to use when parsing
|
@jeremyevans thanks for your work. |
Currently, Rack is hardcoded to use KeySpaceConstrainedParams
as the params hash class, and this commit doesn't change that
behavior.
Rails and Sinatra and other web frameworks that want to allow
indifferent access to the params generally end up making a deep
copy of the params. This is inefficient. If they could overide
the params hash class to use the type of indifferent hash they
want, they wouldn't need to make a deep copy, they could use the
hash Rack provides. This commit makes this possible without
monkey patching.
Note that this isn't really a perfect solution, since changing
the params_hash_class for the Utils class is still a global
change. Ideally, this code would be in a class separate from
Rack::Utils, which you could subclass and then inject that
dependency into whatever Request subclass you use for the
framework.
I'm certainly willing to do the work of making it possible for
local overrides of the params hash class, while keeping
backward compatibility. But as that's a bit more involved,
I'd like to get your feedback on this first part before doing
more work.