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.

Sign up for GitHub

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

Jump to bottom

Fix select helper with block returning non-string #51743

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix select helper with block returning non-string #51743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75c80c3
Fix select helper with block returning non-string
calleluks May 6, 2024
33bd853
Use an explicit block argument
calleluks May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions actionview/lib/action_view/helpers/tags/select.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ class Select < Base # :nodoc:
include SelectRenderer
include FormOptionsHelper

def initialize(object_name, method_name, template_object, choices, options, html_options)
@choices = block_given? ? template_object.capture { yield || "" } : choices
def initialize(object_name, method_name, template_object, choices, options, html_options, &block)
@choices = block_given? ? template_object.capture(&block) || "" : choices
@choices = @choices.to_a if @choices.is_a?(Range)

@html_options = html_options
Expand Down
13 changes: 13 additions & 0 deletions actionview/test/template/form_options_helper_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,19 @@ def test_select_under_fields_for_with_block_without_options
)
end

def test_select_under_fields_for_with_block_returning_a_non_string_value
@post = Post.new

output_buffer = fields_for :post, @post do |f|
concat(f.select(:category) { [] })
Copy link
Member

Choose a reason for hiding this comment

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

What's the expectation here, that it'd return an array of choices, rather than option tags? That seems new behavior, can we test that it works like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question!

No, the expectation remains the same: the block either outputs several option tags that get captured into the output buffer or returns a string that includes the option tags.

The problem is that when the helper is used as suggested in the docs to output several option tags within a call to each, and available_campaigns is empty, no option tags will be output, and the block will effectively return [] which will cause @choices in ActionView::Helpers::Tags::Select#initialize to be set to nil.

select(report, :campaign_ids) do
  available_campaigns.each do |c|
    tag.option(c.name, value: c.id, data: { tags: c.tags.to_json })
  end
end

This will in turn cause the following error in options_for_select:

NoMethodError: undefined method `map' for nil
    lib/action_view/helpers/form_options_helper.rb:365:in `options_for_select'
    lib/action_view/helpers/tags/select.rb:28:in `render'
    lib/action_view/helpers/form_options_helper.rb:160:in `select'
    lib/action_view/helpers/form_options_helper.rb:849:in `select'
    test/template/form_options_helper_test.rb:722:in `block in test_select_under_fields_for_with_block_returning_a_non_string_value'
    lib/action_view/helpers/capture_helper.rb:50:in `block in capture'
    lib/action_view/buffers.rb:75:in `capture'
    lib/action_view/helpers/capture_helper.rb:50:in `capture'
    lib/action_view/helpers/form_helper.rb:1088:in `fields'
    lib/action_view/helpers/form_helper.rb:1031:in `fields_for'
    test/template/form_options_helper_test.rb:721:in `test_select_under_fields_for_with_block_returning_a_non_string_value'

Would changing this line to something like this explain the situation better?

Suggested change
concat(f.select(:category) { [] })
concat(f.select(:category) { [].each { |title| content_tag(:option, title) })

end

assert_dom_equal(
"<select id=\"post_category\" name=\"post[category]\"></select>",
output_buffer
)
end

def test_select_with_multiple_to_add_hidden_input
output_buffer = select(:post, :category, "", {}, { multiple: true })
assert_dom_equal(
Expand Down