I would like to propose a way for us to define error handling in the same FormBuilder used in the forms and not to split the logic between a form builder and a configuration option as is the current approach.
This will allow us to have different error logic and css classes depending on the FormBuilder instance used for the form. This will keep the logic of generating the field and the error of the field in the same file and class.
The problem that this change addresses is the following:
In two of the platforms that we have we had to gradually migrate the user interface between internally developed styles, then bootstrap 3, then bootstrap 4, then externally developed themes based on bootstrap4, then bootstrap 5.
We do this by using the layout method of the controllers and using different form builders like
# app/controllers/articles_controller.rb
class ArticlesController < ApplicationController
layout "based_on_bootstrap5"
end
# app/views/articles/_form.html.erb
<%= form_with @article, builder: Bootstrap5Builder %>
...
<% end %>
# app/controllers/author/articles_controller.rb
class Author::ArticlesController < ApplicationController
layout "based_on_bootstrap4"
end
# app/views/author/articles/_form.html.erb
<%= form_with @article, builder: Bootstrap4Builder %>
...
<% end %>
The example is simplified, but the idea is that we first migrate the Author:: where authors on our platform edit their articles. As we are happy with the new user interface that is limited only to the authors on our platform we then migrate the public views of the articles where readers could read the articles.
In the above example there are two FormBuilders, but there could be only one error handling and it is used by both.
This discussion is preceded by
and
where additional context is provided.
How the code currently works
ActionView allows us to define a custom form builder on a form_with and form_for like.
<%= form_with @article, builder: MyFormBuilder %>
<%= f.text_field :title %>
<% end %>
The custom form builder could look like this
class MyFormBuilder < ActionView::Helpers::FormBuilder
def text_field(method, options = {}, &block)
... # logic for generating the text field
end
end
If there is an error in the activerecord object then Actionview will add the text field in a new div with a class āfield_with_errorsā
# actionview/lib/action_view/base.rb
cattr_accessor :field_error_proc, default: Proc.new { |html_tag, instance| content_tag :div, html_tag, class: "field_with_errors" }
This field_error_proc could be configured with
config.action_view.field_error_proc
as is visible by
# guides/source/configuring.md
#### `config.action_view.field_error_proc`
Provides an HTML generator for displaying errors that come from Active Model. The block is evaluated within
the context of an Action View template. The default is
Proc.new { |html_tag, instance| content_tag :div, html_tag, class: "field_with_errors" }
This means that the logic for generating the field and the logic for wrapping the field in an error class are in two different places
The stack trace is the following
# actionview/lib/actionview/helpers/form_helpers.rb
2003: (field_helpers - [:label, :check_box, :radio_button, :fields_for, :fields, :hidden_field, :file_field]).each do |selector|
2004: class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
2005: def #{selector}(method, options = {}) # def text_field(method, options = {})
2006: @template.public_send( # @template.public_send(
2007: #{selector.inspect}, # :text_field,
2008: @object_name, # @object_name,
2009: method, # method,
2010: objectify_options(options)) # objectify_options(options))
2011: end # end
2012: RUBY_EVAL
2013: end
Here on line 2006 we call
# actionview/lib/actionview/helpers/form_helpers.rb
1169: def text_field(object_name, method, options = {})
1170: Tags::TextField.new(object_name, method, self, options).render
1171: end
This initialises a Tags::TextField and calls render
# actionview/lib/actionview/helpers/tags/text_field.rb
11: def render
12: options = @options.stringify_keys
13: options["size"] = options["maxlength"] unless options.key?("size")
14: options["type"] ||= field_type
15: options["value"] = options.fetch("value") { value_before_type_cast } unless field_type == "file"
16: add_default_name_and_id(options)
17: tag("input", options)
18: end
.tag on line 17 calls a method defined in ActiveModelHelpers If there are errors in the object there is a call to @template_object.instance_exec(html_tag, self, &Base.field_error_proc) on line 30.
# actionview/lib/actionview/helpers/active_model_helpers.rb
24: def tag(type, options, *)
25: tag_generate_errors?(options) ? error_wrapping(super) : super
26: end
27:
28: def error_wrapping(html_tag)
29: if object_has_errors?
30: @template_object.instance_exec(html_tag, self, &Base.field_error_proc)
31: else
32: html_tag
33: end
34: end
The code for generating an error is far away from the form builder. The path is FormBuilder->FormHelper->Tags::TextField->ActiveModelHelpers->config.action_view.field_error_proc
Possible improvements
My recommendation is for the FormBuilder to implement both the logic for generating a text_field and wrapping the generated text field tag in an error
Something like:
class MyFormBuilder < ActionView::Helpers::FormBuilder
def wrap_field_with_error html_tag, field_instance
# notice how text_field and this method that adds the css class around the text field are in the same builder.
@template.content_tag :div, html_tag, class: "my-form-builder-class-with-errors"
end
def text_field(method, options = {}, &block)
... # logic for generating the text field
end
end
For this to work there could be at least two changes that could be introduced.
Injecting the FormBuilder into the ActiveModelHelpers
I thought this would be the better solution - to inject the FormBuilder instance that we are using on the form directly into active_model_helpers. Then we will have:
24: def tag(type, options, *)
25: tag_generate_errors?(options) ? error_wrapping(super) : super
26: end
27:
28: def error_wrapping(html_tag)
29: if object_has_errors?
30: if @form_builder.respond_to?(:wrap_field_with_error)
31: @form_builder.wrap_field_with_error html_tag, self
32: else
33: @template_object.instance_exec(html_tag, self, &Base.field_error_proc)
34: end
35: else
36: html_tag
37: end
38: end
Notice the use of the @form_builder instance variable.
In this way we will not break the API of field_error_proc and will allow the FormBuilders to override the logic temporarily for this call to .text_field.
The challenge with this approach is that bringing the @form_builder instance to ActiveModelHelpers will require changes in method params in FormHelpers, or Tags::TextFields, and all the other fields.
I explored this approach and it might require a rather large change and it will also change API.
Wrapping FormBuilder calls to FormHelpers
FormBuilder acts as a proxy to FormHelpers. This is visible from the comment on the class
# actionview/lib/actionview/helpers/form_helpers.rb
# The +FormBuilder+ object can be thought of as serving as a proxy for the
# methods in the +FormHelper+ module. This class, however, allows you to
# call methods with the model object you are building the form for.
#
This allows us to proxy the logic for changing the field_error_proc before and after the FormBuilder delegates to FormHelpers. Something like:
(field_helpers - [:label, :check_box, :radio_button, :fields_for, :fields, :hidden_field, :file_field]).each do |selector|
class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
def #{selector}(method, options = {}) # def text_field(method, options = {})
ensure_local_error_proc_when_set do
@template.public_send( # @template.public_send(
#{selector.inspect}, # :text_field,
@object_name, # @object_name,
method, # method,
objectify_options(options)) # objectify_options(options))
end
end # end
RUBY_EVAL
end
def ensure_local_error_proc_when_set
old_proc = ActionView::Base.field_error_proc
begin
if self.respond_to?(:wrap_field_with_error)
form_builder = self
@local_field_error_proc = Proc.new { |html_tag, field_instance| form_builder.wrap_field_with_error(html_tag, field_instance) } if @local_field_error_proc == nil
ActionView::Base.field_error_proc = @local_field_error_proc
end
yield
ensure
ActionView::Base.field_error_proc = old_proc
end
end
The goal of this method is before every call to text_field to check if there is a method āwrap_field_with_errorā defined in the FormBuilder and if it is then to switch the field_error_proc. At the end we return the old field_error_proc.
I saw this approach in the tests
actionview/test/actionpack/controller/render_test.rb: old_proc = ActionView::Base.field_error_proc
actionview/test/actionpack/controller/render_test.rb: ActionView::Base.field_error_proc = proc { |html| tag.div html, class: "errors" }
actionview/test/actionpack/controller/render_test.rb: ActionView::Base.field_error_proc = old_proc if old_proc
This will allow us to have the two form builders with two different error wrapping logic.
class Bootstrap4FormBuilder < ActionView::Helpers::FormBuilder
def wrap_field_with_error html_tag, field_instance
@template.content_tag :div, html_tag, class: "one-class-with-errors"
end
def text_field(method, options = {}, &block)
... # logic for generating the text field
end
end
class TailwindFormBuilder < ActionView::Helpers::FormBuilder
def wrap_field_with_error html_tag, field_instance
@template.content_tag :div, html_tag, class: "completely-different-class-with-errors-suitable-for-tailwind"
end
def text_field(method, options = {}, &block)
... # logic for generating the text field
end
end
Pros:
- Engines will be able to provide their own form builders and error field wrapping logic.
- More than one engine could be used.
- Logic is easier to debug as it is in the same class
- API is not broken as we are still using ActionView::Base.field_error_proc if it is provide.
- The FormBuilders API could be introduced as a feature while the ensure_local_error_proc_when_set does not break the APIs and does not introduce changes to FormHelpers, Tags::TextField, all the other fields and ActiveModelHelper. Breaking changes, if needed, could be left for a major release.
Cons:
- There is an additional logic in FormBuilder#text_field.
I thought about preparing a proper PR with specs, but I wanted to get your feedback if this is a reasonable approach.
Let me know your thoughts.
@jacobdaddario tagging you as this might be of interest to you.