[WIP] Introduce form_with #25197

[marekkirejczyk]

Introducing form_with

Introduce form_with in place of form_for and form_tag.
This is a prototype and the purpose of this PR is to gather feedback.

[rails-bot]

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @schneems (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[kaspth]

Thanks for making headway on this! ❤️

I'm going to try to review it in the coming days. Since this PR potentially has a large footprint let's keep it to just @dhh's original examples. Then we get a foundation to build on in other PRs. Thoughts?

[marekkirejczyk]

@kaspth I completed work on comments from CR, improved code style in general and remove unused code. I also added support for radio buttons and nested fields.

I am trying not to grow this PR too big. I think functionality wise it is ~80% through. Test wise ~40%.

What could be postpone to next PRs:

• documentation
• tests for more exotic helpers i.e. color_field, date_field etc
• everything related to select helpers family - there is plenty of work there still
• custom builders

What I would finish in this PR are form_with itself tests. Just to make sure basic use cases are covered.

Looking forward to get feedback.

[kaspth]

everything related to select helpers family - there is plenty of work there still

Oh, yeah, FormOptionsHelper is going to be a big one.

Only got to respond to your comments today, I'll check back in on the tests soon 😁

[rafaelfranca]

I have a feeling that we are undoing all work I did in #4488.

The point of creating reusable classes in #4488 was to make easier to extend existing inputs and create news inputs in both the normal workflow or using form builders like Simple Form. I had a dream to one day make Rails default form builder extensible enough to things like f.input :name, type: :my_custom_input possible, where MyCustomInput is a class that knows how to render itself.

I believe it is possible and better to the framework at long term to reuse the classes created in #4488. Even more that the problem we are trying to solve is more a DSL problem than an implementation problem.

Right now we have two different DSLs to build forms depending if we have an object or not. Those two different DSLs already shares the same implementation. We are creating a new DSL with a new implementation to solve and we will end up with three DSLs and two implementations.

I'd focus on the problem we want to solve, unify the DSLs. Later we can revisit the defaults of current implementation. And if we manage to create a new implementation that satisfies the extensibility requirements of the current, we can go in that direction.

[marekkirejczyk]

@rafaelfranca That was my initial thinking, we have this pretty reusable classes family, why not use them? That is how implementation started. Then I encounter challenges:

1. The implementation is not HTML5 compliant
   To solve this we would need to:

• break backward compatibility or
• implement class hierarchy to support two modes - that would lead to code distributed among multiple classes and modules complicating implementation

2. There is plenty of logic that will have no use in new implementation
   i.e. generating tag id, name, retrieving objects by name
   And on the other hand some we would like to have freedom to add/remove/change options.

This logics spans across the whole hierarchy starting from processing arguments to actual logic.
Solving this would require again one of two: break compatibility or supporting two modes of operations.

Keeping in mind goals: HTML5 compliance and significant simplification of DSL... how would approach it/continue?

[rafaelfranca]

The implementation is not HTML5 compliant

Why they are not HTML5 compliant? AFAIK they are, and if they are not this is something we can fix and we should fix to all implementations. Like I said the goal of the David's proposal is to unify the DSLs. Anything beyond that can be worked but it is future work.

There is plenty of logic that will have no use in new implementation
i.e. generating tag id, name, retrieving objects by name
And on the other hand some we would like to have freedom to add/remove/change options.

This is the defaults revisiting part of my coment. The tag id will not be needed but name will. The form needs to know how to build automatic names otherwise there is no point of having a DSL if you have to explicitly set everything.

Keeping in mind goals: HTML5 compliance and significant simplification of DSL... how would approach it/continue?

By parts.

First, lets focus on the DSL. Having a new DSL building the same HTML code than the actual DSLs.

HTML5 compliance is a bug in the previous implementation and in my opinion and needs to be fixed on it too since, like I said, it is expected to be HTML compliant now.

Defaults revisiting is the last step and if we do the DSL part right, it can be just a matter of configuring the form builder object.

[marekkirejczyk]

@rafaelfranca
Looking at the code level, starting with simple text_field:

form_for @post do 
  f.text_field :title
end`

the text_field line will translate to text_field(@post, :title) and finally to: Tags::TextField.new(@post, :title, self, {}).render
and will produce following html:
<input type="text" name="post_title" id="post_title" />

There are two things we would like to change here:

• remove '/' at the end of tag (HTML5 compliance)
• get rid of id

So that we have:
<input type="text" name="post_title">

Now how do we pass this information from new FormWithBuilder to Tags::TextField.new constructor without braking compatibility?
My first idea was: Tags::TextField.new(object_name, :title, self, {html5: true, id: false}).render

But that would generate quite a lot of if-this-then-that code, i.e. in Tags::TextField:

if options[:html5]
  tag("input", options)
else
  tag.input(options)
end

This same for almost every single Tag and for every single feature. I.e.:

• What if we would like to change options of checkbox (i.e. check_box :admin, on: 'yes', off: 'no' instead of check_box :admin, checked_value: 'yes', unchecked_value: 'no'), radio button or select? Do we put if in every single class for every single option? Do we build two different implementations in two different classes (CheckBox and NewCheckBox)? Or build translation logic into new FromWithBuilder?
• What if we want to use named parameters in Tags::Base initializers? Can we do that or we have to stick to the current implemenation?

I happy to follow any path you show :) Just let me know how we should approach those design trade-offs.

[rafaelfranca]

There are two things we would like to change here:

remove '/' at the end of tag (HTML5 compliance)
get rid of id

Those are exactly what I called defaults revisiting, they don't need to happen now.

The first one is unnecessary, having / in the end of the tag is perfectly valid HTML5. See https://www.w3.org/TR/html5/syntax.html#void-elements.

The second one is valid and we don't need to duplicate all the code just to make this happen. Again, lets solve the DSL problem, later we revisit the defaults.

What if we would like to change options of checkbox (i.e. check_box :admin, on: 'yes', off: 'no' instead of check_box :admin, checked_value: 'yes', unchecked_value: 'no'), radio button or select? Do we put if in every single class for every single option? Do we build two different implementations in two different classes (CheckBox and NewCheckBox)? Or build translation logic into new FromWithBuilder?

If we want this in our new DSL it is easy to archive without changing anything in the Tags implementation.

def check_box(name, on: 'yes', off: 'no')
  Tags::Checkbox.new(name, checked_valid: on, unchecked_valid: off)
end

What if we want to use named parameters in Tags::Base initializers? Can we do that or we have to stick to the current implemenation?

This is also not required. Tags is an internal class we don't need to change the interface of the internal class to please our DSLs needs. We can build whatever DSL we want and still use the current interface of the Tags classes.

[marekkirejczyk]

Thanks for quick response.
Just to make sure - do I understand correctly? - what you're proposing is:

• keep /> at the end of void tags i.e. <input type="text" />
• when it comes to change in default Tags::Base classes behavior, we will do minimum of what is necessary, and implement those behavior with additional params and conditions, i.e.
  Tags::TextField.new(object_name, :title, self, {show_id: false}).render

if options[:show_id]
  tag("input", options)
else
  tag.input(options)
end

• do the parameters logic translation inside new builder i.e.

def check_box(name, on: 'yes', off: 'no')
  Tags::Checkbox.new(name, checked_valid: on, unchecked_valid: off)
end

• skip significant changes in Tags::Base hierarchy interface/api to keep backward compatibility?

I understand that those point are important defaults to revisit. Please correct if I misinterpret something.

I fine to go either way. I guess it is more decision to be made by you, @dhh and @kaspth. I'll be happy to implement whatever makes Rails developers happy :) Just let me know if we're stable on requirements :)

[kaspth]

when it comes to change in default Tags::Base classes behavior, we will do minimum of what is necessary

That decision comes later 😁

do the parameters logic translation inside new builder i.e

We might not need new builders right now. That was @rafaelfranca's point. This PR should just implement a new DSL using the existing Tags classes.

skip significant changes in Tags::Base hierarchy interface/api to keep backward compatibility?

We'll see about that when we revisit defaults in a later PR.

Just to reiterate. This PR: add a new DSL that unifies form_for/form_tag into form_with. A later PR: fine tune the HTML that is output and revisit the output defaults.

Let me know if you have any questions 😄

[kaspth]

@marekkirejczyk hey 👋, I opened a new PR to get this going again: #26976 — credited you as well 😉