-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Conversation
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. |
87514cc
to
b140e8a
Compare
There was a problem hiding this 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 } |
There was a problem hiding this comment.
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 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
There was a problem hiding this 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 } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree
b140e8a
to
c3f79fd
Compare
The suggested changes are applied. |
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.
c3f79fd
to
113d8a2
Compare
Rebased 🎉 |
Fix issue with `button_to`'s `to_form_params`
Fix issue with `button_to`'s `to_form_params`
Summary
I've encountered an exception while using the
button_to
helper. The code goes like this:And it's expected that it will generate a form with hidden fields:
But instead it raises:
This happens because when the names for the hidden fields are extracted the name param is mapped to
and the user param is mapped to
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:
The most unintrusive fix a can think of is to turn all hidden field names to string in the comparison block.