Skip to content

Commit

Permalink
Using strings or symbols for middleware class names is deprecated.
Browse files Browse the repository at this point in the history
Convert things like this:

  middleware.use "Foo::Bar"

to this:

  middleware.use Foo::Bar
  • Loading branch information
tenderlove committed Aug 7, 2015
1 parent 435b224 commit 83b767c
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 50 deletions.
9 changes: 9 additions & 0 deletions actionpack/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
* Using strings or symbols for middleware class names is deprecated. Convert
things like this:

middleware.use "Foo::Bar"

to this:

middleware.use Foo::Bar

* ActionController::TestSession now accepts a default value as well as
a block for generating a default value based off the key provided.

Expand Down
3 changes: 1 addition & 2 deletions actionpack/lib/action_controller/metal.rb
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,7 @@ def build_middleware(klass, args, block)
list = except
end


Middleware.new(klass, args, list, strategy, block)
Middleware.new(get_class(klass), args, list, strategy, block)
end
end

Expand Down
43 changes: 25 additions & 18 deletions actionpack/lib/action_dispatch/middleware/stack.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,15 @@
module ActionDispatch
class MiddlewareStack
class Middleware
attr_reader :args, :block, :name, :classcache
attr_reader :args, :block, :klass

def initialize(klass_or_name, args, block)
@klass = nil

if klass_or_name.respond_to?(:name)
@klass = klass_or_name
@name = @klass.name
else
@name = klass_or_name.to_s
end

@classcache = ActiveSupport::Dependencies::Reference
@args, @block = args, block
def initialize(klass, args, block)
@klass = klass
@args = args
@block = block
end

def klass
@klass || classcache[@name]
end
def name; klass.name; end

def ==(middleware)
case middleware
Expand All @@ -31,7 +21,7 @@ def ==(middleware)
when Class
klass == middleware
else
normalize(@name) == normalize(middleware)
normalize(name) == normalize(middleware)
end
end

Expand Down Expand Up @@ -123,8 +113,25 @@ def assert_index(index, where)

private

def get_class(klass)
if klass.is_a?(String) || klass.is_a?(Symbol)
classcache = ActiveSupport::Dependencies::Reference
converted_klass = classcache[klass.to_s]
ActiveSupport::Deprecation.warn <<-eowarn
Passing strings or symbols to the middleware builder is deprecated, please change
them to actual class references. For example:
"#{klass}" => #{converted_klass}
eowarn
converted_klass
else
klass
end
end

def build_middleware(klass, args, block)
Middleware.new(klass, args, block)
Middleware.new(get_class(klass), args, block)
end
end
end
14 changes: 7 additions & 7 deletions actionpack/test/abstract_unit.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,13 @@ class ActionDispatch::IntegrationTest < ActiveSupport::TestCase

def self.build_app(routes = nil)
RoutedRackApp.new(routes || ActionDispatch::Routing::RouteSet.new) do |middleware|
middleware.use "ActionDispatch::ShowExceptions", ActionDispatch::PublicExceptions.new("#{FIXTURE_LOAD_PATH}/public")
middleware.use "ActionDispatch::DebugExceptions"
middleware.use "ActionDispatch::Callbacks"
middleware.use "ActionDispatch::ParamsParser"
middleware.use "ActionDispatch::Cookies"
middleware.use "ActionDispatch::Flash"
middleware.use "Rack::Head"
middleware.use ActionDispatch::ShowExceptions, ActionDispatch::PublicExceptions.new("#{FIXTURE_LOAD_PATH}/public")
middleware.use ActionDispatch::DebugExceptions
middleware.use ActionDispatch::Callbacks
middleware.use ActionDispatch::ParamsParser
middleware.use ActionDispatch::Cookies
middleware.use ActionDispatch::Flash
middleware.use Rack::Head
yield(middleware) if block_given?
end
end
Expand Down
6 changes: 0 additions & 6 deletions actionpack/test/dispatch/middleware_stack/middleware_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,6 @@ def setup
@stack = ActionDispatch::MiddlewareStack.new
end

def test_string_class
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
stack.use k
Expand Down
36 changes: 19 additions & 17 deletions actionpack/test/dispatch/middleware_stack_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,21 @@ def setup
end

test "use should push middleware as a string onto the stack" do
assert_difference "@stack.size" do
@stack.use "MiddlewareStackTest::BazMiddleware"
assert_deprecated do
assert_difference "@stack.size" do
@stack.use "MiddlewareStackTest::BazMiddleware"
end
assert_equal BazMiddleware, @stack.last.klass
end
assert_equal BazMiddleware, @stack.last.klass
end

test "use should push middleware as a symbol onto the stack" do
assert_difference "@stack.size" do
@stack.use :"MiddlewareStackTest::BazMiddleware"
assert_deprecated do
assert_difference "@stack.size" do
@stack.use :"MiddlewareStackTest::BazMiddleware"
end
assert_equal BazMiddleware, @stack.last.klass
end
assert_equal BazMiddleware, @stack.last.klass
end

test "use should push middleware class with arguments onto the stack" do
Expand Down Expand Up @@ -88,8 +92,10 @@ def setup
end

test "unshift adds a new middleware at the beginning of the stack" do
@stack.unshift :"MiddlewareStackTest::BazMiddleware"
assert_equal BazMiddleware, @stack.first.klass
assert_deprecated do
@stack.unshift :"MiddlewareStackTest::BazMiddleware"
assert_equal BazMiddleware, @stack.first.klass
end
end

test "raise an error on invalid index" do
Expand All @@ -103,15 +109,11 @@ def setup
end

test "lazy evaluates middleware class" do
assert_difference "@stack.size" do
@stack.use "MiddlewareStackTest::BazMiddleware"
assert_deprecated do
assert_difference "@stack.size" do
@stack.use "MiddlewareStackTest::BazMiddleware"
end
assert_equal BazMiddleware, @stack.last.klass
end
assert_equal BazMiddleware, @stack.last.klass
end

test "lazy compares so unloaded constants are not loaded" do
@stack.use "UnknownMiddleware"
@stack.use :"MiddlewareStackTest::BazMiddleware"
assert @stack.include?("::MiddlewareStackTest::BazMiddleware")
end
end

6 comments on commit 83b767c

@memoht
Copy link

@memoht memoht commented on 83b767c Nov 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just ran across a deprecation message in my app for this today and tried to follow the advice. Given the following:

  • File app/middleware/client_searcher.rb with class ClientSearcher
  • Under application.rb used to have: config.middleware.insert_before ActionDispatch::Cookies, "ClientSearcher"
  • Tried to change that to config.middleware.insert_before ActionDispatch::Cookies, ClientSearcher leads to a NameError: uninitialized constant error.

If I add require_relative "../app/middleware/client_searcher" to application.rb I am back to booting. Looking through the docs, I am not certain why I have to do this. Without the require_relative line, what should I have done differently to fix the deprecation warning?

@memoht
Copy link

@memoht memoht commented on 83b767c Nov 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understood, Rails 3.2 introduced the support for app/middleware as the home for custom middleware and it would be auto loaded. Where in the docs for Rails 5 does it cover this change?

@gsamokovarov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@memoht You may wanna do this in an initializer, as the autoloading code still haven't kicked in when you are referencing this constant in config/application.rb.

@memoht
Copy link

@memoht memoht commented on 83b767c Nov 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gsamokovarov I haven't done that before. What would I put in the initializer to load this custom middleware? I created GIST to show what I have in this file: https://gist.github.com/memoht/30781d733a85eab94d86213b48b09cf2

@gsamokovarov
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# config/initializers/client_searcher.rb

Rails.configuration.middleware.insert_before ActionDispatch::Cookies, ClientSearcher

@memoht
Copy link

@memoht memoht commented on 83b767c Nov 14, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gsamokovarov . That worked. I appreciate your time.

Please sign in to comment.