Skip to content
This repository
Browse code

Refactor FormBuilder arguments and default config

* Do not reopen AV::Base to define default form builder
  Inside the load hook we are already in AV::Base context.

* Do not pass the given block to the form builder
  The block is evaluated in fields_for context using capture, with the
  builder as argument. This means we do not need to give the block to the
  FormBuilder itself.
  • Loading branch information...
commit 56089ca986c767763f29159c8de0aa1ebabfd4d2 1 parent d4a9ce8
Carlos Antonio da Silva authored January 18, 2012
17  actionpack/lib/action_view/helpers/form_helper.rb
@@ -373,7 +373,7 @@ def form_for(record, options = {}, &proc)
373 373
         options[:html][:method] = options.delete(:method) if options.has_key?(:method)
374 374
         options[:html][:authenticity_token] = options.delete(:authenticity_token)
375 375
 
376  
-        builder = options[:parent_builder] = instantiate_builder(object_name, object, options, &proc)
  376
+        builder = options[:parent_builder] = instantiate_builder(object_name, object, options)
377 377
         fields_for = fields_for(object_name, object, options, &proc)
378 378
         default_options = builder.multipart? ? { :multipart => true } : {}
379 379
         default_options.merge!(options.delete(:html))
@@ -601,7 +601,7 @@ def apply_form_for_options!(record, object, options) #:nodoc:
601 601
       #     ...
602 602
       #   <% end %>
603 603
       def fields_for(record_name, record_object = nil, options = {}, &block)
604  
-        builder = instantiate_builder(record_name, record_object, options, &block)
  604
+        builder = instantiate_builder(record_name, record_object, options)
605 605
         output = capture(builder, &block)
606 606
         output.concat builder.hidden_field(:id) if output && options[:hidden_field_id] && !builder.emitted_hidden_id?
607 607
         output
@@ -925,7 +925,7 @@ def range_field(object_name, method, options = {})
925 925
 
926 926
       private
927 927
 
928  
-        def instantiate_builder(record_name, record_object, options, &block)
  928
+        def instantiate_builder(record_name, record_object, options)
929 929
           case record_name
930 930
           when String, Symbol
931 931
             object = record_object
@@ -936,7 +936,7 @@ def instantiate_builder(record_name, record_object, options, &block)
936 936
           end
937 937
 
938 938
           builder = options[:builder] || ActionView::Base.default_form_builder
939  
-          builder.new(object_name, object, self, options, block)
  939
+          builder.new(object_name, object, self, options)
940 940
         end
941 941
     end
942 942
 
@@ -967,9 +967,9 @@ def to_model
967 967
         self
968 968
       end
969 969
 
970  
-      def initialize(object_name, object, template, options, proc)
  970
+      def initialize(object_name, object, template, options)
971 971
         @nested_child_index = {}
972  
-        @object_name, @object, @template, @options, @proc = object_name, object, template, options, proc
  972
+        @object_name, @object, @template, @options = object_name, object, template, options
973 973
         @parent_builder = options[:parent_builder]
974 974
         @default_options = @options ? @options.slice(:index, :namespace) : {}
975 975
         if @object_name.to_s.match(/\[\]$/)
@@ -1183,9 +1183,6 @@ def convert_to_model(object)
1183 1183
   end
1184 1184
 
1185 1185
   ActiveSupport.on_load(:action_view) do
1186  
-    class ActionView::Base
1187  
-      cattr_accessor :default_form_builder
1188  
-      @@default_form_builder = ::ActionView::Helpers::FormBuilder
1189  
-    end
  1186
+    cattr_accessor(:default_form_builder) { ::ActionView::Helpers::FormBuilder }
1190 1187
   end
1191 1188
 end
6  actionpack/test/controller/render_test.rb
@@ -9,7 +9,7 @@ def hello_world
9 9
     end
10 10
 
11 11
     def nested_partial_with_form_builder
12  
-      render :partial => ActionView::Helpers::FormBuilder.new(:post, nil, view_context, {}, Proc.new {})
  12
+      render :partial => ActionView::Helpers::FormBuilder.new(:post, nil, view_context, {})
13 13
     end
14 14
   end
15 15
 end
@@ -558,11 +558,11 @@ def partial_with_locals
558 558
   end
559 559
 
560 560
   def partial_with_form_builder
561  
-    render :partial => ActionView::Helpers::FormBuilder.new(:post, nil, view_context, {}, Proc.new {})
  561
+    render :partial => ActionView::Helpers::FormBuilder.new(:post, nil, view_context, {})
562 562
   end
563 563
 
564 564
   def partial_with_form_builder_subclass
565  
-    render :partial => LabellingFormBuilder.new(:post, nil, view_context, {}, Proc.new {})
  565
+    render :partial => LabellingFormBuilder.new(:post, nil, view_context, {})
566 566
   end
567 567
 
568 568
   def partial_collection

0 notes on commit 56089ca

Aaron Patterson

Can we keep the constructor signature the same, and emit a warning if someone passes in a proc? This borked my custom form builder subclass where I just call super. :(

Carlos Antonio da Silva

Hm I think so, although the internal implementation doesn't need to give the block to the constructor, we can leave the same signature for now. I can review that and add a warning in case the block attribute is given in case of super, defaulting to nil, sounds good?

Please sign in to comment.
Something went wrong with that request. Please try again.