Writing comments to AbstractController

8 files changed, 191 insertions, 72 deletions

diff --git a/actionpack/lib/action_controller/abstract/base.rb b/actionpack/lib/action_controller/abstract/base.rb
index 306f3d2ccb..d2cf4b1aad 100644
--- a/actionpack/lib/action_controller/abstract/base.rb
+++ b/actionpack/lib/action_controller/abstract/base.rb
@@ -17,36 +17,58 @@ module AbstractController
 
     class << self
       attr_reader :abstract
+      alias_method :abstract?, :abstract
 
+      # Define a controller as abstract. See internal_methods for more
+      # details.
       def abstract!
         @abstract = true
       end
 
-      alias_method :abstract?, :abstract
-
       def inherited(klass)
-        ::AbstractController::Base.subclasses << klass.to_s
+        ::AbstractController::Base.descendants << klass.to_s
         super
       end
 
-      def subclasses
-        @subclasses ||= []
+      # A list of all descendents of AbstractController::Base. This is
+      # useful for initializers which need to add behavior to all controllers.
+      def descendants
+        @descendants ||= []
       end
 
+      # A list of all internal methods for a controller. This finds the first
+      # abstract superclass of a controller, and gets a list of all public
+      # instance methods on that abstract class. Public instance methods of
+      # a controller would normally be considered action methods, so we
+      # are removing those methods on classes declared as abstract
+      # (ActionController::Http and ActionController::Base are defined
+      # as abstract)
       def internal_methods
         controller = self
         controller = controller.superclass until controller.abstract?
         controller.public_instance_methods(true)
       end
 
-      def process(action)
-        new.process(action.to_s)
-      end
-
+      # The list of hidden actions to an empty Array. Defaults to an
+      # empty Array. This can be modified by other modules or subclasses
+      # to specify particular actions as hidden.
+      #
+      # ==== Returns
+      # Array[String]:: An array of method names that should not be
+      #                 considered actions.
       def hidden_actions
         []
       end
 
+      # A list of method names that should be considered actions. This
+      # includes all public instance methods on a controller, less
+      # any internal methods (see #internal_methods), adding back in
+      # any methods that are internal, but still exist on the class
+      # itself. Finally, #hidden_actions are removed.
+      #
+      # ==== Returns
+      # Array[String]:: A list of all methods that should be considered
+      #                 actions.
       def action_methods
         @action_methods ||=
           # All public instance methods of this class, including ancestors
@@ -62,6 +84,14 @@ module AbstractController
 
     abstract!
 
+    # Calls the action going through the entire action dispatch stack.
+    # 
+    # The actual method that is called is determined by calling
+    # #method_for_action. If no method can handle the action, then an
+    # ActionNotFound error is raised.
+    #
+    # ==== Returns
+    # self
     def process(action)
       @_action_name = action_name = action.to_s
 
@@ -74,33 +104,68 @@ module AbstractController
     end
 
   private
+    # See AbstractController::Base.action_methods
     def action_methods
       self.class.action_methods
     end
 
-    def action_method?(action)
-      action_methods.include?(action)
+    # Returns true if the name can be considered an action. This can
+    # be overridden in subclasses to modify the semantics of what
+    # can be considered an action.
+    #
+    # ==== Parameters
+    # name<String>:: The name of an action to be tested
+    #
+    # ==== Returns
+    # TrueClass, FalseClass
+    def action_method?(name)
+      action_methods.include?(name)
     end
 
-    # It is possible for respond_to?(action_name) to be false and
-    # respond_to?(:action_missing) to be false if respond_to_action?
-    # is overridden in a subclass. For instance, ActionController::Base
-    # overrides it to include the case where a template matching the
-    # action_name is found.
+    # Call the action. Override this in a subclass to modify the
+    # behavior around processing an action. This, and not #process,
+    # is the intended way to override action dispatching.
     def process_action(method_name)
       send_action(method_name)
     end
 
+    # Actually call the method associated with the action. Override
+    # this method if you wish to change how action methods are called,
+    # not to add additional behavior around it. For example, you would
+    # override #send_action if you want to inject arguments into the
+    # method.
     alias send_action send
 
+    # If the action name was not found, but a method called "action_missing"
+    # was found, #method_for_action will return "_handle_action_missing".
+    # This method calls #action_missing with the current action name.
     def _handle_action_missing
       action_missing(@_action_name)
     end
 
-    # Override this to change the conditions that will raise an
-    # ActionNotFound error. If you accept a difference case,
-    # you must handle it by also overriding process_action and
-    # handling the case.
+    # Takes an action name and returns the name of the method that will
+    # handle the action. In normal cases, this method returns the same
+    # name as it receives. By default, if #method_for_action receives
+    # a name that is not an action, it will look for an #action_missing
+    # method and return "_handle_action_missing" if one is found.
+    #
+    # Subclasses may override this method to add additional conditions
+    # that should be considered an action. For instance, an HTTP controller
+    # with a template matching the action name is considered to exist.
+    #
+    # If you override this method to handle additional cases, you may
+    # also provide a method (like _handle_method_missing) to handle
+    # the case.
+    #
+    # If none of these conditions are true, and method_for_action
+    # returns nil, an ActionNotFound exception will be raised.
+    #
+    # ==== Parameters
+    # action_name<String>:: An action name to find a method name for
+    #
+    # ==== Returns
+    # String:: The name of the method that handles the action
+    # nil::    No method name could be found. Raise ActionNotFound.
     def method_for_action(action_name)
       if action_method?(action_name) then action_name
       elsif respond_to?(:action_missing, true) then "_handle_action_missing"
diff --git a/actionpack/lib/action_controller/abstract/benchmarker.rb b/actionpack/lib/action_controller/abstract/benchmarker.rb
index 07294cede3..58e9564c2f 100644
--- a/actionpack/lib/action_controller/abstract/benchmarker.rb
+++ b/actionpack/lib/action_controller/abstract/benchmarker.rb
@@ -5,6 +5,16 @@ module AbstractController
     include Logger
 
     module ClassMethods
+      # Execute the passed in block, timing the duration of the block in ms.
+      #
+      # ==== Parameters
+      # title<#to_s>:: The title of block to benchmark
+      # log_level<Integer>:: A valid log level. Defaults to Logger::DEBUG
+      # use_silence<TrueClass, FalseClass>:: Whether or not to silence the
+      #   logger for the duration of the block.
+      #
+      # ==== Returns
+      # Object:: The result of the block
       def benchmark(title, log_level = ::Logger::DEBUG, use_silence = true)
         if logger && logger.level >= log_level
           result = nil
diff --git a/actionpack/lib/action_controller/abstract/callbacks.rb b/actionpack/lib/action_controller/abstract/callbacks.rb
index fd7e5ebfda..0d5161c80e 100644
--- a/actionpack/lib/action_controller/abstract/callbacks.rb
+++ b/actionpack/lib/action_controller/abstract/callbacks.rb
@@ -2,12 +2,17 @@ module AbstractController
   module Callbacks
     extend ActiveSupport::Concern
 
+    # Uses ActiveSupport::NewCallbacks as the base functionality. For
+    # more details on the whole callback system, read the documentation
+    # for ActiveSupport::NewCallbacks.
     include ActiveSupport::NewCallbacks
 
     included do
       define_callbacks :process_action, "response_body"
     end
 
+    # Override AbstractController::Base's process_action to run the
+    # process_action callbacks around the normal behavior.
     def process_action(method_name)
       _run_process_action_callbacks(method_name) do
         super
@@ -15,6 +20,17 @@ module AbstractController
     end
 
     module ClassMethods
+      # If :only or :accept are used, convert the options into the
+      # primitive form (:per_key) used by ActiveSupport::Callbacks.
+      # The basic idea is that :only => :index gets converted to
+      # :if => proc {|c| c.action_name == "index" }, but that the
+      # proc is only evaluated once per action for the lifetime of
+      # a Rails process.
+      #
+      # ==== Options
+      # :only<#to_s>:: The callback should be run only for this action
+      # :except<#to_s>:: The callback should be run for all actions
+      #   except this action
       def _normalize_callback_options(options)
         if only = options[:only]
           only = Array(only).map {|o| "action_name == '#{o}'"}.join(" || ")
@@ -26,41 +42,69 @@ module AbstractController
         end
       end
 
+      # Skip before, after, and around filters matching any of the names
+      #
+      # ==== Parameters
+      # *names<Object>:: A list of valid names that could be used for
+      #   callbacks. Note that skipping uses Ruby equality, so it's
+      #   impossible to skip a callback defined using an anonymous proc
+      #   using #skip_filter
       def skip_filter(*names, &blk)
-        skip_before_filter(*names, &blk)
-        skip_after_filter(*names, &blk)
-        skip_around_filter(*names, &blk)
+        skip_before_filter(*names)
+        skip_after_filter(*names)
+        skip_around_filter(*names)
       end
 
-      def _insert_callbacks(names, block)
-        options = names.last.is_a?(Hash) ? names.pop : {}
+      # Take callback names and an optional callback proc, normalize them,
+      # then call the block with each callback. This allows us to abstract
+      # the normalization across several methods that use it.
+      #
+      # ==== Parameters
+      # callbacks<Array[*Object, Hash]>:: A list of callbacks, with an optional
+      #   options hash as the last parameter.
+      # block<Proc>:: A proc that should be added to the callbacks.
+      #
+      # ==== Block Parameters
+      # name<Symbol>:: The callback to be added
+      # options<Hash>:: A list of options to be used when adding the callback
+      def _insert_callbacks(callbacks, block)
+        options = callbacks.last.is_a?(Hash) ? callbacks.pop : {}
         _normalize_callback_options(options)
-        names.push(block) if block
-        names.each do |name|
-          yield name, options
+        callbacks.push(block) if block
+        callbacks.each do |callback|
+          yield callback, options
         end
       end
 
+      # set up before_filter, prepend_before_filter, skip_before_filter, etc.
+      # for each of before, after, and around.
       [:before, :after, :around].each do |filter|
         class_eval <<-RUBY_EVAL, __FILE__, __LINE__ + 1
+          # Append a before, after or around filter. See _insert_callbacks
+          # for details on the allowed parameters.
           def #{filter}_filter(*names, &blk)
             _insert_callbacks(names, blk) do |name, options|
               set_callback(:process_action, :#{filter}, name, options)
             end
           end
 
+          # Prepend a before, after or around filter. See _insert_callbacks
+          # for details on the allowed parameters.
           def prepend_#{filter}_filter(*names, &blk)
             _insert_callbacks(names, blk) do |name, options|
               set_callback(:process_action, :#{filter}, name, options.merge(:prepend => true))
             end
           end
 
+          # Skip a before, after or around filter. See _insert_callbacks
+          # for details on the allowed parameters.
           def skip_#{filter}_filter(*names, &blk)
             _insert_callbacks(names, blk) do |name, options|
               skip_callback(:process_action, :#{filter}, name, options)
             end
           end
 
+          # *_filter is the same as append_*_filter
           alias_method :append_#{filter}_filter, :#{filter}_filter
         RUBY_EVAL
       end
diff --git a/actionpack/lib/action_controller/new_base/base.rb b/actionpack/lib/action_controller/new_base/base.rb
index d7b65d37fa..e8fc153578 100644
--- a/actionpack/lib/action_controller/new_base/base.rb
+++ b/actionpack/lib/action_controller/new_base/base.rb
@@ -39,7 +39,7 @@ module ActionController
     # TODO: Extract into its own module
     # This should be moved together with other normalizing behavior
     module ImplicitRender
-      def send_action(method_name)
+      def send_action(*)
         ret = super
         default_render unless performed?
         ret
diff --git a/actionpack/test/abstract_controller/abstract_controller_test.rb b/actionpack/test/abstract_controller/abstract_controller_test.rb
index 9c028e7d1e..c7eaaeb4ba 100644
--- a/actionpack/test/abstract_controller/abstract_controller_test.rb
+++ b/actionpack/test/abstract_controller/abstract_controller_test.rb
@@ -19,7 +19,7 @@ module AbstractController
     
     class TestBasic < ActiveSupport::TestCase
       test "dispatching works" do
-        result = Me.process(:index)
+        result = Me.new.process(:index)
         assert_equal "Hello world", result.response_body
       end
     end
@@ -68,27 +68,27 @@ module AbstractController
     
     class TestRenderer < ActiveSupport::TestCase
       test "rendering templates works" do
-        result = Me2.process(:index)
+        result = Me2.new.process(:index)
         assert_equal "Hello from index.erb", result.response_body
       end
       
       test "rendering passes ivars to the view" do
-        result = Me2.process(:action_with_ivars)
+        result = Me2.new.process(:action_with_ivars)
         assert_equal "Hello from index_with_ivars.erb", result.response_body
       end
       
       test "rendering with no template name" do
-        result = Me2.process(:naked_render)
+        result = Me2.new.process(:naked_render)
         assert_equal "Hello from naked_render.erb", result.response_body
       end
 
       test "rendering to a rack body" do
-        result = Me2.process(:rendering_to_body)
+        result = Me2.new.process(:rendering_to_body)
         assert_equal "Hello from naked_render.erb", result.response_body
       end
 
       test "rendering to a string" do
-        result = Me2.process(:rendering_to_string)
+        result = Me2.new.process(:rendering_to_string)
         assert_equal "Hello from naked_render.erb", result.response_body
       end
     end
@@ -120,12 +120,12 @@ module AbstractController
     
     class TestPrefixedViews < ActiveSupport::TestCase
       test "templates are located inside their 'prefix' folder" do
-        result = Me3.process(:index)
+        result = Me3.new.process(:index)
         assert_equal "Hello from me3/index.erb", result.response_body
       end
 
       test "templates included their format" do
-        result = Me3.process(:formatted)
+        result = Me3.new.process(:formatted)
         assert_equal "Hello from me3/formatted.html.erb", result.response_body
       end
     end
@@ -173,7 +173,7 @@ module AbstractController
     
     class TestLayouts < ActiveSupport::TestCase
       test "layouts are included" do
-        result = Me4.process(:index)
+        result = Me4.new.process(:index)
         assert_equal "Me4 Enter : Hello from me4/index.erb : Exit", result.response_body
       end
     end
@@ -210,7 +210,7 @@ module AbstractController
     class TestRespondToAction < ActiveSupport::TestCase
       
       def assert_dispatch(klass, body = "success", action = :index)
-        response = klass.process(action).response_body
+        response = klass.new.process(action).response_body
         assert_equal body, response
       end
       
@@ -219,7 +219,7 @@ module AbstractController
       end
       
       test "raises ActionNotFound when method does not exist and action_missing is not defined" do
-        assert_raise(ActionNotFound) { DefaultRespondToActionController.process(:fail) }
+        assert_raise(ActionNotFound) { DefaultRespondToActionController.new.process(:fail) }
       end
       
       test "dispatches to action_missing when method does not exist and action_missing is defined" do
@@ -231,7 +231,7 @@ module AbstractController
       end
       
       test "raises ActionNotFound if method is defined but respond_to_action? returns false" do
-        assert_raise(ActionNotFound) { RespondToActionController.process(:fail) }
+        assert_raise(ActionNotFound) { RespondToActionController.new.process(:fail) }
       end
     end
     
diff --git a/actionpack/test/abstract_controller/callbacks_test.rb b/actionpack/test/abstract_controller/callbacks_test.rb
index 32450257f0..817f60f7d1 100644
--- a/actionpack/test/abstract_controller/callbacks_test.rb
+++ b/actionpack/test/abstract_controller/callbacks_test.rb
@@ -21,7 +21,7 @@ module AbstractController
     
     class TestCallbacks < ActiveSupport::TestCase
       test "basic callbacks work" do
-        result = Callback1.process(:index)
+        result = Callback1.new.process(:index)
         assert_equal "Hello world", result.response_body
       end
     end
@@ -52,17 +52,17 @@ module AbstractController
     
     class TestCallbacks < ActiveSupport::TestCase
       test "before_filter works" do
-        result = Callback2.process(:index)
+        result = Callback2.new.process(:index)
         assert_equal "Hello world", result.response_body
       end
       
       test "after_filter works" do
-        result = Callback2.process(:index)
+        result = Callback2.new.process(:index)
         assert_equal "Goodbye", result.instance_variable_get("@second")
       end
       
       test "around_filter works" do
-        result = Callback2.process(:index)
+        result = Callback2.new.process(:index)
         assert_equal "FIRSTSECOND", result.instance_variable_get("@aroundz")
       end
     end
@@ -83,12 +83,12 @@ module AbstractController
 
     class TestCallbacks < ActiveSupport::TestCase
       test "before_filter works with procs" do
-        result = Callback3.process(:index)
+        result = Callback3.new.process(:index)
         assert_equal "Hello world", result.response_body
       end
       
       test "after_filter works with procs" do
-        result = Callback3.process(:index)
+        result = Callback3.new.process(:index)
         assert_equal "Goodbye", result.instance_variable_get("@second")
       end      
     end
@@ -118,17 +118,17 @@ module AbstractController
     
     class TestCallbacks < ActiveSupport::TestCase
       test "when :only is specified, a before filter is triggered on that action" do
-        result = CallbacksWithConditions.process(:index)
+        result = CallbacksWithConditions.new.process(:index)
         assert_equal "Hello, World", result.response_body
       end
       
       test "when :only is specified, a before filter is not triggered on other actions" do
-        result = CallbacksWithConditions.process(:sekrit_data)
+        result = CallbacksWithConditions.new.process(:sekrit_data)
         assert_equal "true", result.response_body
       end
       
       test "when :except is specified, an after filter is not triggered on that action" do
-        result = CallbacksWithConditions.process(:index)
+        result = CallbacksWithConditions.new.process(:index)
         assert_nil result.instance_variable_get("@authenticated")
       end
     end
@@ -158,17 +158,17 @@ module AbstractController
     
     class TestCallbacks < ActiveSupport::TestCase
       test "when :only is specified with an array, a before filter is triggered on that action" do
-        result = CallbacksWithArrayConditions.process(:index)
+        result = CallbacksWithArrayConditions.new.process(:index)
         assert_equal "Hello, World", result.response_body
       end
       
       test "when :only is specified with an array, a before filter is not triggered on other actions" do
-        result = CallbacksWithArrayConditions.process(:sekrit_data)
+        result = CallbacksWithArrayConditions.new.process(:sekrit_data)
         assert_equal "true", result.response_body
       end
       
       test "when :except is specified with an array, an after filter is not triggered on that action" do
-        result = CallbacksWithArrayConditions.process(:index)
+        result = CallbacksWithArrayConditions.new.process(:index)
         assert_nil result.instance_variable_get("@authenticated")
       end
     end    
@@ -183,12 +183,12 @@ module AbstractController
     
     class TestCallbacks < ActiveSupport::TestCase
       test "when a callback is modified in a child with :only, it works for the :only action" do
-        result = ChangedConditions.process(:index)
+        result = ChangedConditions.new.process(:index)
         assert_equal "Hello world", result.response_body
       end
       
       test "when a callback is modified in a child with :only, it does not work for other actions" do
-        result = ChangedConditions.process(:not_index)
+        result = ChangedConditions.new.process(:not_index)
         assert_equal "", result.response_body        
       end
     end
@@ -207,7 +207,7 @@ module AbstractController
     
     class TestHalting < ActiveSupport::TestCase
       test "when a callback sets the response body, the action should not be invoked" do
-        result = SetsResponseBody.process(:index)
+        result = SetsResponseBody.new.process(:index)
         assert_equal "Success", result.response_body
       end
     end
diff --git a/actionpack/test/abstract_controller/helper_test.rb b/actionpack/test/abstract_controller/helper_test.rb
index f91aefe606..0a2535f834 100644
--- a/actionpack/test/abstract_controller/helper_test.rb
+++ b/actionpack/test/abstract_controller/helper_test.rb
@@ -34,7 +34,7 @@ module AbstractController
     
     class TestHelpers < ActiveSupport::TestCase
       def test_helpers
-        result = MyHelpers1.process(:index)
+        result = MyHelpers1.new.process(:index)
         assert_equal "Hello World : Included", result.response_body
       end
     end
diff --git a/actionpack/test/abstract_controller/layouts_test.rb b/actionpack/test/abstract_controller/layouts_test.rb
index d3440c3de0..4b66f063f3 100644
--- a/actionpack/test/abstract_controller/layouts_test.rb
+++ b/actionpack/test/abstract_controller/layouts_test.rb
@@ -142,7 +142,7 @@ module AbstractControllerTests
     end
     
     # TODO Move to bootloader
-    AbstractController::Base.subclasses.each do |klass|
+    AbstractController::Base.descendants.each do |klass|
       klass = klass.constantize
       next unless klass < AbstractController::Layouts
       klass.class_eval do
@@ -152,70 +152,70 @@ module AbstractControllerTests
     
     class TestBase < ActiveSupport::TestCase
       test "when no layout is specified, and no default is available, render without a layout" do
-        result = Blank.process(:index)
+        result = Blank.new.process(:index)
         assert_equal "Hello blank!", result.response_body
       end
       
       test "when layout is specified as a string, render with that layout" do
-        result = WithString.process(:index)
+        result = WithString.new.process(:index)
         assert_equal "With String Hello string!", result.response_body
       end
       
       test "when layout is specified as a string, but the layout is missing, raise an exception" do
-        assert_raises(ActionView::MissingTemplate) { WithMissingLayout.process(:index) }
+        assert_raises(ActionView::MissingTemplate) { WithMissingLayout.new.process(:index) }
       end
       
       test "when layout is specified as false, do not use a layout" do
-        result = WithFalseLayout.process(:index)
+        result = WithFalseLayout.new.process(:index)
         assert_equal "Hello false!", result.response_body
       end
       
       test "when layout is specified as nil, do not use a layout" do
-        result = WithNilLayout.process(:index)
+        result = WithNilLayout.new.process(:index)
         assert_equal "Hello nil!", result.response_body
       end
       
       test "when layout is specified as a symbol, call the requested method and use the layout returned" do
-        result = WithSymbol.process(:index)
+        result = WithSymbol.new.process(:index)
         assert_equal "OMGHI2U Hello symbol!", result.response_body
       end
       
       test "when layout is specified as a symbol and the method returns nil, don't use a layout" do
-        result = WithSymbolReturningNil.process(:index)
+        result = WithSymbolReturningNil.new.process(:index)
         assert_equal "Hello nilz!", result.response_body
       end
       
       test "when the layout is specified as a symbol and the method doesn't exist, raise an exception" do
-        assert_raises(NoMethodError, /:nilz/) { WithSymbolAndNoMethod.process(:index) }
+        assert_raises(NoMethodError, /:nilz/) { WithSymbolAndNoMethod.new.process(:index) }
       end
       
       test "when the layout is specified as a symbol and the method returns something besides a string/false/nil, raise an exception" do
-        assert_raises(ArgumentError) { WithSymbolReturningObj.process(:index) }
+        assert_raises(ArgumentError) { WithSymbolReturningObj.new.process(:index) }
       end
       
       test "when a child controller does not have a layout, use the parent controller layout" do
-        result = WithStringChild.process(:index)
+        result = WithStringChild.new.process(:index)
         assert_equal "With String Hello string!", result.response_body
       end
       
       test "when a child controller has specified a layout, use that layout and not the parent controller layout" do
-        result = WithStringOverriddenChild.process(:index)
+        result = WithStringOverriddenChild.new.process(:index)
         assert_equal "With Override Hello string!", result.response_body
       end
       
       test "when a child controller has an implied layout, use that layout and not the parent controller layout" do
-        result = WithStringImpliedChild.process(:index)
+        result = WithStringImpliedChild.new.process(:index)
         assert_equal "With Implied Hello string!", result.response_body
       end
       
       test "when a child controller specifies layout nil, do not use the parent layout" do
-        result = WithNilChild.process(:index)
+        result = WithNilChild.new.process(:index)
         assert_equal "Hello string!", result.response_body
       end
             
       test "when a grandchild has no layout specified, the child has an implied layout, and the " \
         "parent has specified a layout, use the child controller layout" do
-          result = WithChildOfImplied.process(:index)
+          result = WithChildOfImplied.new.process(:index)
           assert_equal "With Implied Hello string!", result.response_body
       end