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

Fix issue with button_to's to_form_params #31320

Merged
merged 1 commit into from Jul 18, 2018

Conversation

gtyanchev
Copy link
Contributor

@gtyanchev gtyanchev commented Dec 3, 2017

Summary

I've encountered an exception while using the button_to helper. The code goes like this:

button_to "Submit", clients_path, method: :post, 
           params: { name: "Georgi", user: { email: "georgi@example.com" } }

And it's expected that it will generate a form with hidden fields:

<input type="hidden" name="name" value="Georgi" />
<input type="hidden" name="user[email]" value="georgi@example.com" />

But instead it raises:

ArgumentError: comparison of Symbol with String failed 

This happens because when the names for the hidden fields are extracted the name param is mapped to

{ name: :name, value: "Georgi" } 

and the user param is mapped to

{ name: "user[email]", value: "georgi@example.com" }

and after that these hashes are sorted by name, which triggers a comparison between :name and "user[email]" and so an exception is raised.

The same exception is also raised if we give a params hash with symbol and string key, for example:

button_to "Submit", clients_path, method: :post, 
           params: { :name => "Georgi", "email" => "georgi@example.com" }

The most unintrusive fix a can think of is to turn all hidden field names to string in the comparison block.

@rails-bot
Copy link

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @kaspth (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

Copy link
Member

@gmcgibbon gmcgibbon left a comment

Choose a reason for hiding this comment

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

Still reproducible in 6.0.0.alpha. Seems like a reasonable change to ensure proper sorting in #to_form_params.

@@ -664,7 +664,7 @@ def to_form_params(attribute, namespace = nil)
params << { name: namespace, value: attribute.to_param }
end

params.sort_by { |pair| pair[:name] }
params.sort_by { |pair| pair[:name].to_s }
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make more sense to cast to a string a few lines before in the case's else block: params << { name: namespace.to_s, value: attribute.to_param }

Copy link
Member

Choose a reason for hiding this comment

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

Agree

Copy link
Member

@rafaelfranca rafaelfranca left a comment

Choose a reason for hiding this comment

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

Could you apply the change suggested by @gmcgibbon?

@@ -664,7 +664,7 @@ def to_form_params(attribute, namespace = nil)
params << { name: namespace, value: attribute.to_param }
end

params.sort_by { |pair| pair[:name] }
params.sort_by { |pair| pair[:name].to_s }
Copy link
Member

Choose a reason for hiding this comment

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

Agree

@gtyanchev
Copy link
Contributor Author

The suggested changes are applied.

@kaspth
Copy link
Contributor

kaspth commented Jul 2, 2018

Thanks! Needs a rebase for the changelog, then I'll merge 😄

`button_to` was throwing exception when invoked with `params` hash that
contains symbol and string keys. The reason for the exception was that
`to_form_params` was comparing the given symbol and string keys.

The issue is fixed by turning all keys to strings inside
`to_form_params` before comparing them.
@gtyanchev
Copy link
Contributor Author

Rebased 🎉

@rafaelfranca rafaelfranca merged commit 5df23a0 into rails:master Jul 18, 2018
rafaelfranca added a commit that referenced this pull request Jul 18, 2018
Fix issue with `button_to`'s `to_form_params`
rafaelfranca added a commit that referenced this pull request Jul 18, 2018
Fix issue with `button_to`'s `to_form_params`
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.

None yet

5 participants