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

Set default form builder for a controller #19736

Merged
merged 1 commit into from Apr 14, 2015

Conversation

Projects
None yet
8 participants
@kmcphillips
Contributor

kmcphillips commented Apr 12, 2015

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

@senny

This comment has been minimized.

Show comment
Hide comment
@senny

senny Apr 13, 2015

Member

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

Member

senny commented Apr 13, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@edward

edward Apr 13, 2015

Contributor

@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.

Contributor

edward commented Apr 13, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@kmcphillips

kmcphillips Apr 13, 2015

Contributor

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.

Contributor

kmcphillips commented Apr 13, 2015

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

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 13, 2015

Member

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?

Member

rafaelfranca commented Apr 13, 2015

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

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 13, 2015

Member

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

Member

rafaelfranca commented Apr 13, 2015

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

@kmcphillips

This comment has been minimized.

Show comment
Hide comment
@kmcphillips

kmcphillips Apr 13, 2015

Contributor

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

Contributor

kmcphillips commented Apr 13, 2015

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

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 13, 2015

Member

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).

Member

rafaelfranca commented Apr 13, 2015

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

This comment has been minimized.

Show comment
Hide comment
@kmcphillips

kmcphillips Apr 14, 2015

Contributor

@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?

Contributor

kmcphillips commented Apr 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 14, 2015

Member

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.

Member

rafaelfranca commented Apr 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@kmcphillips

kmcphillips Apr 14, 2015

Contributor

Ok, updated.

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

Contributor

kmcphillips commented Apr 14, 2015

Ok, updated.

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

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

Merge pull request #19736 from kmcphillips/master
Set default form builder for a controller

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

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 14, 2015

Member

Awesome!

Member

rafaelfranca commented Apr 14, 2015

Awesome!

@@ -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

This comment has been minimized.

@s-aida

s-aida Apr 14, 2015

Contributor

Typo of subclass?

@s-aida

s-aida Apr 14, 2015

Contributor

Typo of subclass?

This comment has been minimized.

@kmcphillips
@kmcphillips
@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Apr 14, 2015

Member

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).

Member

carlosantoniodasilva commented Apr 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Apr 14, 2015

Member

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

Member

rafaelfranca commented Apr 14, 2015

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

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Apr 14, 2015

Member

👍

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

Member

carlosantoniodasilva commented Apr 14, 2015

👍

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

This comment has been minimized.

Show comment
Hide comment
@kmcphillips

kmcphillips Apr 14, 2015

Contributor

Super great. Thanks! 🚀 🌙

Contributor

kmcphillips commented Apr 14, 2015

Super great. Thanks! 🚀 🌙

@intrip

This comment has been minimized.

Show comment
Hide comment
@intrip

intrip Apr 24, 2015

Contributor

Nice job everybody.

Contributor

intrip commented Apr 24, 2015

Nice job everybody.

@md5

This comment has been minimized.

Show comment
Hide comment
@md5

md5 Jun 9, 2015

Contributor

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

Contributor

md5 commented Jun 9, 2015

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

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 9, 2015

Member

No. New features are not added to stable releases

Member

rafaelfranca commented Jun 9, 2015

No. New features are not added to stable releases

@md5

This comment has been minimized.

Show comment
Hide comment
@md5

md5 Jun 9, 2015

Contributor

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?

Contributor

md5 commented Jun 9, 2015

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

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jun 9, 2015

Member

It will be in the Rails 5 release

Member

rafaelfranca commented Jun 9, 2015

It will be in the Rails 5 release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment