Fix missing search params for sort link #644
Conversation
|
Thanks! 💜 Will try to look at this soon. We'll need to keep the fix compatible with Rails 3 as well. |
|
@ryanswood, with the current Rails 5 master your test now passes without the HWIA fix. Could you check if this is the case for you as well? If so, it may still be a good idea to merge in your AC::Parameters test. |
|
@ryanswood Merged in a slightly modified version of your test spec and credited you in the change log. Thanks! |
|
@jonatack Sorry I have not been more reliable/available. I am at an early startup with just myself and another engineer. Regarding this PR, the fix was related to using Rails 4. I can't answer to Rails 5 as I am not using it yet (waiting for the final release). The reason being is that: return params unless params.respond_to?(:to_unsafe_h)
params.to_unsafe_hin Rails 4, the second line would always be evaluated which would return a hash that has strings for keys, thus the attempt to pull off the ActionController::Parameters.new(q: 1).to_unsafe_h.fetch(:q)
=> KeyError: key not found: :q
ActionController::Parameters.new(q: 1).to_unsafe_h.fetch('q')
=> 1
ActionController::Parameters.new(q: 1).to_unsafe_h.with_indifferent_access.fetch(:q)
=> 1Does the above make sense? It has been awhile since I had this all in my brain. My memory is failing me now as to how the previous change to prepare for Rails 5 made this a problem for me. Let me know if I can be of assistance. I should be more available this time. |
|
Thanks, you are correct. When the following is run... ActionController::Parameters.new(q: 1).to_unsafe_h.fetch(:q)...with Rails 5 it works, but not with Rails 4.2.6. The reason is that in Rails 5, def convert_parameters_to_hashes(value, using)
case value
when Array
value.map { |v| convert_parameters_to_hashes(v, using) }
when Hash
value.transform_values do |v|
convert_parameters_to_hashes(v, using)
end.with_indifferent_access
when Parameters
value.send(using)
else
value
end
end |
This adresses an issue correctly brought up by @ryanswood in PR #644 concerning Rails 4.2 where params#to_unsafe_h still needs to be converted from a Hash to a HWIA to respond to symbol hash keys. I prefer to trying adding a conditional on Active Record version rather than an extra HWIA conversion. The tests are run for Rails 4 and 5, since ActionController::Parameters was backported to 4.x. Rails 3 raises if ActionController::Parameters is invoked, so the tests are skipped in that case. This commit is necessary because when the following is run: ActionController::Parameters.new(q: 1).to_unsafe_h.fetch(:q) with Rails 5 it works, but not with Rails 4.2. The reason is that in Rails 5, #to_unsafe_h calls StrongParameters::convert_parameters_to_hashes which converts hashes to HWIA, whereas in Rails 4.2 it just calls #to_hash without HWIA.
|
@ryanswood Hopefully this resolves it correctly. Thanks again. |
This adresses an issue correctly brought up by @ryanswood in PR activerecord-hackery#644 concerning Rails 4.2 where params#to_unsafe_h still needs to be converted from a Hash to a HWIA to respond to symbol hash keys. I prefer to trying adding a conditional on Active Record version rather than an extra HWIA conversion. The tests are run for Rails 4 and 5, since ActionController::Parameters was backported to 4.x. Rails 3 raises if ActionController::Parameters is invoked, so the tests are skipped in that case. This commit is necessary because when the following is run: ActionController::Parameters.new(q: 1).to_unsafe_h.fetch(:q) with Rails 5 it works, but not with Rails 4.2. The reason is that in Rails 5, #to_unsafe_h calls StrongParameters::convert_parameters_to_hashes which converts hashes to HWIA, whereas in Rails 4.2 it just calls #to_hash without HWIA.
Problem:
SortLinksinternal parameter hash. It is taking the controller's parameter object and converting it to a regular hash in which all the keys are strings. During the assembly of the sort link, the code is then attempting to extract the search params using the default key of:q(or whatever what was configured by the user).I added a test that is using the real AC::Parameter object so that this bug could be caught. I
fixedmy personal app by changing the search key to'q'though I feel that change is a hack until this PR has been addressed.Bug introduced here: 989d