-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
form_tag
and form_with
: close tag when block omitted
#44275
base: main
Are you sure you want to change the base?
form_tag
and form_with
: close tag when block omitted
#44275
Conversation
6652fea
to
6b60718
Compare
Sounds reasonable, and the change looks good. Just wanted to bump this PR 🙃 |
This is the described intent of the method, per #26976 -- it has a non-block form so that it can be support the I'm doubtful that entirely-empty-form-tag is a common enough use case that people are likely to call this blocklessly and expect/want that result... is that a frequent pattern in a context I'm unfamiliar with? |
The use case that got me digging was inline editing: <%# app/views/articles/_inline_edit.html.erb %>
<% frame_id = dom_id(model, "#{method}_turbo_frame") %>
<%= form_with model: model, data: { turbo_frame: frame_id } do %>
<turbo-frame id="<%= frame_id %>" class="contents group inline-edit">
<%= yield %>
<%= link_to edit_article_path(model) do %>
Edit <%= model.class.human_attribute_name(method) %>
<% end %>
</turbo-frame>
<% end %> Rendering the partial with a block would wrap the contents (through the <% frame_id = dom_id(model, "#{method}_turbo_frame") %>
<%= form_with model: model, id: "#{method}_form", data: { turbo_frame: frame_id } %>
<turbo-frame id="<%= frame_id %>" class="contents group inline-edit">
<%= yield %>
<%= link_to edit_article_path(model) do %>
Edit <%= model.class.human_attribute_name(method) %>
<% end %>
</turbo-frame> This version would require callers to render fields with a I'd never deployed the pattern before. It's certainly an edge case that's extremely uncommon. I think my surprise stems from the overlap between empty If ensuring the |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
6b60718
to
074a5bf
Compare
074a5bf
to
cf62569
Compare
72f8221
to
ca7ff94
Compare
@matthewd I'm sorry to have let this PR drift so far behind To re-iterate my main motivation for this change: I think my surprise stems from the overlap between empty Even if it isn't intended as an end-result, a call to |
ca7ff94
to
34d8993
Compare
34d8993
to
f48c19b
Compare
@matthewd I've expanded this work to include a new Does that level on granular control improve this change's backwards compatibility? I'm hopeful that it could also provide an avenue toward deprecation, then maybe global support in the next major release. |
form_tag
and form_with
: close tag when block omitted
f48c19b
to
becbe8a
Compare
becbe8a
to
b3537c9
Compare
87360e5
to
86e6247
Compare
c7f8de5
to
7ac65d2
Compare
7ac65d2
to
49fae33
Compare
4bd9be2
to
ebd793c
Compare
ebd793c
to
732a606
Compare
@rafaelfranca could you review these changes? |
e4b8c7d
to
a22496e
Compare
Another use case for an empty form, is a form with the submit button outside the form element: <form id="create_like" action="likes" method="POST" >
<input type="hidden" name="authenticity_token" value="zZEbm" />
</form>
<button type="submit" form="create_like">:+1</button> This would be useful when the button is already in a form: <form id="create_like" action="likes" method="POST" >
...
</form>
<form action="comment" method="POST">
....
<button type="submit" form="create_like">:+1</button>
</form> |
a22496e
to
dbc32eb
Compare
dbc32eb
to
2cb3b07
Compare
@skipkayhil are you able to review this proposal? |
2cb3b07
to
99bc8e8
Compare
99bc8e8
to
b5d9b50
Compare
5e28e7b
to
12dfc38
Compare
Prior to this change, calling a `form_tag` or `form_with` without passing a block generates a `<form>` element without a closing `</form>` tag. The [form_for][] version of building a `<form>` element requires a block to execute, so omitting the closing `</form>` element isn't a possibility. A current work-around to achieve the desired behavior is to declare the `form_tag` or `form_with` call with an empty block: ```erb <%= form_tag "https://example.com" do %> <% end %> <%= form_with url: "https://example.com" do %> <% end %> ``` Unfortunately, being unaware of this issue will likely yield HTML that is silently invalid, which can cause surprising nesting and behavior, such as elements declared after the `<form>` becoming descendants instead of siblings. Introduce the `config.action_view.closes_form_tag_without_block` configuration value. By default (in versions prior to 7.2), that value is `false` and preserves backwards compatibility. When set to `true`, calls to `form_with` and `form_tag` without a block will result in elements that close themselves. [form_for]: https://github.com/rails/rails/blob/efae65d005ee298207d720fb4f61c28a38973e8e/actionview/test/template/form_helper_test.rb#L1555-L1560
12dfc38
to
43724ba
Compare
Motivation / Background
Prior to this change, calling a
form_tag
orform_with
withoutpassing a block generates a
<form>
element without a closing</form>
tag.
The form_for version of building a
<form>
element requires a blockto execute, so omitting the closing
</form>
element isn't apossibility.
A current work-around to achieve the desired behavior is to declare the
form_tag
orform_with
call with an empty block:Unfortunately, being unaware of this issue will likely yield HTML that
is silently invalid, which can cause surprising nesting and behavior,
such as elements declared after the
<form>
becoming descendantsinstead of siblings.
Detail
Introduce the
config.action_view.closes_form_tag_without_block
configuration value.
By default (in versions prior to 7.2), that value is
false
andpreserves backwards compatibility. When set to
true
, calls toform_with
andform_tag
without a block will result in elements thatclose themselves.
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]