r1318@iwill: jeremy | 2005-06-15 01:08:22 -0700

 Ticket 1394 - Helper isolation
 r1319@iwill:  jeremy | 2005-06-15 01:10:00 -0700
 Formulate a test case for helper isolation.
 r1331@iwill:  jeremy | 2005-06-15 15:21:07 -0700
 Update changelog
 r1332@iwill:  jeremy | 2005-06-15 15:21:30 -0700
 Remove superfluous, broken layout_test
 r1333@iwill:  jeremy | 2005-06-15 15:24:10 -0700
 Use an anonymous Module to store helpers per-class instead of tossing them all in template_class.  Create a new helper module for subclasses which includes its superclass' helper module.  Remove unnecessary ActionView::Base.controller_delegate.  Update helper tests.


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

5 files changed, 109 insertions, 79 deletions

diff --git a/actionpack/CHANGELOG b/actionpack/CHANGELOG
index e6c7f58098..75e62096ab 100644
--- a/actionpack/CHANGELOG
+++ b/actionpack/CHANGELOG
@@ -1,5 +1,7 @@
 *SVN*
 
+* Ensure that helpers are only available to the controllers where they are defined and their subclasses.  #1394 [kdole@tamu.edu]
+
 * render("foo/bar") works with a layout again 
 
 * Fixed double-singularization on scaffolded pagination call (Address would be turned into Addres) #1216, #1404 [nilsga]
diff --git a/actionpack/lib/action_controller/helpers.rb b/actionpack/lib/action_controller/helpers.rb
index ef58d38021..86653e4f06 100644
--- a/actionpack/lib/action_controller/helpers.rb
+++ b/actionpack/lib/action_controller/helpers.rb
@@ -2,8 +2,26 @@ module ActionController #:nodoc:
   module Helpers #:nodoc:
     def self.append_features(base)
       super
-      base.class_eval { class << self; alias_method :inherited_without_helper, :inherited; end }
+
+      # Initialize the base module to aggregate its helpers.
+      base.class_inheritable_accessor :master_helper_module
+      base.master_helper_module = Module.new
+
+      # Extend base with class methods to declare helpers.
       base.extend(ClassMethods)
+
+      base.class_eval do
+        # Wrap inherited to create a new master helper module for subclasses.
+        class << self
+          alias_method :inherited_without_helper, :inherited
+          alias_method :inherited, :inherited_with_helper
+        end
+
+        # Wrap initialize_template_class to extend new template class
+        # instances with the master helper module.
+        alias_method :initialize_template_class_without_helper, :initialize_template_class
+        alias_method :initialize_template_class, :initialize_template_class_with_helper
+      end
     end
 
     # The template helpers serves to relieve the templates from including the same inline code again and again. It's a
@@ -32,7 +50,7 @@ module ActionController #:nodoc:
       # See ActionView::Helpers (link:classes/ActionView/Helpers.html) for more about making your own helper modules 
       # available to the templates.
       def add_template_helper(helper_module) #:nodoc:
-        template_class.class_eval "include #{helper_module}"
+        master_helper_module.module_eval "include #{helper_module}"
       end
 
       # Declare a helper:
@@ -68,7 +86,7 @@ module ActionController #:nodoc:
         end
 
         # Evaluate block in template class if given.
-        template_class.module_eval(&block) if block_given?
+        master_helper_module.module_eval(&block) if block_given?
       end
 
       # Declare a controller method as a helper.  For example,
@@ -76,7 +94,13 @@ module ActionController #:nodoc:
       #   def link_to(name, options) ... end
       # makes the link_to controller method available in the view.
       def helper_method(*methods)
-        template_class.controller_delegate(*methods)
+        methods.flatten.each do |method|
+          master_helper_module.module_eval <<-end_eval
+            def #{method}(*args, &block)
+              controller.send(%(#{method}), *args, &block)
+            end
+          end_eval
+        end
       end
 
       # Declare a controller attribute as a helper.  For example,
@@ -89,13 +113,24 @@ module ActionController #:nodoc:
       end
 
       private 
-        def inherited(child)
+        def inherited_with_helper(child)
           inherited_without_helper(child)
-          begin child.helper(child.controller_path)
+          begin
+            child.master_helper_module = Module.new
+            child.master_helper_module.send :include, master_helper_module
+            child.helper child.controller_path
           rescue MissingSourceFile => e
             raise unless e.is_missing?("helpers/#{child.controller_path}_helper")
           end
         end        
     end
+
+    private
+      # Extend the template class instance with our controller's helper module.
+      def initialize_template_class_with_helper(response)
+        returning(initialize_template_class_without_helper(response)) do
+          response.template.extend self.class.master_helper_module
+        end
+      end
   end
 end
diff --git a/actionpack/lib/action_view/base.rb b/actionpack/lib/action_view/base.rb
index 06df395cb2..9a4e00057a 100644
--- a/actionpack/lib/action_view/base.rb
+++ b/actionpack/lib/action_view/base.rb
@@ -144,16 +144,6 @@ module ActionView #:nodoc:
       end
     end
 
-    def self.controller_delegate(*methods)#:nodoc:
-      methods.flatten.each do |method|
-        class_eval <<-end_eval
-          def #{method}(*args, &block)
-            controller.send(%(#{method}), *args, &block)
-          end
-        end_eval
-      end
-    end
-
     def self.register_template_handler(extension, klass)
       @@template_handlers[extension] = klass
     end
@@ -305,4 +295,4 @@ module ActionView #:nodoc:
   end
 end
 
-require 'action_view/template_error'
\ No newline at end of file
+require 'action_view/template_error'
diff --git a/actionpack/test/controller/helper_test.rb b/actionpack/test/controller/helper_test.rb
index 147d36501b..77a3fc6115 100644
--- a/actionpack/test/controller/helper_test.rb
+++ b/actionpack/test/controller/helper_test.rb
@@ -9,7 +9,7 @@ end
 module Fun
   class GamesController < ActionController::Base
     def render_hello_world
-      render_template "hello: <%= stratego %>"
+      render :inline => "hello: <%= stratego %>"
     end
 
     def rescue_action(e) raise end
@@ -23,7 +23,6 @@ module LocalAbcHelper
 end
 
 class HelperTest < Test::Unit::TestCase
-
   def setup
     # Increment symbol counter.
     @symbol = (@@counter ||= 'A0').succ!.dup
@@ -50,7 +49,7 @@ class HelperTest < Test::Unit::TestCase
 
 
   def test_deprecated_helper
-    assert_equal helper_methods, missing_methods
+    assert_equal expected_helper_methods, missing_methods
     assert_nothing_raised { @controller_class.helper TestHelper }
     assert_equal [], missing_methods
   end
@@ -58,13 +57,13 @@ class HelperTest < Test::Unit::TestCase
   def test_declare_helper
     require 'abc_helper'
     self.test_helper = AbcHelper
-    assert_equal helper_methods, missing_methods
+    assert_equal expected_helper_methods, missing_methods
     assert_nothing_raised { @controller_class.helper :abc }
     assert_equal [], missing_methods
   end
 
   def test_declare_missing_helper
-    assert_equal helper_methods, missing_methods
+    assert_equal expected_helper_methods, missing_methods
     assert_raise(MissingSourceFile) { @controller_class.helper :missing }
   end
 
@@ -78,11 +77,11 @@ class HelperTest < Test::Unit::TestCase
     assert_nothing_raised {
       @controller_class.helper { def block_helper_method; end }
     }
-    assert template_methods.include?('block_helper_method')
+    assert master_helper_methods.include?('block_helper_method')
   end
 
   def test_helper_block_include
-    assert_equal helper_methods, missing_methods
+    assert_equal expected_helper_methods, missing_methods
     assert_nothing_raised {
       @controller_class.helper { include TestHelper }
     }
@@ -91,13 +90,13 @@ class HelperTest < Test::Unit::TestCase
 
   def test_helper_method
     assert_nothing_raised { @controller_class.helper_method :delegate_method }
-    assert template_methods.include?('delegate_method')
+    assert master_helper_methods.include?('delegate_method')
   end
 
   def test_helper_attr
     assert_nothing_raised { @controller_class.helper_attr :delegate_attr }
-    assert template_methods.include?('delegate_attr')
-    assert template_methods.include?('delegate_attr=')
+    assert master_helper_methods.include?('delegate_attr')
+    assert master_helper_methods.include?('delegate_attr=')
   end
 
   def test_helper_for_nested_controller
@@ -109,9 +108,17 @@ class HelperTest < Test::Unit::TestCase
   end
 
   private
-    def helper_methods;   TestHelper.instance_methods      end
-    def template_methods; @template_class.instance_methods  end
-    def missing_methods;  helper_methods - template_methods end
+    def expected_helper_methods
+      TestHelper.instance_methods
+    end
+
+    def master_helper_methods
+      @controller_class.master_helper_module.instance_methods
+    end
+
+    def missing_methods
+      expected_helper_methods - master_helper_methods
+    end
 
     def test_helper=(helper_module)
       old_verbose, $VERBOSE = $VERBOSE, nil
@@ -119,3 +126,48 @@ class HelperTest < Test::Unit::TestCase
       $VERBOSE = old_verbose
     end
 end
+
+
+class IsolatedHelpersTest < Test::Unit::TestCase
+  class A < ActionController::Base
+    def index
+      render :inline => '<%= shout %>'
+    end
+
+    def rescue_action(e) raise end
+  end
+
+  class B < A
+    helper { def shout; 'B' end }
+
+    def index
+      render :inline => '<%= shout %>'
+    end
+  end
+
+  class C < A
+    helper { def shout; 'C' end }
+
+    def index
+      render :inline => '<%= shout %>'
+    end
+  end
+
+  def setup
+    @request    = ActionController::TestRequest.new
+    @response   = ActionController::TestResponse.new
+    @request.action = 'index'
+  end
+
+  def test_helper_in_a
+    assert_raise(NameError) { A.process(@request, @response) }
+  end
+
+  def test_helper_in_b
+    assert_equal 'B', B.process(@request, @response).body
+  end
+
+  def test_helper_in_c
+    assert_equal 'C', C.process(@request, @response).body
+  end
+end
diff --git a/actionpack/test/controller/layout_test.rb b/actionpack/test/controller/layout_test.rb
deleted file mode 100644
index f652453ebd..0000000000
--- a/actionpack/test/controller/layout_test.rb
+++ /dev/null
@@ -1,49 +0,0 @@
-require File.dirname(__FILE__) + '/../abstract_unit'
-
-class TestLayoutController < ActionController::Base
-  layout "layouts/standard"
-  
-  def hello_world
-  end
-  
-  def hello_world_outside_layout
-  end
-
-  def rescue_action(e) raise end
-end
-
-class ChildWithoutTestLayoutController < TestLayoutController
-  layout nil
-
-  def hello_world
-  end
-end
-
-class ChildWithOtherTestLayoutController < TestLayoutController
-  layout nil
-
-  def hello_world
-  end
-end
-
-class RenderTest < Test::Unit::TestCase
-  def setup
-    @request    = ActionController::TestRequest.new
-    @response   = ActionController::TestResponse.new
-
-    @request.host = "www.nextangle.com"
-  end
-
-  def test_layout_rendering
-    @request.action = "hello_world"
-    response = process_request
-    assert_equal "200 OK", response.headers["Status"]
-    assert_equal "layouts/standard", response.template.template_name
-  end
-
-
-  private
-    def process_request
-      TestLayoutController.process(@request, @response)
-    end
-end
\ No newline at end of file