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

Add form_with to unify form_tag/form_for. #26976

Merged
merged 16 commits into from
Nov 21, 2016
Merged
Prev Previous commit
Next Next commit
Invert include_id to skip_id.
`skip_id: true` reads better than `include_id: false` (since the
`include_id` default is true).
  • Loading branch information
kaspth committed Nov 20, 2016
commit 407d9f28ed7732981162a94ba247d3276455b130
16 changes: 14 additions & 2 deletions actionview/lib/action_view/helpers/form_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -653,7 +653,7 @@ def apply_form_for_options!(record, object, options) #:nodoc:
# thus there is no primary key for comments.
#
# <%= form_with(model: @post) do |form| %>
# <%= form.fields(:comments, include_id: false) do |fields| %>
# <%= form.fields(:comments, skip_id: true) do |fields| %>
# ...
# <% end %>
# <% end %>
Expand Down Expand Up @@ -698,7 +698,7 @@ def form_with(model: nil, scope: nil, url: nil, format: nil, html: {}, remote: t
scope ||= model_name_from_record_or_class(model).param_key
end

html_options = html.merge(options.except(:index, :include_id, :builder))
html_options = html.merge(options.except(:index, :skip_id, :builder))
html_options[:method] ||= :patch if model.respond_to?(:persisted?) && model.persisted?
html_options[:remote] = remote unless html_options.key?(:remote)
Copy link
Member

Choose a reason for hiding this comment

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

All this prep work is only used in the block_given? path, so it should be done there. Also, I think there's more we can do to make this a Composed Method. A lot of mechanics exposed at a high level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used in the else too as the last parameter here too: html_options = html_options_for_form(url || {}, html_options)

But yeah, let me see what I can do with the composition 👍


Expand Down Expand Up @@ -1583,13 +1583,17 @@ def initialize(object_name, object, template, options)
@nested_child_index = {}
@object_name, @object, @template, @options = object_name, object, template, options
@default_options = @options ? @options.slice(:index, :namespace) : {}

convert_to_legacy_options(@options)

if @object_name.to_s.match(/\[\]$/)
if (object ||= @template.instance_variable_get("@#{Regexp.last_match.pre_match}")) && object.respond_to?(:to_param)
@auto_index = object.to_param
else
raise ArgumentError, "object[] naming but object param and @object var don't exist or don't respond to to_param: #{object.inspect}"
end
end

@multipart = nil
@index = options[:index] || options[:child_index]
end
Expand Down Expand Up @@ -1885,6 +1889,8 @@ def fields_for(record_name, record_object = nil, fields_options = {}, &block)

# See the docs for the <tt>ActionView::FormHelper.fields</tt> helper method.
def fields(scope = nil, model: nil, **options, &block)
Copy link
Member

Choose a reason for hiding this comment

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

I believe this needs the same fixes from above.

Copy link
Member

Choose a reason for hiding this comment

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

Seems all this doc stuff is repeated from above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, copied to mirror how fields_for documents itself (i.e. both as the helper and in the builder). I'll update this to just refer to the fields helper docs above.

convert_to_legacy_options(options)

fields_for(scope || model, model, **options, &block)
end

Expand Down Expand Up @@ -2236,6 +2242,12 @@ def nested_child_index(name)
@nested_child_index[name] ||= -1
@nested_child_index[name] += 1
end

def convert_to_legacy_options(options)
if options.key?(:skip_id)
options[:include_id] = !options[:skip_id]
end
end
end
end

Expand Down
16 changes: 8 additions & 8 deletions actionview/test/template/form_helper/form_with_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1141,7 +1141,7 @@ def test_nested_fields_with_an_existing_record_on_a_nested_attributes_one_to_one

form_with(model: @post) do |f|
concat f.text_field(:title)
concat f.fields(:author, include_id: false) { |af|
concat f.fields(:author, skip_id: true) { |af|
af.text_field(:name)
}
end
Expand All @@ -1157,7 +1157,7 @@ def test_nested_fields_with_an_existing_record_on_a_nested_attributes_one_to_one
def test_nested_fields_with_an_existing_record_on_a_nested_attributes_one_to_one_association_with_disabled_hidden_id_inherited
@post.author = Author.new(321)

form_with(model: @post, include_id: false) do |f|
form_with(model: @post, skip_id: true) do |f|
concat f.text_field(:title)
concat f.fields(:author) { |af|
af.text_field(:name)
Expand All @@ -1175,9 +1175,9 @@ def test_nested_fields_with_an_existing_record_on_a_nested_attributes_one_to_one
def test_nested_fields_with_an_existing_record_on_a_nested_attributes_one_to_one_association_with_disabled_hidden_id_override
@post.author = Author.new(321)

form_with(model: @post, include_id: false) do |f|
form_with(model: @post, skip_id: true) do |f|
concat f.text_field(:title)
concat f.fields(:author, include_id: true) { |af|
concat f.fields(:author, skip_id: false) { |af|
af.text_field(:name)
}
end
Expand Down Expand Up @@ -1244,7 +1244,7 @@ def test_nested_fields_with_existing_records_on_a_nested_attributes_collection_a
concat af.text_field(:name)
}
@post.comments.each do |comment|
concat f.fields(:comments, model: comment, include_id: false) { |cf|
concat f.fields(:comments, model: comment, skip_id: true) { |cf|
concat cf.text_field(:name)
}
end
Expand All @@ -1265,7 +1265,7 @@ def test_nested_fields_with_existing_records_on_a_nested_attributes_collection_a
@post.comments = Array.new(2) { |id| Comment.new(id + 1) }
@post.author = Author.new(321)

form_with(model: @post, include_id: false) do |f|
form_with(model: @post, skip_id: true) do |f|
concat f.text_field(:title)
concat f.fields(:author) { |af|
concat af.text_field(:name)
Expand All @@ -1291,9 +1291,9 @@ def test_nested_fields_with_existing_records_on_a_nested_attributes_collection_a
@post.comments = Array.new(2) { |id| Comment.new(id + 1) }
@post.author = Author.new(321)

form_with(model: @post, include_id: false) do |f|
form_with(model: @post, skip_id: true) do |f|
concat f.text_field(:title)
concat f.fields(:author, include_id: true) { |af|
concat f.fields(:author, skip_id: false) { |af|
concat af.text_field(:name)
}
@post.comments.each do |comment|
Expand Down