Set default form builder for a controller #19736

kmcphillips · 2015-04-12T18:47:21Z

Problem

Form builders can only be overridden per call of form_for, or globally for the entire application. Often form builders will be specialized for all forms in a particular section or area. There is currently no way of defining a default form builder scoped to a particular area or context. This leads to repetition with having to pass in the form builder explicitly into every form.

Solution

Define a form_builder singleton method on the controller which allows a from builder to be specified per controller. DRY in the views.

This is modeled after how a layout is defined per controller.

This override has a higher prescidence than the ActionView global default_form_builder option, but lower prescidence than the explicit builder: option when calling form_for.

Example

Given a section specific form builder:

class AdminFormBuilder < ActionView::Helpers::FormBuilder
  def admin_element(name)
    # snip
  end
end

And an abstract controller superclass for an admin area:

class AdminAreaController < ApplicationController
  form_builder AdminAreaFormBuilder
end

Any calls to form_for for that controller or its subclasses will default to the form builder declared in that controller.

Tested in this example app:
https://github.com/kmcphillips/rails-test-app

Workarounds

The existing but insufficient workarounds I have been able to identify are:

Redefining the form_for method to inject builder: into the options hash.
Pass the builder: option into every form_for call.
Define a global form builder config.action_view.default_form_builder which contains specializations that may not apply to all areas.

cc @arthurnn @edward

kmcphillips · 2015-04-12T18:50:12Z

actionview/test/template/erb/helper.rb

@@ -14,7 +14,9 @@ def protect_against_forgery?() false end
  class BlockTestCase < ActiveSupport::TestCase
    def render_content(start, inside)
      template = block_helper(start, inside)
-      ActionView::Template::Handlers::Erubis.new(template).evaluate(ViewContext.new)
+      context = ViewContext.new
+      context.controller = BasicController.new


The context in this test was missing a controller and was raising on nil.

senny · 2015-04-13T07:55:43Z

@kmcphillips what about defining a method like admin_form_for, which adds the :builder option? In what area do you think that is insufficient?

edward · 2015-04-13T15:28:24Z

@senny it’s not that writing a wrapper like admin_form_for would be insufficient (since it would work), but that setting a default form builder at a controller/resource level feels like something that should be part of Rails already, given that you can already override the default view/layout a controller uses.

The background to this patch is that we’re starting to use more custom form builders in Shopify (a rather large Rails project) and are hitting a complexity wall with the workarounds mentioned above.

kmcphillips · 2015-04-13T15:34:40Z

Overriding a custom admin_form_for helper works of course, but it's another instance of having to remember to opt in. May as well just pass the builder in each time.

Rails allows you to scope nearly everything cleanly to split an app into sections, with this notable exception. Especially since you have the ability to override the form builder globally.

Shopify isn't the first project where I have wanted this feature either, FWIW.

rafaelfranca · 2015-04-13T15:47:44Z

About the feature, I can see as it can be useful. This would be similar to when we define which helper module is loaded by controller (if we disable the global require).

About the implementation, this is coupling Action View with Action Controller. How about instead making Action View asking Action Controller what is the form builder, we do not set it when creating the ActionView::Base instance at controller level?

rafaelfranca · 2015-04-13T15:49:04Z

Also I can see how Simple Form can benefits of this feature.

kmcphillips · 2015-04-13T15:51:15Z

@rafaelfranca Agree. Don't like the coupling. I'll look at refactoring it to Action View.

rafaelfranca · 2015-04-13T21:13:55Z

Given we will have a default form builder per action view instance and it
can be configured at controller level I wonder if we can deprecate the
application level configuration. If you want to define a application level
default form builder after this feature you could just define it in
ApplicationController.

Another thing, maybe we should call the singleton method
default_form_builder.

On Mon, Apr 13, 2015, 12:51 Kevin McPhillips notifications@github.com
wrote:

@rafaelfranca https://github.com/rafaelfranca Agree. Don't like the
coupling. I'll look at refactoring it to Action View.

—
Reply to this email directly or view it on GitHub
#19736 (comment).

kmcphillips · 2015-04-14T02:27:08Z

@rafaelfranca Great feedback.

I think it's a great idea to deprecate the global ActionView::Base.default_form_builder config, though I'd do that in a separate pull request.

I'll rename the method to be default_form_builder to be consistent. I like that.

About the implementation.

I can delegate :default_form_builder, to: :controller here. This is called when ActionView::Base is initialized:
https://github.com/rails/rails/blob/8ac458ad2e252ba041d9f4e42b94bd5997a622be/actionview/lib/action_view/helpers/controller_helper.rb#L10-11

That makes the implementation in the ActionView::Helper::FormHelper much cleaner:

def default_form_builder_class
  builder = default_form_builder || ActionView::Base.default_form_builder

And that works.

But when the view is not an instance of ActionView::Base and/or does not include ActionView::Helpers::ControllerHelper it raises since the method is not defined. For example, this test then fails:
https://github.com/rails/rails/blob/8ac458ad2e252ba041d9f4e42b94bd5997a622be/actionview/test/template/erb/helper.rb

I'm not sure if that means this test is broken, or if this is a valid case.

I can do self.respond_to?(:default_form_builder) but I think that's not any better.

What do you think?

rafaelfranca · 2015-04-14T02:45:21Z

I think instead of delegating we should do the same we do with request and config https://github.com/rails/rails/blob/8ac458ad2e252ba041d9f4e42b94bd5997a622be/actionview/lib/action_view/helpers/controller_helper.rb#L14-17.

And we define:

def default_form_builder_class
  builder = @_default_form_builder || ActionView::Base.default_form_builder

Or use a attr_internal as we do with request but defined in FormHelper module.

I think this would fix the problem with the custom view context.

kmcphillips · 2015-04-14T03:45:21Z

Ok, updated.

I also added tests to both the actionpack and actionview sides to make sure the values are passed through as expected.

Set default form builder for a controller

rafaelfranca · 2015-04-14T15:24:53Z

Awesome!

s-aida · 2015-04-14T16:02:52Z

actionpack/lib/action_controller/form_builder.rb

@@ -0,0 +1,48 @@
+module ActionController
+  # Override the default form builder for all views rendered by this
+  # controller and any of its descendents. Accepts a sublcass of


Typo of subclass?

@rafaelfranca Fixed in b4a9c59

carlosantoniodasilva · 2015-04-14T16:28:28Z

Just to add to the discussion, deprecating the AV config might not be an option if we want people to actually use it decoupled (which means there will be no controller default to inherit from).

rafaelfranca · 2015-04-14T16:55:06Z

@carlosantoniodasilva yeah, I thought that when I was merging it and forget to comment here.

carlosantoniodasilva · 2015-04-14T16:56:49Z

👍

On Tue, Apr 14, 2015 at 1:56 PM, Rafael Mendonça França <
notifications@github.com> wrote:

@carlosantoniodasilva https://github.com/carlosantoniodasilva yeah, I
thought that when I was merging it and forget to comment here.

—
Reply to this email directly or view it on GitHub
#19736 (comment).

At.
Carlos Antonio

kmcphillips · 2015-04-14T19:02:26Z

Super great. Thanks! 🚀 🌙

intrip · 2015-04-24T18:04:03Z

Nice job everybody.

md5 · 2015-06-09T21:55:16Z

Any chance this will end up in a 4.2.x release?

rafaelfranca · 2015-06-09T22:01:50Z

No. New features are not added to stable releases

md5 · 2015-06-09T22:04:40Z

I looked around for a Rails release roadmap, but I didn't find one. Should I expect this in a 4.3.x release or something else?

rafaelfranca · 2015-06-09T22:06:54Z

It will be in the Rails 5 release

kmcphillips reviewed Apr 12, 2015
View reviewed changes

rafaelfranca added the actionview label Apr 13, 2015

Override default form builder for a controller

2b8acdc

rafaelfranca added a commit that referenced this pull request Apr 14, 2015

Merge pull request #19736 from kmcphillips/master

9ec54d9

Set default form builder for a controller

rafaelfranca merged commit 9ec54d9 into rails:master Apr 14, 2015

s-aida reviewed Apr 14, 2015
View reviewed changes

md5 mentioned this pull request Jun 2, 2015

Add Admin::AdminController and Admin::FormBuilder hackforla/ohana-api-la#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set default form builder for a controller #19736

Set default form builder for a controller #19736

kmcphillips commented Apr 12, 2015

kmcphillips Apr 12, 2015

senny commented Apr 13, 2015

edward commented Apr 13, 2015

kmcphillips commented Apr 13, 2015

rafaelfranca commented Apr 13, 2015

rafaelfranca commented Apr 13, 2015

kmcphillips commented Apr 13, 2015

rafaelfranca commented Apr 13, 2015

kmcphillips commented Apr 14, 2015

rafaelfranca commented Apr 14, 2015

kmcphillips commented Apr 14, 2015

rafaelfranca commented Apr 14, 2015

s-aida Apr 14, 2015

kmcphillips Apr 14, 2015

carlosantoniodasilva commented Apr 14, 2015

rafaelfranca commented Apr 14, 2015

carlosantoniodasilva commented Apr 14, 2015

kmcphillips commented Apr 14, 2015

intrip commented Apr 24, 2015

md5 commented Jun 9, 2015

rafaelfranca commented Jun 9, 2015

md5 commented Jun 9, 2015

rafaelfranca commented Jun 9, 2015

Set default form builder for a controller #19736

Set default form builder for a controller #19736

Conversation

kmcphillips commented Apr 12, 2015

Problem

Solution

Example

Workarounds

kmcphillips Apr 12, 2015

Choose a reason for hiding this comment

senny commented Apr 13, 2015

edward commented Apr 13, 2015

kmcphillips commented Apr 13, 2015

rafaelfranca commented Apr 13, 2015

rafaelfranca commented Apr 13, 2015

kmcphillips commented Apr 13, 2015

rafaelfranca commented Apr 13, 2015

kmcphillips commented Apr 14, 2015

rafaelfranca commented Apr 14, 2015

kmcphillips commented Apr 14, 2015

rafaelfranca commented Apr 14, 2015

s-aida Apr 14, 2015

Choose a reason for hiding this comment

kmcphillips Apr 14, 2015

Choose a reason for hiding this comment

carlosantoniodasilva commented Apr 14, 2015

rafaelfranca commented Apr 14, 2015

carlosantoniodasilva commented Apr 14, 2015

kmcphillips commented Apr 14, 2015

intrip commented Apr 24, 2015

md5 commented Jun 9, 2015

rafaelfranca commented Jun 9, 2015

md5 commented Jun 9, 2015

rafaelfranca commented Jun 9, 2015