Fix button_to's params option to support nested names. #17043
Conversation
|
Note: the two tests I added may incorrectly report passing status even without all of my changes due to the fact that assert_dom_equals is broken (I've resolved the issue in rails/rails-dom-testing#18 ) |
|
+1 I am just running into this bug! This has not been merged I guess? |
| # | ||
| # {name: 'Denmark'}.to_query('country') | ||
| # # => [{name: 'country[name]', value: 'Denmark'}] | ||
| def to_form_params(namespace = nil) |
There was a problem hiding this comment.
This method is only used in one place. It probably doesn't need to live in Active Support for general use. IMO, it'd make more sense as a private method in ActionView::Helpers::UrlHelper.
There was a problem hiding this comment.
@georgeclaghorn I put it here because it parallels to_param/to_query, and because I think it's a potentially helpful helper in Rails projects. For example, I've used it to take a hash of values (that could have been serialized as JSON but instead needed to be used in a HTML form) and create a set of hidden input fields.
There was a problem hiding this comment.
We should not add this to the public API.
|
@lustremedia Sadly, no. Anyone in the core team want to take a look at this? |
|
Thanks for doing this @jcoleman ! I'm hoping this gets added. It would be quite useful. |
jcoleman
force-pushed
the
fix-nested-params-in-button-to
branch
from
7224092
to
c212cb3
Compare
|
@sgrif I just rebased this to master and ran the test suite again. |
jcoleman
force-pushed
the
fix-nested-params-in-button-to
branch
from
bab4abd
to
9390e18
Compare
|
@sgrif I rebased to master and refactored the method I'd added in ActiveSupport into a function in UrlHelper to container the API leakage. |
| # | ||
| # to_form_params({name: 'Denmark'}, 'country') | ||
| # # => [{name: 'country[name]', value: 'Denmark'}] | ||
| def to_form_params(attribute, namespace = nil) |
There was a problem hiding this comment.
This still needs to be marked as # :nodoc: to avoid adding it to the public API
|
@sgrif I've added |
|
Please rebase. |
jcoleman
force-pushed
the
fix-nested-params-in-button-to
branch
from
f662cde
to
659080a
Compare
|
@maclover7 Rebased. |
|
@jcoleman conflicts |
jcoleman
force-pushed
the
fix-nested-params-in-button-to
branch
2 times, most recently
from
5c12910
to
4019bc1
Compare
|
@bluehallu @maclover7 @sgrif This is becoming rather annoying. This will be at least the 3rd time I've rebased this pull request. Each time the reason is purely because it takes so long to hear back from a committer or maintainer after I rebase. Could we please get this merged in this time? |
|
Rebased. Again. |
|
+1 Please merge |
|
+1 Please merge this already |
|
+1 Please merge! |
| # | ||
| # to_form_params({name: 'Denmark'}, 'country') | ||
| # # => [{name: 'country[name]', value: 'Denmark'}] | ||
| def to_form_params(attribute, namespace = nil) # :nodoc: |
There was a problem hiding this comment.
This method should be private.
There was a problem hiding this comment.
@rafaelfranca Actually marked with the Ruby keyword private? It's already marked :nodoc:
There was a problem hiding this comment.
Yes. This method should not be used by users (this is what # :nodoc: is for) and it is only used in this file (so it has private visibility)
| @@ -71,6 +71,34 @@ def test_url_for_with_invalid_referer | |||
| assert_equal 'javascript:history.back()', url_for(:back) | |||
| end | |||
|
|
|||
| def test_to_form_params_with_hash | |||
| assert_equal( | |||
| [{name: :name, value: 'David'}, {name: :nationality, value: 'Danish'}], | |||
There was a problem hiding this comment.
[{ name: :name, value: 'David' }, { name: :nationality, value: 'Danish' }],
In e6e0579 the `params` option was added to the `button_to` helper. However, the patch doesn't support nested hashes so `{a: {b: 'c'}}` for example gets turned into a hidden form input with the name 'a' and the value being the string representation of the `{b: 'c'}` nested hash. Since Rails supports nested hashes everywhere else (and even in the URL params of link_to and button_to), I believe this to be a bug/unfinished feature.
jcoleman
force-pushed
the
fix-nested-params-in-button-to
branch
from
4019bc1
to
459cd7f
Compare
|
@rafaelfranca I've incorporated all of your suggested changes. |
Fix button_to's params option to support nested names.
|
Thanks! |
Improve CHANGELOG for #17043 [ci skip]
In e6e0579 the
paramsoption was added to thebutton_tohelper. However, the patch doesn't support nested hashes so{a: {b: 'c'}}for example gets turned into a hidden form input with the name 'a' and the value being the string representation of the{b: 'c'}nested hash.Since Rails supports nested hashes everywhere else (and even in the URL params of link_to and button_to), I believe this to be a bug/unfinished feature.