Add form_with to unify form_tag/form_for. #26976

Merged
merged 16 commits into from Nov 21, 2016

Projects

None yet

4 participants

@kaspth
Member
kaspth commented Nov 5, 2016 edited

Ref #25197

form_tag and form_for serve very similar use cases. This
PR unifies that usage such that form_with can output just
the opening form tag akin to form_tag and can just work with
a url, for instance.

form_with by default doesn't attach a class or id to the form.

Ported over old tests where applicable to ensure maximum coverage,
but left some commented out because they don't yet apply (e.g.
fields_for later being replaced by fields).

Pending in this PR:

  • Add new fields DSL
  • Default to remote: true
  • Remove commented out tests in the form_with_test.rb.
  • Basic documentation
  • Changelog entry 😁

Later work up for grabs once this is in:

  • Add overridable field values, e.g. form.text_field :name, 'override'
  • Allow fields not on the model, e.g. form.text_field :not_implemented_on_model
  • Revisit form field defaults e.g. stop outputting ids and classes on fields by default
  • Revisit form options DSL methods
  • Implement form_tag and form_for through form_with (extract the implementations to a common object and set configs)
@kaspth kaspth added the actionview label Nov 5, 2016
@kaspth kaspth added this to the 5.1.0 milestone Nov 5, 2016
@kaspth kaspth self-assigned this Nov 5, 2016
+ # TODO: Documentation
+ def fields(scope = nil, model: nil, **options, &block)
+ fields_for(scope || model, model, **options, &block)
+ end
@kaspth
kaspth Nov 6, 2016 Member

@dhh added the fields method to do:

form_with(model: @post) do |form|
  form.fields(:comment, model: @comment) do |fields|
    fields.text_field # ...
  end
end

But it reads a bit weird if the passed model is a collection: fields(:comments, model: @post.comments).

Not sure if you intended this to be much more than a fields_for alias with the don't output ids and classes by default config.

kaspth added some commits Nov 5, 2016
@kaspth kaspth Add form_with to unify form_tag/form_for.
`form_tag` and `form_for` serve very similar use cases. This
PR unifies that usage such that `form_with` can output just
the opening form tag akin to `form_tag` and can just work with
a url, for instance.

`form_with` by default doesn't attach class or id to the form —
removing them on fields is moved out to a default revisiting PR later.

Ported over old tests where applicable to ensure maximum coverage,
but left some commented out because they don't yet apply (e.g.
`fields_for` later being replaced by `fields`).

[ Kasper Timm Hansen & Marek Kirejczyk ]
18a44be
@kaspth kaspth Code climatize. c0df7c6
@kaspth kaspth Add fields DSL method.
Strips `_for` and requires models passed as a keyword argument.
1e7e5cb
@kaspth kaspth Document form_with.
Graft the `form_for` docs: rewrite, revise and expand where
needed.

Also test that a `format` isn't used when an explicit URL
is passed.
a4a5945
@kaspth
Member
kaspth commented Nov 6, 2016

Documented form_with and had one hell of a time doing it! Really got to know the existing API and where the new one differs, plus in what way it does.

Will add docs for fields tomorrow.

@kaspth kaspth Enable remote by default.
Brand new world! Forms submit via XHRs by default, woah.
31a9608
@kaspth
Member
kaspth commented Nov 6, 2016

Figured since form_with already doesn't output id and class by default, we may as well rejigger the remaining default at the form level: remote. It's now on by default!

@kaspth kaspth Readd commented out tests.
Gives us something to revise when we're redoing the
form options helpers.

Also deletes the needless tests for the unsupported namespace
option.
3a55155
@kaspth
Member
kaspth commented Nov 8, 2016

This should be everything that's needed to prevent the default ids:

diff --git a/actionview/lib/action_view/helpers/form_helper.rb b/actionview/lib/action_view/helpers/form_helper.rb
index ef114b3..8550ad8 100644
--- a/actionview/lib/action_view/helpers/form_helper.rb
+++ b/actionview/lib/action_view/helpers/form_helper.rb
@@ -703,7 +703,7 @@ def form_with(model: nil, scope: nil, url: nil, format: nil, html: {}, remote: t
         html_options[:remote] = remote unless html_options.key?(:remote)

         if block_given?
-          builder = instantiate_builder(scope, model, options)
+          builder = instantiate_builder(scope, model, options.merge(skip_default_ids: true))
           output  = capture(builder, &Proc.new)
           html_options[:multipart] ||= builder.multipart?

@@ -963,12 +963,7 @@ def fields_for(record_name, record_object = nil, options = {}, &block)

       # TODO: Documentation
       def fields(scope = nil, model: nil, **options, &block)
-        # TODO: Remove when ids and classes are no longer output by default.
-        if model
-          scope ||= model_name_from_record_or_class(model).param_key
-        end
-
-        builder = instantiate_builder(scope, model, options)
+        builder = instantiate_builder(scope, model, options.merge(skip_default_ids: true))
         capture(builder, &block)
       end

@@ -1537,7 +1532,7 @@ def to_model
       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) : {}
+        @default_options = @options ? @options.slice(:index, :namespace, :skip_default_ids) : {}
         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
@@ -1840,6 +1835,7 @@ def fields_for(record_name, record_object = nil, fields_options = {}, &block)

       # TODO: Documentation
       def fields(scope = nil, model: nil, **options, &block)
+        options[:skip_default_ids] = true
         fields_for(scope || model, model, **options, &block)
       end

diff --git a/actionview/lib/action_view/helpers/tags/base.rb b/actionview/lib/action_view/helpers/tags/base.rb
index cf8a6d6..b8c446c 100644
--- a/actionview/lib/action_view/helpers/tags/base.rb
+++ b/actionview/lib/action_view/helpers/tags/base.rb
@@ -13,6 +13,7 @@ def initialize(object_name, method_name, template_object, options = {})

           @object_name.sub!(/\[\]$/, "") || @object_name.sub!(/\[\]\]$/, "]")
           @object = retrieve_object(options.delete(:object))
+          @skip_default_ids = options.delete(:skip_default_ids)
           @options = options
           @auto_index = Regexp.last_match ? retrieve_autoindex(Regexp.last_match.pre_match) : nil
         end
@@ -81,9 +82,12 @@ def add_default_name_and_id_for_value(tag_value, options)
           def add_default_name_and_id(options)
             index = name_and_id_index(options)
             options["name"] = options.fetch("name") { tag_name(options["multiple"], index) }
-            options["id"] = options.fetch("id") { tag_id(index) }
-            if namespace = options.delete("namespace")
-              options["id"] = options["id"] ? "#{namespace}_#{options['id']}" : namespace
+
+            unless skip_default_ids?
+              options["id"] = options.fetch("id") { tag_id(index) }
+              if namespace = options.delete("namespace")
+                options["id"] = options["id"] ? "#{namespace}_#{options['id']}" : namespace
+              end
             end
           end

@@ -154,6 +158,10 @@ def add_options(option_tags, options, value = nil)
           def name_and_id_index(options)
             options.key?("index") ? options.delete("index") || "" : @auto_index
           end
+
+          def skip_default_ids?
+            @skip_default_ids
+          end
       end
     end
   end

I think there's likely more code we can skip in the tags.

@kaspth kaspth [ci skip] Document the fields helpers.
Treat both the FormBuilder and FormHelper.
b714aa8
@kaspth
Member
kaspth commented Nov 13, 2016

Allright, I think this is ready for review 😁 — I'll merge this tomorrow/tuesday/later depending on the feedback.

+ # <input type="text" name="post[title]" value="<the title of the post>">
+ # </form>
+ #
+ # The parameters in the forms are accessible in controlleres according to
@vipulnsward
vipulnsward Nov 13, 2016 Member

controllers

+ #
+ # For ease of comparison the examples above left out the submit button,
+ # as well as the auto generated hidden fields that enable UTF-8 support
+ # and adds an authenticity token needed for Cross Site Request Forgery
@vipulnsward
vipulnsward Nov 13, 2016 Member

Don't think this needs title case. It is referenced as small case in its own documentation.

+ #
+ # ==== +form_with+ options
+ #
+ # * <tt>:url</tt> - The URL the form submits to. Akin to values passed to
@vipulnsward
vipulnsward Nov 13, 2016 Member

I would use "similar" instead of "Akin", since its easier to understand.

@kaspth
kaspth Nov 14, 2016 Member

Akin seems fine to me.

+ # either "get" or "post". If "patch", "put", "delete", or another verb
+ # is used, a hidden input named <tt>_method</tt> is added to
+ # simulate the verb over post.
+ # * <tt>:format</tt> - The format of the route post submits to.
@vipulnsward
vipulnsward Nov 13, 2016 Member

post, put, patch, etc

+ # * <tt>:scope</tt> - The scope to prefix input field names with and
+ # thereby how the submitted parameters are grouped in controllers.
+ # * <tt>:model</tt> - A model object to infer the <tt>:url</tt> and
+ # <tt>:scope</tt> by plus fill out input field values.
@vipulnsward
vipulnsward Nov 13, 2016 Member

either comma after "by", or "and" instead of "by".

+ # E.g. turn <tt>params[:post]</tt> into <tt>params[:article]</tt>.
+ # * <tt>:authenticity_token</tt> - Authenticity token to use in the form.
+ # Override with a custom authenticity token or pass <tt>false</tt> to
+ # skip the authenticity_token field altogether.
@vipulnsward
vipulnsward Nov 13, 2016 Member

<tt>:authenticity_token</tt>

+ # <form action="http://www.example.com" method="post" data-behavior="autosave" name="go">
+ # <input name="_method" type="hidden" value="patch" />
+ # ...
+ # </form>
@vipulnsward
vipulnsward Nov 13, 2016 Member

Looks like wrong output?

+ # Some ORM systems do not use IDs on nested models so in this case you want to be able
+ # to disable the hidden id.
+ #
+ # In the following example the Post model has many Comments stored within it in a NoSQL database,
@kaspth
kaspth Nov 14, 2016 Member

Think it's valid without the comma.

+ # Writers are considered a nested attributes setter if they're of the
+ # <tt>*_attributes=</tt> form, e.g. <tt>address_attributes=</tt>.
+ #
+ # Depending on the association's reader method return value a different
+ # <tt>*_attributes=</tt> form, e.g. <tt>address_attributes=</tt>.
+ #
+ # Depending on the association's reader method return value a different
+ # form builder is yielded. For single object returns a one-to-one builder,
@vipulnsward
vipulnsward Nov 13, 2016 Member

object, returns

@kaspth
kaspth Nov 14, 2016 Member

No those words are meant to stay together. We're talking about the return value of the reader method.

+ # Note +fields+ automatically generates a hidden field to store the record
+ # ID. For circumstances where this is not needed pass
+ # <tt>include_id: false</tt> to skip it.
+ def fields(scope = nil, model: nil, **options, &block)
@vipulnsward
vipulnsward Nov 13, 2016 Member

I believe this needs the same fixes from above.

+ #
+ # By default +form_with+ attaches the <tt>data-remote</tt> attribute
+ # submitting the form via an XMLHTTPRequest in the background if an
+ # an Unobtrusive JavaScript driver, like jquery-ujs, is used. See the
@dhh
dhh Nov 14, 2016 Member

Since we're actually dropping jquery-ujs, maybe we need to tell a different story here.

@kaspth
kaspth Nov 14, 2016 Member

I thought we we're just dropping the jquery in jquery-ujs. Isn't the plan to keep the ujs behavior around?

@dhh
dhh Nov 15, 2016 Member

Yeah, UJS is still there. Just that referencing jquery-ujs seems dated when that won't be the default.

@kaspth
kaspth Nov 15, 2016 Member

Got it 👍

+ # <tt>config.action_view.embed_authenticity_token_in_remote_forms = false</tt>.
+ # This is helpful when fragment-caching the form. Remote forms
+ # get the authenticity token from the <tt>meta</tt> tag, so embedding is
+ # unnecessary unless you support browsers without JavaScript.
@dhh
dhh Nov 14, 2016 Member

Since our new default is remote: true, I think we need to think this through a little more. Like having authenticity_token be reliant on that value. If you do remote: false, THEN we'll by default include authenticity_token, but otherwise not. And in both cases you can always supply one via the option to overwrite the default.

@kaspth
kaspth Nov 14, 2016 Member

Yup, figured it was just flipping the default. But of course there's repercussions.

@matthewd
matthewd Nov 14, 2016 Member

I suspect we should keep the token by default, because otherwise our default forms are broken on JS-less browsers (and anywhere without UJS present). I could maybe see the UJS gem toggling it off by default, but making UJS a hard dep for form submissions to work [out of the box] seems a bit rough.

@kaspth
kaspth Nov 14, 2016 Member

True, I guess that was the reason for config.action_view.embed_authenticity_token_in_remote_forms as well.

@dhh
dhh Nov 15, 2016 Member

True. Guess it can just ignore those embedded tokens. I was thinking that remote: true would force the form remote, but of course, if JS is off, that won't be read.

+ # unnecessary unless you support browsers without JavaScript.
+ # * <tt>:remote</tt> - Set to true to allow the Unobtrusive
+ # JavaScript drivers to control the submit behavior, defaulting to
+ # to an XHR submit. Disable with <tt>remote: false</tt>.
@dhh
dhh Nov 14, 2016 Member

The default is true, so needs to flip what this is for.

@kaspth
kaspth Nov 14, 2016 Member

Ah yes!

+ # JavaScript drivers to control the submit behavior, defaulting to
+ # to an XHR submit. Disable with <tt>remote: false</tt>.
+ # * <tt>:enforce_utf8</tt> - If set to false, a hidden input with name
+ # utf8 is not output. Default is true.
@dhh
dhh Nov 14, 2016 Member

When we have options that are default true, then it's generally better to flip the key name to a negative, imo. So we'd go with skip_enforcing_utf8: true.

@kaspth
kaspth Nov 14, 2016 Member

This was lifted from form_for. I figured the parity was better when it was a relatively oft unused option. Do you prefer that we rename still?

@dhh
dhh Nov 15, 2016 Member

This is a new API, so I saw we make the best choices we know how. Keeping parity is not important.

@kaspth
kaspth Nov 15, 2016 edited Member

Sure, then we should give that treatment to these options too:

skip_id: true # Was `include_id: false` for nested `fields`
skip_authenticity_token: true # Was just `authenticity_token: false`
local: true # Was `remote: true`. Not sure about this one though.
@kaspth
kaspth Nov 20, 2016 Member

Backing out of the skip_authenticy_token: true change. Since specifying a custom token via authenticy_token: 'abcdef' is valid. And would rather not have two options for the same thing.

+ # form_with(model: @post, url: super_posts_path)
+ # form_with(model: @post, scope: :article)
+ # form_with(model: @post, format: :json)
+ # form_with(model: @post, authenticity_token: false) # Disables the token.
@dhh
dhh Nov 14, 2016 Member

Fix when we default token to off with remote: true.

+
+ html_options = html.merge(options.except(:index, :include_id, :builder))
+ html_options[:method] ||= :patch if model.respond_to?(:persisted?) && model.persisted?
+ html_options[:remote] = remote unless html_options.key?(:remote)
@dhh
dhh Nov 14, 2016 Member

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.

@kaspth
kaspth Nov 14, 2016 Member

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 👍

+ # Zip code: <%= address_fields.text_field :zip_code %>
+ # <% end %>
+ # ...
+ # <% end %>
@dhh
dhh Nov 14, 2016 Member

Mixed use of ERB insertion tags. Should use them for form_with too.

+ # <% end %>
+ #
+ # When address is already an association on a Person you can use
+ # +accepts_nested_attributes_for+ to define the writer method for you:
@dhh
dhh Nov 14, 2016 Member

I'd actually like to kill accepts_nested_attributes_for in due time. Don't think we should promote it for this new API. Rather, let's just show how to do it by hand in the controller.

@kaspth
kaspth Nov 14, 2016 Member

Oh yeah! 🔥 Reading through this I thought accepts_nested_attributes_for stuck out, refrained from doing anything because I didn't have a better idea yet. But yes, let's start down the path toward something better!

@dhh
dhh Nov 15, 2016 Member

For starters, I'd just drop the entire topic for now.

+ # Note +fields+ automatically generates a hidden field to store the record
+ # ID. For circumstances where this is not needed pass
+ # <tt>include_id: false</tt> to skip it.
+ def fields(scope = nil, model: nil, **options, &block)
@dhh
dhh Nov 14, 2016 Member

Seems all this doc stuff is repeated from above?

@kaspth
kaspth Nov 14, 2016 Member

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.

kaspth added some commits Nov 14, 2016
@kaspth kaspth Link to `fields` helper method docs. e3eb691
@kaspth kaspth [ci skip] Documentation edits. 5a4052a
@kaspth kaspth Use ERB tags.
7d6fc1f
@kaspth
Member
kaspth commented Nov 14, 2016

@vipulnsward @dhh thanks for the feedback, I'll look more into this tomorrow 👍

kaspth added some commits Nov 20, 2016
@kaspth kaspth [ci skip] Remove nested attributes examples 694c226
@kaspth kaspth Invert `include_id` to `skip_id`.
`skip_id: true` reads better than `include_id: false` (since the
`include_id` default is true).
407d9f2
@kaspth kaspth Invert `remote` to `local`.
Since forms are remote by default, the option name makes more sense
as `local: true`.
42f4fb2
@kaspth kaspth Invert `enforce_utf8` to `skip_enforcing_utf8`. 15a805c
@kaspth kaspth Refer to the brand spanking new rails-ujs.
Soon to be bundled in Rails proper, so jquery-ujs is out.
17429bb
@kaspth kaspth Make `form_with` a bit more composed.
The flow is still not quite what it should be because the legacy
methods and these new ones pull at opposite ends.

Lots of options have been renamed, so now the new pieces don't fit
in so well.

I'll try to work on this in later commits after this PR (it's likely
there's a much better way to structure this whole part of Action View).
30ebe5d
@kaspth
Member
kaspth commented Nov 20, 2016

@dhh inverted the option names, removed the nested attributes example and tried to make form_with a bit more composed.

However, that carried over a lot of baggage from copying over html_options_for_formhttps://github.com/rails/rails/blob/6d6249b1c1abda4f62fafcc42a7ece570c8da7e9/actionview/lib/action_view/helpers/form_tag_helper.rb#L833-L854 — which was the most sensible way to do it that I could see.

I think it would be easier to improve the code structure further here in more focused commits after this PR.

@dhh
Member
dhh commented Nov 21, 2016

Looks pretty good to me 👍

@kaspth kaspth merged commit 67f81cc into rails:master Nov 21, 2016

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaspth kaspth deleted the kaspth:form-with-helper branch Nov 21, 2016
@kamipo kamipo added a commit to kamipo/rails that referenced this pull request Nov 24, 2016
@kamipo kamipo Fix warning: method redefined; discarding old fields
Follow up to #26976.
571c743
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment