Skip to content
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

Only merge params option if params is a Hash in url_for helper #42020

Merged
merged 2 commits into from Jun 21, 2021
Merged

Only merge params option if params is a Hash in url_for helper #42020

merged 2 commits into from Jun 21, 2021

Conversation

ghost
Copy link

@ghost ghost commented Apr 19, 2021

This PR Fixes the problem described in this original issue that was fixed here and reverted here

The fix was reverted because the keyword params is special and is used to make possible to pass parameters that are reserved.

So this fix suggests to only merge params when it is a Hash.

The example in the revert commit from @rafaelfranca suggests that params, when used, takes the form of a Hash.

closes #42028
closes #42052

@rails-bot rails-bot bot added the actionpack label Apr 19, 2021
@ghost ghost changed the title This PR Fixes the problem described in [this original issue]{https://github.com/rails/rails/issues/33248} that was fixed [here]{https://github.com/rails/rails/pull/33256} and reverted [here]{https://github.com/rails/rails/commit/17e4b49292fb8da8a212e8200e2e94c44aedb788} Only merge params option if params is a Hash in url_for helper Apr 19, 2021
@benoittgt
Copy link
Contributor

Don't we need a test?

@sholden
Copy link
Contributor

sholden commented Apr 20, 2021

@benoittgt I added a test in the pull request @marivaldo just referenced, can pluck it from there. It actually tests the issue through a controller test case, which is how I first encountered the bug, but it might be better to test more directly.

@ghost
Copy link
Author

ghost commented Apr 20, 2021

@benoittgt I made a test for when params are not a hash. which made me realise that url_for is overriding existing params in this line.
In other words, if a special param called params is being passed, it will get deleted (or replaced). Therefore, in that special case, we want to keep the param called params in params[:params]

if you inspect the options that url_for built in that test case, you'll get something like
{:only_path=>true, :controller=>"c", :action=>"a", :params=>{:params=>"p"}, :path=>"/c/a", :script_name=>"", :user=>nil, :password=>nil}

@zzak zzak requested a review from rafaelfranca June 12, 2021 23:24
@zenspider
Copy link
Contributor

nudge

@@ -822,8 +822,10 @@ 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.key? :params
if options[:params].is_a?(Hash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this broke our app quite extensively as it's not rare for options[:params] to be an ActionController::Parameters.

I'll fix it in #42603, feel free to chime in.

casperisfine pushed a commit to Shopify/rails that referenced this pull request Jun 25, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants