Refactor strategy initialization, implementing pretty big changes.

lib/omniauth/strategy.rb

@@ -11,7 +11,7 @@ module OmniAuth
     def self.included(base)
       OmniAuth.strategies << base
       base.class_eval do
-        attr_reader :app, :name, :env, :options, :response
+        attr_reader :app, :env, :options, :response
       end
       base.extend ClassMethods
     end
@@ -70,6 +70,14 @@ module OmniAuth
       def option(name, value = nil)
         default_options[name] = value
       end
+
+      # Sets (and retrieves) option key names for initializer arguments to be
+      # recorded as. This takes care of 90% of the use cases for overriding
+      # the initializer in OmniAuth Strategies.
+      def args(args = nil)
+        return @args || [] unless args
+        @args = Array(args)
+      end
     end
 
     # Initializes the strategy by passing in the Rack endpoint,
@@ -78,14 +86,30 @@ module OmniAuth
     # created from the last argument if it is a hash.
     #
     # @param app [Rack application] The application on which this middleware is applied.
-    # @param name [String] A unique URL segment to describe this particular strategy. For example, `'openid'`.
-    # @yield [Strategy] Yields itself for block-based configuration.
-    def initialize(app, name, *args, &block)
+    #
+    # @overload new(app, options = {})
+    #   If nothing but a hash is supplied, initialized with the supplied options
+    #   overriding the strategy's default options via a deep merge.
+    # @overload new(app, *args, options = {})
+    #   If the strategy has supplied custom arguments that it accepts, they may
+    #   will be passed through and set to the appropriate values.
+    #
+    # @yield [Options] Yields options to block for further configuration.
+    def initialize(app, *args, &block)
       @app = app
-      @name = name.to_s
-      @options = self.class.default_options.deep_merge(args.last.is_a?(Hash) ? args.pop : {})
+      @options = self.class.default_options.dup
 
-      yield self if block_given?
+      options.deep_merge!(args.pop) if args.last.is_a?(Hash)
+      options.name ||= self.class.name.split('::').last.downcase
+
+      self.class.args.each do |arg|
+        options[arg] = args.shift
+      end
+
+      # Make sure that all of the args have been dealt with, otherwise error out.
+      raise ArgumentError, "Received wrong number of arguments. #{args.inspect}" unless args.empty?
+
+      yield options if block_given?
     end
 
     def inspect
@@ -211,7 +235,7 @@ module OmniAuth
     def setup_phase
       if options[:setup].respond_to?(:call)
         options[:setup].call(env)
-      elsif options[:setup]
+      elsif options.setup?
         setup_env = env.merge('PATH_INFO' => setup_path, 'REQUEST_METHOD' => 'GET')
         call_app!(setup_env)
       end
@@ -298,6 +322,10 @@ module OmniAuth
       @request ||= Rack::Request.new(@env)
     end
 
+    def name
+      options.name
+    end
+
     def redirect(uri)
       r = Rack::Response.new
 

spec/omniauth/strategy_spec.rb

@@ -2,6 +2,7 @@ require File.expand_path('../../spec_helper', __FILE__)
 
 class ExampleStrategy
   include OmniAuth::Strategy
+  option :name, 'test'
   def call(env); self.call!(env) end
   attr_reader :last_env
   def request_phase
@@ -75,22 +76,18 @@ describe OmniAuth::Strategy do
   describe '#initialize' do
     context 'options extraction' do
       it 'should be the last argument if the last argument is a Hash' do
-        ExampleStrategy.new(app, 'test', :abc => 123).options[:abc].should == 123
-      end
-
-      it 'should be a blank hash if none are provided' do
-        ExampleStrategy.new(app, 'test').options.should == {}
+        ExampleStrategy.new(app, :abc => 123).options[:abc].should == 123
       end
 
       it 'should be the default options if any are provided' do
         ExampleStrategy.stub!(:default_options).and_return(OmniAuth::Strategy::Options.new(:abc => 123))
-        ExampleStrategy.new(app, 'test').options.abc.should == 123
+        ExampleStrategy.new(app).options.abc.should == 123
       end
     end
   end
 
   describe '#full_host' do
-    let(:strategy){ ExampleStrategy.new(app, 'test', {}) }
+    let(:strategy){ ExampleStrategy.new(app, {}) }
     it 'should not freak out if there is a pipe in the URL' do
       strategy.call!(make_env('/whatever', 'rack.url_scheme' => 'http', 'SERVER_NAME' => 'facebook.lame', 'QUERY_STRING' => 'code=asofibasf|asoidnasd', 'SCRIPT_NAME' => '', 'SERVER_PORT' => 80))
       lambda{ strategy.full_host }.should_not raise_error
@@ -98,7 +95,7 @@ describe OmniAuth::Strategy do
   end
 
   describe '#call' do
-    let(:strategy){ ExampleStrategy.new(app, 'test', @options) }
+    let(:strategy){ ExampleStrategy.new(app, @options || {}) }
 
     context 'omniauth.origin' do
       it 'should be set on the request phase' do
@@ -195,7 +192,7 @@ describe OmniAuth::Strategy do
     end
 
     context 'pre-request call through' do
-      subject { ExampleStrategy.new(app, 'test') }
+      subject { ExampleStrategy.new(app) }
       let(:app){ lambda{|env| env['omniauth.boom'] = true; [env['test.status'] || 404, {}, ['Whatev']] } }
       it 'should be able to modify the env on the fly before the request_phase' do
         lambda{ subject.call(make_env) }.should raise_error("Request Phase")
@@ -408,7 +405,7 @@ describe OmniAuth::Strategy do
 
   context 'setup phase' do
     context 'when options[:setup] = true' do
-      let(:strategy){ ExampleStrategy.new(app, 'test', :setup => true) }
+      let(:strategy){ ExampleStrategy.new(app, :setup => true) }
       let(:app){lambda{|env| env['omniauth.strategy'].options[:awesome] = 'sauce' if env['PATH_INFO'] == '/auth/test/setup'; [404, {}, 'Awesome'] }}
 
       it 'should call through to /auth/:provider/setup' do
@@ -429,7 +426,7 @@ describe OmniAuth::Strategy do
         end
       end
 
-      let(:strategy){ ExampleStrategy.new(app, 'test', :setup => setup_proc) }
+      let(:strategy){ ExampleStrategy.new(app, :setup => setup_proc) }
 
       it 'should not call the app on a non-omniauth endpoint' do
         strategy.call(make_env('/somehwere/else'))