Consider AC::Parameters as Hash in url_for #42603
Conversation
casperisfine
force-pushed
the
url-for-params-ac-paramters
branch
from
6ea988d
to
67fd03a
Compare
| @@ -822,7 +822,7 @@ def url_for(options, route_name = nil, url_strategy = UNKNOWN, method_name = nil | |||
| path = route_with_params.path(method_name) | |||
| params = route_with_params.params | |||
|
|
|||
| if options[:params].is_a?(Hash) | |||
| if options[:params].is_a?(Hash) || options[:params].is_a?(ActionController::Parameters) | |||
There was a problem hiding this comment.
I was thinking that anything mergeable should fall in here. Instead of having an exhaustive list of mergeable types 🤔
| if options[:params].is_a?(Hash) || options[:params].is_a?(ActionController::Parameters) | |
| if options[:params].respond_to? :to_h |
There was a problem hiding this comment.
Maybe yeah. Would need to be :to_hash though, as to_h is explicit casting, and to_hash implicit.
There was a problem hiding this comment.
Ok, did just that. If that's good with you I'll merge on green.
Ref: rails#42020 It's not uncommon to want to forward request parameters, as such the `:params` paramter of `url_for` often receive an AC::Parameters instance.
casperisfine
force-pushed
the
url-for-params-ac-paramters
branch
from
67fd03a
to
fcc3cd3
Compare
| params.merge! options[:params] | ||
| elsif options.key? :params | ||
| params[:params] = options[:params] | ||
| if options.key? :params |
There was a problem hiding this comment.
I don't understand why you made a nest conditional. You already use the safe operator for checking options[:params]&.respond_to?(:to_hash)
There was a problem hiding this comment.
| if options.key? :params | |
| if options[:params]&.respond_to?(:to_hash) | |
| params.merge! options[:params] | |
| elsif options.key? :params | |
| params[:params] = options[:params] | |
| end |
|
can this be backported to v6.x please? |
|
No. This is a fix for a change that wasn't applied in 6.x. So not sure what you are after, but there's nothing to backport here. Additionally 6.x is security fixes only. |
Ref: #42020
It's not uncommon to want to forward request parameters, as such the
:paramsparameter ofurl_foroften receive an AC::Parameters instance.That previous PR did break our app quite extensively, and while I'm not 100% certain that behavior was intended, I can say with certainty that it's common.