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

When passed index: nil, form builder helpers should not infix name attribute with empty brackets [] #43194

Open
heynan0 opened this issue Sep 9, 2021 · 1 comment

Comments

@heynan0
Copy link

heynan0 commented Sep 9, 2021

I have a situation that requires me to explicitly define the named argument index: passed to a call of form_builder.text_field. The value for index is either set or not. If it is set (not nil), then it should take that and place the [] infix with the value inside the brackets, for the name attribute of the <input> element.

Steps to reproduce

Have a template that uses form_for and under that place a call to: form.text_field with an index: nil option.

<%= form_for :some_model do |form| %>
  <%= form.text_field :something, index: nil %>
<% end %>

Steps tracing the output for the name attribute that contains the "index":

From: [...]/lib/action_view/helpers/tags/base.rb:176 ActionView::Helpers::Tags::Base#name_and_id_index:

    174: def name_and_id_index(options)
    175:   binding.pry
 => 176:   if options.key?("index")
    177:     options.delete("index") || ""
    178:   elsif @generate_indexed_names
    179:     @auto_index || ""
    180:   end
    181: end

[1] pry(#<ActionView::Helpers::Tags::TextField>)> options
=> {"index"=>nil, "class"=>"form-control", "size"=>nil, "type"=>"text", "value"=>nil}

def name_and_id_index(options)
if options.key?("index")
options.delete("index") || ""

Note index is nil in the hash above, options. Since the key index is defined, it's taking the nil value, but due to short circuit with || it makes the function return empty string instead of nil.

[2] pry(#<ActionView::Helpers::Tags::TextField>)> up

From: [...]/lib/action_view/helpers/tags/base.rb:96 ActionView::Helpers::Tags::Base#add_default_name_and_id:

     95: def add_default_name_and_id(options)
 =>  96:   index = name_and_id_index(options)
     97:   options["name"] = options.fetch("name") { tag_name(options["multiple"], index) }
     98:
     99:   if generate_ids?
    100:     options["id"] = options.fetch("id") { tag_id(index) }
    101:     if namespace = options.delete("namespace")
    102:       options["id"] = options["id"] ? "#{namespace}_#{options['id']}" : namespace
    103:     end
    104:   end
    105: end

index = name_and_id_index(options)

index in line 96 is then set to empty string "", and a call to tag_name is made with the empty string as second argument.

[2] pry(#<ActionView::Helpers::Tags::TextField>)> next
[2] pry(#<ActionView::Helpers::Tags::TextField>)> tag_name(options["multiple"], index)
=> "some_model[][something]"
[3] pry(#<ActionView::Helpers::Tags::TextField>)> step
[3] pry(#<ActionView::Helpers::Tags::TextField>)> step

From: [...]/lib/action_view/helpers/tags/base.rb:108 ActionView::Helpers::Tags::Base#tag_name:

    107: def tag_name(multiple = false, index = nil)
 => 108:   binding.pry
    109:   # a little duplication to construct fewer strings
    110:   case
    111:   when @object_name.empty?
    112:     "#{sanitized_method_name}#{multiple ? "[]" : ""}"
    113:   when index
    114:     "#{@object_name}[#{index}][#{sanitized_method_name}]#{multiple ? "[]" : ""}"
    115:   else
    116:     "#{@object_name}[#{sanitized_method_name}]#{multiple ? "[]" : ""}"
    117:   end
    118: end

[3] pry(#<ActionView::Helpers::Tags::TextField>)> index
=> ""
[4] pry(#<ActionView::Helpers::Tags::TextField>)> next
[4] pry(#<ActionView::Helpers::Tags::TextField>)> next

From: [...]/lib/action_view/helpers/tags/base.rb:114 ActionView::Helpers::Tags::Base#tag_name:

    107: def tag_name(multiple = false, index = nil)
    108:   binding.pry
    109:   # a little duplication to construct fewer strings
    110:   case
    111:   when @object_name.empty?
    112:     "#{sanitized_method_name}#{multiple ? "[]" : ""}"
    113:   when index
 => 114:     "#{@object_name}[#{index}][#{sanitized_method_name}]#{multiple ? "[]" : ""}"
    115:   else
    116:     "#{@object_name}[#{sanitized_method_name}]#{multiple ? "[]" : ""}"
    117:   end
    118: end
    

def tag_name(multiple = false, index = nil)
# a little duplication to construct fewer strings
case
when @object_name.empty?
"#{sanitized_method_name}#{multiple ? "[]" : ""}"
when index
"#{@object_name}[#{index}][#{sanitized_method_name}]#{multiple ? "[]" : ""}"

Because index is passed as empty string (and not nil), it goes to the case when index, since empty string IS NOT FALSY in Ruby.

That results in an infix [] being added to the name attribute string. That would make sense if I had originally passed index: "", but here originally I passed index: nil. So I don't think this is the appropriate or correct behavior.


[4] pry(#<ActionView::Helpers::Tags::TextField>)> "#{@object_name}[#{index}][#{sanitized_method_name}]#{multiple ? "[]" : ""}"
=> "some_model[][something]"

Expected behavior

index: nil should not add an infix [] to the name attribute of the element

In contrast, index: "" should place the empty-bracket infix [] (this already happens)

Actual behavior

index: nil is adding the infix [] to the name attribute of the element

System configuration

Rails version: Rails 7.0.0.alpha.790723b

Ruby version: 3.0.0

@rails-bot
Copy link

rails-bot bot commented Dec 8, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants