simplify the Middleware constructor

We should do the hard work outside the constructor.  Also fix the tests
to not directly construct middleware objects, but to go through the
stack object.

actionpack/lib/action_controller/metal.rb

@@ -11,18 +11,16 @@ module ActionController
   #
   class MiddlewareStack < ActionDispatch::MiddlewareStack #:nodoc:
     class Middleware < ActionDispatch::MiddlewareStack::Middleware #:nodoc:
-      def initialize(klass, *args, &block)
-        options = args.extract_options!
-        @only   = Array(options.delete(:only)).map(&:to_s)
-        @except = Array(options.delete(:except)).map(&:to_s)
-        args << options unless options.empty?
-        super
+      def initialize(klass, args, only, except, block)
+        @only = only
+        @except = except
+        super(klass, args, block)
       end
 
       def valid?(action)
-        if @only.present?
+        if @only.any?
           @only.include?(action)
-        elsif @except.present?
+        elsif @except.any?
           !@except.include?(action)
         else
           true
@@ -37,6 +35,17 @@ def build(action, app = Proc.new)
         middleware.valid?(action) ? middleware.build(a) : a
       end
     end
+
+    private
+
+    def build_middleware(klass, args, block)
+      options = args.extract_options!
+      only   = Array(options.delete(:only)).map(&:to_s)
+      except = Array(options.delete(:except)).map(&:to_s)
+      args << options unless options.empty?
+
+      Middleware.new(klass, args, only, except, block)
+    end
   end
 
   # <tt>ActionController::Metal</tt> is the simplest possible controller, providing a

actionpack/lib/action_dispatch/middleware/stack.rb

@@ -6,7 +6,7 @@ class MiddlewareStack
     class Middleware
       attr_reader :args, :block, :name, :classcache
 
-      def initialize(klass_or_name, *args, &block)
+      def initialize(klass_or_name, args, block)
         @klass = nil
 
         if klass_or_name.respond_to?(:name)
@@ -75,19 +75,17 @@ def [](i)
       middlewares[i]
     end
 
-    def unshift(*args, &block)
-      middleware = self.class::Middleware.new(*args, &block)
-      middlewares.unshift(middleware)
+    def unshift(klass, *args, &block)
+      middlewares.unshift(build_middleware(klass, args, block))
     end
 
     def initialize_copy(other)
       self.middlewares = other.middlewares.dup
     end
 
-    def insert(index, *args, &block)
+    def insert(index, klass, *args, &block)
       index = assert_index(index, :before)
-      middleware = self.class::Middleware.new(*args, &block)
-      middlewares.insert(index, middleware)
+      middlewares.insert(index, build_middleware(klass, args, block))
     end
 
     alias_method :insert_before, :insert
@@ -107,9 +105,8 @@ def delete(target)
       middlewares.delete target
     end
 
-    def use(*args, &block)
-      middleware = self.class::Middleware.new(*args, &block)
-      middlewares.push(middleware)
+    def use(klass, *args, &block)
+      middlewares.push(build_middleware(klass, args, block))
     end
 
     def build(app = Proc.new)
@@ -123,5 +120,11 @@ def assert_index(index, where)
       raise "No such middleware to insert #{where}: #{index.inspect}" unless i
       i
     end
+
+    private
+
+    def build_middleware(klass, args, block)
+      Middleware.new(klass, args, block)
+    end
   end
 end

actionpack/test/dispatch/middleware_stack/middleware_test.rb

@@ -12,65 +12,50 @@ class Omg; end
       }.each do |name, klass|
 
         define_method("test_#{name}_klass") do
-          mw = Middleware.new klass
-          assert_equal klass, mw.klass
+          stack = ActionDispatch::MiddlewareStack.new
+          stack.use klass
+          assert_equal klass, stack.first.klass
         end
 
         define_method("test_#{name}_==") do
-          mw1 = Middleware.new klass
-          mw2 = Middleware.new klass
-          assert_equal mw1, mw2
+          stack = ActionDispatch::MiddlewareStack.new
+          stack.use klass
+          stack.use klass
+          assert_equal 2, stack.size
+          assert_equal stack.first, stack.last
         end
 
       end
 
+      attr_reader :stack
+
+      def setup
+        @stack = ActionDispatch::MiddlewareStack.new
+      end
+
       def test_string_class
-        mw = Middleware.new Omg.name
-        assert_equal Omg, mw.klass
+        stack = ActionDispatch::MiddlewareStack.new
+        stack.use Omg.name
+        assert_equal Omg, stack.first.klass
       end
 
       def test_double_equal_works_with_classes
         k = Class.new
-        mw = Middleware.new k
-        assert_operator mw, :==, k
+        stack.use k
+        assert_operator stack.first, :==, k
 
-        result = mw != Class.new
+        result = stack.first != Class.new
         assert result, 'middleware should not equal other anon class'
       end
 
       def test_double_equal_works_with_strings
-        mw = Middleware.new Omg
-        assert_operator mw, :==, Omg.name
+        stack.use Omg
+        assert_operator stack.first, :==, Omg.name
       end
 
       def test_double_equal_normalizes_strings
-        mw = Middleware.new Omg
-        assert_operator mw, :==, "::#{Omg.name}"
-      end
-
-      def test_middleware_loads_classnames_from_cache
-        mw = Class.new(Middleware) {
-          attr_accessor :classcache
-        }.new(Omg.name)
-
-        fake_cache    = { mw.name => Omg }
-        mw.classcache = fake_cache
-
-        assert_equal Omg, mw.klass
-
-        fake_cache[mw.name] = Middleware
-        assert_equal Middleware, mw.klass
-      end
-
-      def test_middleware_always_returns_class
-        mw = Class.new(Middleware) {
-          attr_accessor :classcache
-        }.new(Omg)
-
-        fake_cache    = { mw.name => Middleware }
-        mw.classcache = fake_cache
-
-        assert_equal Omg, mw.klass
+        stack.use Omg
+        assert_operator stack.first, :==, "::#{Omg.name}"
       end
     end
   end