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 enforce_utf8 to skip_enforcing_utf8.
  • Loading branch information
kaspth committed Nov 20, 2016
commit 15a805c38059a951c5fba1f6bc9281d6707fed4e
7 changes: 4 additions & 3 deletions actionview/lib/action_view/helpers/form_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -563,8 +563,8 @@ def apply_form_for_options!(record, object, options) #:nodoc:
# unnecessary unless you support browsers without JavaScript.
Copy link
Member

Choose a reason for hiding this comment

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

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.

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, figured it was just flipping the default. But of course there's repercussions.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

# * <tt>:local</tt> - By default form submits are remote and unobstrusive XHRs.
# Disable remote submits with <tt>local: true</tt>.
# * <tt>:enforce_utf8</tt> - If set to false, a hidden input with name
# utf8 is not output. Default is true.
# * <tt>:skip_enforcing_utf8</tt> - By default a hidden field named +utf8+
# is output to enforce UTF-8 submits. Set to true to skip the field.
# * <tt>:builder</tt> - Override the object used to build the form.
# * <tt>:id</tt> - Optional HTML id attribute.
# * <tt>:class</tt> - Optional HTML class attribute.
Expand Down Expand Up @@ -689,7 +689,7 @@ def apply_form_for_options!(record, object, options) #:nodoc:
# def labelled_form_with(**options, &block)
# form_with(**options.merge(builder: LabellingFormBuilder), &block)
# end
def form_with(model: nil, scope: nil, url: nil, format: nil, html: {}, local: false, **options)
def form_with(model: nil, scope: nil, url: nil, format: nil, html: {}, local: false, skip_enforcing_utf8: false, **options)
if model
url ||= polymorphic_path(model, format: format)

Expand All @@ -700,6 +700,7 @@ def form_with(model: nil, scope: nil, url: nil, format: nil, html: {}, local: fa
html_options = html.merge(options.except(:index, :skip_id, :builder))
html_options[:method] ||= :patch if model.respond_to?(:persisted?) && model.persisted?
html_options[:remote] = !local unless html_options.key?(:remote)
html_options[:enforce_utf8] = !skip_enforcing_utf8

if block_given?
builder = instantiate_builder(scope, model, options)
Expand Down
37 changes: 15 additions & 22 deletions actionview/test/template/form_helper/form_with_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ class FormWithActsLikeFormTagTest < FormWithTest

def hidden_fields(options = {})
method = options[:method]
enforce_utf8 = options.fetch(:enforce_utf8, true)
skip_enforcing_utf8 = options.fetch(:skip_enforcing_utf8, false)

"".tap do |txt|
if enforce_utf8
unless skip_enforcing_utf8
txt << %{<input name="utf8" type="hidden" value="&#x2713;" />}
end

Expand Down Expand Up @@ -93,16 +93,9 @@ def test_form_with_with_remote_false
assert_dom_equal expected, actual
end

def test_form_with_enforce_utf8_true
actual = form_with(enforce_utf8: true)
expected = whole_form("https://www.example.com", enforce_utf8: true)
assert_dom_equal expected, actual
assert actual.html_safe?
end

def test_form_with_enforce_utf8_false
actual = form_with(enforce_utf8: false)
expected = whole_form("https://www.example.com", enforce_utf8: false)
def test_form_with_skip_enforcing_utf8_true
actual = form_with(skip_enforcing_utf8: true)
expected = whole_form("https://www.example.com", skip_enforcing_utf8: true)
assert_dom_equal expected, actual
assert actual.html_safe?
end
Expand Down Expand Up @@ -673,24 +666,24 @@ def test_form_with_enables_remote_by_default
assert_dom_equal expected, output_buffer
end

def test_form_with_enforce_utf8_true
form_with(scope: :post, enforce_utf8: true) do |f|
def test_form_with_skip_enforcing_utf8_true
form_with(scope: :post, skip_enforcing_utf8: true) do |f|
concat f.text_field(:title)
end

expected = whole_form("/", enforce_utf8: true) do
expected = whole_form("/", skip_enforcing_utf8: true) do
"<input name='post[title]' type='text' id='post_title' value='Hello World' />"
end

assert_dom_equal expected, output_buffer
end

def test_form_with_enforce_utf8_false
form_with(scope: :post, enforce_utf8: false) do |f|
def test_form_with_skip_enforcing_utf8_false
form_with(scope: :post, skip_enforcing_utf8: false) do |f|
concat f.text_field(:title)
end

expected = whole_form("/", enforce_utf8: false) do
expected = whole_form("/", skip_enforcing_utf8: false) do
"<input name='post[title]' type='text' id='post_title' value='Hello World' />"
end

Expand Down Expand Up @@ -2117,10 +2110,10 @@ def test_form_with_only_instantiates_builder_once
def hidden_fields(options = {})
method = options[:method]

if options.fetch(:enforce_utf8, true)
txt = %{<input name="utf8" type="hidden" value="&#x2713;" />}
else
if options.fetch(:skip_enforcing_utf8, false)
txt = ""
else
txt = %{<input name="utf8" type="hidden" value="&#x2713;" />}
end

if method && !%w(get post).include?(method.to_s)
Expand All @@ -2145,7 +2138,7 @@ def whole_form(action = "/", id = nil, html_class = nil, local: false, **options

method, multipart = options.values_at(:method, :multipart)

form_text(action, id, html_class, local, multipart, method) + hidden_fields(options.slice :method, :enforce_utf8) + contents + "</form>"
form_text(action, id, html_class, local, multipart, method) + hidden_fields(options.slice :method, :skip_enforcing_utf8) + contents + "</form>"
end

def protect_against_forgery?
Expand Down