Refactor partial rendering into a PartialTemplate class. [Pratik]

git-svn-id: http://svn-commit.rubyonrails.org/rails/trunk@8976 5ecf4fe2-1ee6-0310-87b1-e25e094e27de

14 files changed, 211 insertions, 140 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index 4413ed3477..782e364f7e 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Refactor partial rendering into a PartialTemplate class. [Pratik]
+
 * Added that requests with JavaScript as the priority mime type in the accept header and no format extension in the parameters will be treated as though their format was :js when it comes to determining which template to render. This makes it possible for JS requests to automatically render action.js.rjs files without an explicit respond_to block  [DHH]
 
 * Tests for distance_of_time_in_words with TimeWithZone instances. Closes #10914 [ernesto.jimenez]
diff --git a/actionpack/lib/action_view.rb b/actionpack/lib/action_view.rb
index c3b846eece..e20812d308 100644
--- a/actionpack/lib/action_view.rb
+++ b/actionpack/lib/action_view.rb
@@ -29,6 +29,7 @@ require 'action_view/template_handlers/rjs'
 
 require 'action_view/template_finder'
 require 'action_view/template'
+require 'action_view/partial_template'
 
 require 'action_view/base'
 require 'action_view/partials'
diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb
index f5346bd480..143428c6d9 100644
--- a/actionpack/lib/action_view/base.rb
+++ b/actionpack/lib/action_view/base.rb
@@ -292,24 +292,22 @@ If you are rendering a subtemplate, you must now use controller-like partial syn
     
     # Renders the template present at <tt>template_path</tt> (relative to the view_paths array). 
     # The hash in <tt>local_assigns</tt> is made available as local variables.
-    def render(options = {}, old_local_assigns = {}, &block) #:nodoc:
+    def render(options = {}, local_assigns = {}, &block) #:nodoc:
       if options.is_a?(String)
-        render_file(options, true, old_local_assigns)
+        render_file(options, true, local_assigns)
       elsif options == :update
         update_page(&block)
       elsif options.is_a?(Hash)
         options = options.reverse_merge(:locals => {}, :use_full_path => true)
 
-        if options[:layout]
-          path, partial_name = partial_pieces(options.delete(:layout))
-
+        if partial_layout = options.delete(:layout)
           if block_given?
             wrap_content_for_layout capture(&block) do 
-              concat(render(options.merge(:partial => "#{path}/#{partial_name}")), block.binding)
+              concat(render(options.merge(:partial => partial_layout)), block.binding)
             end
           else
             wrap_content_for_layout render(options) do
-              render(options.merge(:partial => "#{path}/#{partial_name}"))
+              render(options.merge(:partial => partial_layout))
             end
           end
         elsif options[:file]
@@ -325,17 +323,8 @@ If you are rendering a subtemplate, you must now use controller-like partial syn
       end
     end
 
-    # Renders the +template+ which is given as a string as either erb or builder depending on <tt>template_extension</tt>.
-    # The hash in <tt>local_assigns</tt> is made available as local variables.
     def render_template(template) #:nodoc:
-      handler = template.handler
-      @current_render_extension = template.extension
-
-      if handler.compilable?
-        compile_and_render_template(handler, template)
-      else
-        handler.render(template.source, template.locals)
-      end
+      template.render
     end
 
     # Returns true is the file may be rendered implicitly.
@@ -369,7 +358,7 @@ If you are rendering a subtemplate, you must now use controller-like partial syn
       end
     end
 
-    private
+    private    
       def wrap_content_for_layout(content)
         original_content_for_layout = @content_for_layout
         @content_for_layout = content
@@ -388,24 +377,11 @@ If you are rendering a subtemplate, you must now use controller-like partial syn
       def assign_variables_from_controller
         @assigns.each { |key, value| instance_variable_set("@#{key}", value) }
       end
-
-      # Render the provided template with the given local assigns. If the template has not been rendered with the provided
-      # local assigns yet, or if the template has been updated on disk, then the template will be compiled to a method.
-      #
-      # Either, but not both, of template and file_path may be nil. If file_path is given, the template
-      # will only be read if it has to be compiled.
-      #
-      def compile_and_render_template(handler, template) #:nodoc:
-        # compile the given template, if necessary
-        handler.compile_template(template)
-
-        # Get the method name for this template and run it
-        method_name = @@method_names[template.method_key]
-        evaluate_assigns
-
-        send(method_name, template.locals) do |*name|
-          instance_variable_get "@content_for_#{name.first || 'layout'}"
-        end
+      
+      def execute(template)
+        send(template.method, template.locals) do |*names|
+          instance_variable_get "@content_for_#{names.first || 'layout'}"
+        end        
       end
   end
 end
diff --git a/actionpack/lib/action_view/partial_template.rb b/actionpack/lib/action_view/partial_template.rb
new file mode 100644
index 0000000000..a470c915cd
--- /dev/null
+++ b/actionpack/lib/action_view/partial_template.rb
@@ -0,0 +1,61 @@
+module ActionView #:nodoc:
+  class PartialTemplate < Template #:nodoc:
+    
+    attr_reader :variable_name, :object
+    
+    def initialize(view, partial_path, object = nil, locals = {})
+      @path, @variable_name = extract_partial_name_and_path(view, partial_path)
+      super(view, @path, true, locals)
+      add_object_to_local_assigns!(object)
+      
+      # This is needed here in order to compile template with knowledge of 'counter'
+      initialize_counter
+      
+      # Prepare early. This is a performance optimization for partial collections
+      prepare!
+    end
+    
+    def render
+      ActionController::Base.benchmark("Rendered #{@path}", Logger::DEBUG, false) do
+        @handler.render(self)
+      end
+    end
+    
+    def render_member(object)
+      @locals[@counter_name] += 1
+      @locals[:object] = @locals[@variable_name] = object
+      render
+    end
+    
+    private
+    
+    def add_object_to_local_assigns!(object)
+      @locals[:object] ||=
+        @locals[@variable_name] ||=
+          if object.is_a?(ActionView::Base::ObjectWrapper)
+            object.value
+          else
+            object
+          end || @view.controller.instance_variable_get("@#{variable_name}")
+    end
+    
+    def extract_partial_name_and_path(view, partial_path)
+      path, partial_name = partial_pieces(view, partial_path)
+      [File.join(path, "_#{partial_name}"), partial_name.split('/').last.split('.').first.to_sym] 
+    end
+    
+    def partial_pieces(view, partial_path)
+      if partial_path.include?('/')
+        return File.dirname(partial_path), File.basename(partial_path)
+      else
+        return view.controller.class.controller_path, partial_path
+      end
+    end
+    
+    def initialize_counter
+      @counter_name ||= "#{@variable_name}_counter".to_sym
+      @locals[@counter_name] = 0
+    end
+    
+  end
+end
diff --git a/actionpack/lib/action_view/partials.rb b/actionpack/lib/action_view/partials.rb
index e2aa98a319..7f1963fb29 100644
--- a/actionpack/lib/action_view/partials.rb
+++ b/actionpack/lib/action_view/partials.rb
@@ -103,23 +103,11 @@ module ActionView
   # As you can see, the :locals hash is shared between both the partial and its layout.
   module Partials
     private
-      def render_partial(partial_path, object_assigns = nil, local_assigns = nil) #:nodoc:
+      def render_partial(partial_path, object_assigns = nil, local_assigns = {}) #:nodoc:
         case partial_path
         when String, Symbol, NilClass
-          path, partial_name = partial_pieces(partial_path)
-          full_partial_path = File.join(path, "_#{partial_name}")
-          object = extracting_object(partial_name, object_assigns)
-          local_assigns = local_assigns ? local_assigns.clone : {}
-          add_counter_to_local_assigns!(partial_name, local_assigns)
-          add_object_to_local_assigns!(partial_name, local_assigns, object)
-
-          if logger && logger.debug?
-            ActionController::Base.benchmark("Rendered #{full_partial_path}", Logger::DEBUG, false) do
-              render(full_partial_path, local_assigns)
-            end
-          else
-              render(full_partial_path, local_assigns)
-          end
+          # Render the template
+          ActionView::PartialTemplate.new(self, partial_path, object_assigns, local_assigns).render
         when ActionView::Helpers::FormBuilder
           builder_partial_path = partial_path.class.to_s.demodulize.underscore.sub(/_builder$/, '')
           render_partial(builder_partial_path, object_assigns, (local_assigns || {}).merge(builder_partial_path.to_sym => partial_path))
@@ -132,75 +120,21 @@ module ActionView
             ""
           end
         else
-          render_partial(
-            ActionController::RecordIdentifier.partial_path(partial_path),
-            partial_path, local_assigns)
+          render_partial(ActionController::RecordIdentifier.partial_path(partial_path), partial_path, local_assigns)
         end
       end
 
-      def render_partial_collection(partial_name, collection, partial_spacer_template = nil, local_assigns = nil) #:nodoc:
-        collection_of_partials = Array.new
-        counter_name = partial_counter_name(partial_name)
+      def render_partial_collection(partial_path, collection, partial_spacer_template = nil, local_assigns = {}) #:nodoc:
+        return " " if collection.empty?
+        
         local_assigns = local_assigns ? local_assigns.clone : {}
-        collection.each_with_index do |element, counter|
-          local_assigns[counter_name] = counter
-          collection_of_partials.push(render_partial(partial_name, element, local_assigns))
-        end
-
-        return " " if collection_of_partials.empty?
-
-        if partial_spacer_template
-          spacer_path, spacer_name = partial_pieces(partial_spacer_template)
-          collection_of_partials.join(render("#{spacer_path}/_#{spacer_name}"))
-        else
-          collection_of_partials.join
-        end
-      end
-
-      alias_method :render_collection_of_partials, :render_partial_collection
-
-      def partial_pieces(partial_path)
-        if partial_path.include?('/')
-          return File.dirname(partial_path), File.basename(partial_path)
-        else
-          return controller.class.controller_path, partial_path
-        end
-      end
-
-      def partial_counter_name(partial_name)
-        "#{partial_variable_name(partial_name)}_counter".intern
-      end
-
-      def partial_variable_name(partial_name)
-        @@partial_variable_names ||= {}
-        @@partial_variable_names[partial_name] ||=
-          partial_name.split('/').last.split('.').first.intern
-      end
-
-      def extracting_object(partial_name, object_assigns)
-        variable_name = partial_variable_name(partial_name)
-        if object_assigns.nil?
-          controller.instance_variable_get("@#{variable_name}")
-        else
-          object_assigns
-        end
-      end
-
-      def add_counter_to_local_assigns!(partial_name, local_assigns)
-        counter_name = partial_counter_name(partial_name)
-        local_assigns[counter_name] = 1 unless local_assigns.has_key?(counter_name)
-      end
-
-      def add_object_to_local_assigns!(partial_name, local_assigns, object)
-        variable_name = partial_variable_name(partial_name)
-
-        local_assigns[:object] ||=
-          local_assigns[variable_name] ||=
-            if object.is_a?(ActionView::Base::ObjectWrapper)
-              object.value
-            else
-              object
-            end || controller.instance_variable_get("@#{variable_name}")
+        template = ActionView::PartialTemplate.new(self, partial_path, nil, local_assigns)
+        
+        spacer = partial_spacer_template ? render(:partial => partial_spacer_template) : ''
+        
+        collection.map do |element|
+          template.render_member(element)
+        end.join(spacer)
       end
   end
 end
diff --git a/actionpack/lib/action_view/template.rb b/actionpack/lib/action_view/template.rb
index a814d7b4e5..9f979e2e5d 100644
--- a/actionpack/lib/action_view/template.rb
+++ b/actionpack/lib/action_view/template.rb
@@ -2,7 +2,7 @@ module ActionView #:nodoc:
   class Template #:nodoc:
 
     attr_accessor :locals
-    attr_reader :handler, :path, :source, :extension, :filename, :path_without_extension
+    attr_reader :handler, :path, :source, :extension, :filename, :path_without_extension, :method
 
     def initialize(view, path_or_source, use_full_path, locals = {}, inline = false, inline_type = nil)
       @view = view
@@ -19,6 +19,12 @@ module ActionView #:nodoc:
         @extension = inline_type
       end
       @locals = locals || {}
+      @handler = @view.class.handler_class_for_extension(@extension).new(@view)
+    end
+    
+    def render
+      prepare!
+      @handler.render(self)
     end
 
     def source
@@ -29,13 +35,19 @@ module ActionView #:nodoc:
       @method_key ||= (@filename || @source)
     end
 
-    def handler
-      @handler ||= @view.class.handler_class_for_extension(@extension).new(@view)
-    end
-
     def base_path_for_exception
       @finder.find_base_path_for("#{@path_without_extension}.#{@extension}") || @finder.view_paths.first
     end
+    
+    def prepare!
+      @view.send :evaluate_assigns
+      @view.current_render_extension = @extension
+      
+      if @handler.compilable?
+        @handler.compile_template(self) # compile the given template, if necessary
+        @method = @view.method_names[method_key] # Set the method name for this template and run it
+      end
+    end
 
     private
 
diff --git a/actionpack/lib/action_view/template_handler.rb b/actionpack/lib/action_view/template_handler.rb
index 687c0db4c6..ec407e3fb3 100644
--- a/actionpack/lib/action_view/template_handler.rb
+++ b/actionpack/lib/action_view/template_handler.rb
@@ -13,7 +13,7 @@ module ActionView
       @view = view
     end
 
-    def render(template, local_assigns)
+    def render(template)
     end
 
     def compile(template)
diff --git a/actionpack/lib/action_view/template_handlers/compilable.rb b/actionpack/lib/action_view/template_handlers/compilable.rb
index 110c90af0a..35c74f6c51 100644
--- a/actionpack/lib/action_view/template_handlers/compilable.rb
+++ b/actionpack/lib/action_view/template_handlers/compilable.rb
@@ -24,6 +24,10 @@ module ActionView
           true
         end
       end
+      
+      def render(template)
+        @view.send :execute, template
+      end
 
       # Compile and evaluate the template's code
       def compile_template(template)
diff --git a/actionpack/test/controller/custom_handler_test.rb b/actionpack/test/controller/custom_handler_test.rb
index 932b8c15c3..cdde00cccc 100644
--- a/actionpack/test/controller/custom_handler_test.rb
+++ b/actionpack/test/controller/custom_handler_test.rb
@@ -5,9 +5,9 @@ class CustomHandler < ActionView::TemplateHandler
     @view = view
   end
 
-  def render( template, local_assigns )
-    [ template,
-      local_assigns,
+  def render( template )
+    [ template.source,
+      template.locals,
       @view ]
   end
 end
diff --git a/actionpack/test/controller/layout_test.rb b/actionpack/test/controller/layout_test.rb
index 11ad4a20af..56d7f69774 100644
--- a/actionpack/test/controller/layout_test.rb
+++ b/actionpack/test/controller/layout_test.rb
@@ -35,8 +35,8 @@ class MabView < ActionView::TemplateHandler
   def initialize(view)
   end
   
-  def render(text, locals = {})
-    text
+  def render(template)
+    template.source
   end
 end
 
diff --git a/actionpack/test/controller/new_render_test.rb b/actionpack/test/controller/new_render_test.rb
index 928c46cb6d..0f14de54b1 100644
--- a/actionpack/test/controller/new_render_test.rb
+++ b/actionpack/test/controller/new_render_test.rb
@@ -146,6 +146,14 @@ class NewRenderTestController < ActionController::Base
   def partial_collection
     render :partial => "customer", :collection => [ Customer.new("david"), Customer.new("mary") ]
   end
+  
+  def partial_collection_with_spacer
+    render :partial => "customer", :spacer_template => "partial_only", :collection => [ Customer.new("david"), Customer.new("mary") ]
+  end
+  
+  def partial_collection_with_counter
+    render :partial => "customer_counter", :collection => [ Customer.new("david"), Customer.new("mary") ]
+  end
 
   def partial_collection_with_locals
     render :partial => "customer_greeting", :collection => [ Customer.new("david"), Customer.new("mary") ], :locals => { :greeting => "Bonjour" }
@@ -712,11 +720,21 @@ EOS
     get :partial_collection
     assert_equal "Hello: davidHello: mary", @response.body
   end
-
+  
+  def test_partial_collection_with_counter
+    get :partial_collection_with_counter
+    assert_equal "david1mary2", @response.body
+  end
+  
   def test_partial_collection_with_locals
     get :partial_collection_with_locals
     assert_equal "Bonjour: davidBonjour: mary", @response.body
   end
+  
+  def test_partial_collection_with_spacer
+    get :partial_collection_with_spacer
+    assert_equal "Hello: davidonly partialHello: mary", @response.body
+  end
 
   def test_partial_collection_shorthand_with_locals
     get :partial_collection_shorthand_with_locals
diff --git a/actionpack/test/fixtures/test/_customer_counter.erb b/actionpack/test/fixtures/test/_customer_counter.erb
new file mode 100644
index 0000000000..3435979dba
--- /dev/null
+++ b/actionpack/test/fixtures/test/_customer_counter.erb
@@ -0,0 +1 @@
+<%= customer_counter.name %><%= customer_counter_counter %>
\ No newline at end of file
diff --git a/actionpack/test/template/compiled_templates_test.rb b/actionpack/test/template/compiled_templates_test.rb
index ad002e4cce..b969575235 100644
--- a/actionpack/test/template/compiled_templates_test.rb
+++ b/actionpack/test/template/compiled_templates_test.rb
@@ -109,9 +109,9 @@ class CompiledTemplateTests < Test::Unit::TestCase
 
       # All templates are rendered at t+2
       Time.expects(:now).times(windows ? 2 : 3).returns(t + 2.seconds)
-      v.send(:compile_and_render_template, @handler, ta)
-      v.send(:compile_and_render_template, @handler, tb)
-      v.send(:compile_and_render_template, @handler, ts) unless windows
+      v.send(:render_template, ta)
+      v.send(:render_template, tb)
+      v.send(:render_template, ts) unless windows
       a_n = v.method_names[@a]
       b_n = v.method_names[@b]
       s_n = v.method_names[@s] unless windows
@@ -129,9 +129,9 @@ class CompiledTemplateTests < Test::Unit::TestCase
       assert !@handler.send(:compile_template?, ta)
       assert !@handler.send(:compile_template?, tb)
       assert !@handler.send(:compile_template?, ts) unless windows
-      v.send(:compile_and_render_template, @handler, ta)
-      v.send(:compile_and_render_template, @handler, tb)
-      v.send(:compile_and_render_template, @handler, ts)  unless windows
+      v.send(:render_template, ta)
+      v.send(:render_template, tb)
+      v.send(:render_template, ts)  unless windows
       # none of the files have changed since last compile
       assert @handler.compile_time[a_n] < t + 3.seconds
       assert @handler.compile_time[b_n] < t + 3.seconds
@@ -154,9 +154,9 @@ class CompiledTemplateTests < Test::Unit::TestCase
 
       # Only the symlink template gets rendered at t+3
       Time.stubs(:now).returns(t + 3.seconds) unless windows
-      v.send(:compile_and_render_template, @handler, ta)
-      v.send(:compile_and_render_template, @handler, tb)
-      v.send(:compile_and_render_template, @handler, ts) unless windows
+      v.send(:render_template, ta)
+      v.send(:render_template, tb)
+      v.send(:render_template, ts) unless windows
       # the symlink has changed since last compile
       assert @handler.compile_time[a_n] < t + 3.seconds
       assert @handler.compile_time[b_n] < t + 3.seconds
@@ -179,9 +179,9 @@ class CompiledTemplateTests < Test::Unit::TestCase
       assert @handler.send(:compile_template?, ts) unless windows
 
       Time.expects(:now).times(windows ? 1 : 2).returns(t + 5.seconds)
-      v.send(:compile_and_render_template, @handler, ta)
-      v.send(:compile_and_render_template, @handler, tb)
-      v.send(:compile_and_render_template, @handler, ts) unless windows
+      v.send(:render_template, ta)
+      v.send(:render_template, tb)
+      v.send(:render_template, ts) unless windows
       # the file at the end of the symlink has changed since last compile
       # both the symlink and the file at the end of it should be recompiled
       assert @handler.compile_time[a_n] < t + 5.seconds
diff --git a/actionpack/test/template/template_object_test.rb b/actionpack/test/template/template_object_test.rb
new file mode 100644
index 0000000000..780df17f0c
--- /dev/null
+++ b/actionpack/test/template/template_object_test.rb
@@ -0,0 +1,62 @@
+require 'abstract_unit'
+
+class TemplateObjectTest < Test::Unit::TestCase
+  LOAD_PATH_ROOT = File.join(File.dirname(__FILE__), '..', 'fixtures')
+  ActionView::TemplateFinder.process_view_paths(LOAD_PATH_ROOT)
+  
+  class TemplateTest < Test::Unit::TestCase
+    def setup
+      @view = ActionView::Base.new(LOAD_PATH_ROOT)
+      @path = "test/hello_world.erb"
+    end
+    
+    def test_should_create_valid_template
+      template = ActionView::Template.new(@view, @path, true)
+      
+      assert_kind_of ActionView::TemplateHandlers::ERB, template.handler
+      assert_equal "test/hello_world.erb", template.path
+      assert_nil template.instance_variable_get(:"@source")
+      assert_equal "erb", template.extension
+    end
+    
+    uses_mocha 'Template preparation tests' do
+      
+      def test_should_prepare_template_properly
+        template = ActionView::Template.new(@view, @path, true)
+        view = template.instance_variable_get(:"@view")
+        
+        view.expects(:evaluate_assigns)
+        template.handler.expects(:compile_template).with(template)
+        view.expects(:method_names).returns({})
+        
+        template.prepare!
+      end
+      
+    end
+  end
+  
+  class PartialTemplateTest < Test::Unit::TestCase
+    def setup
+      @view = ActionView::Base.new(LOAD_PATH_ROOT)
+      @path = "test/partial_only"
+    end
+    
+    def test_should_create_valid_partial_template
+      template = ActionView::PartialTemplate.new(@view, @path, nil)
+      
+      assert_equal "test/_partial_only", template.path
+      assert_equal :partial_only, template.variable_name
+      
+      assert template.locals.has_key?(:object)
+      assert template.locals.has_key?(:partial_only)
+    end
+    
+    uses_mocha 'Partial template preparation tests' do
+      def test_should_prepare_on_initialization
+        ActionView::PartialTemplate.any_instance.expects(:prepare!)
+        template = ActionView::PartialTemplate.new(@view, @path, 1)
+      end
+    end
+  end
+  
+end