Merge branch 'master' of gitproxy:rails/rails

9 files changed, 143 insertions, 159 deletions

diff --git a/actionpack/lib/action_controller/base.rb b/actionpack/lib/action_controller/base.rb
index d00afa6d4e..fcd3cb9bd3 100644
--- a/actionpack/lib/action_controller/base.rb
+++ b/actionpack/lib/action_controller/base.rb
@@ -29,6 +29,7 @@ module ActionController
     include ActionController::Verification
     include ActionController::RequestForgeryProtection
     include ActionController::Streaming
+    include ActionController::RecordIdentifier
     include ActionController::HttpAuthentication::Basic::ControllerMethods
     include ActionController::HttpAuthentication::Digest::ControllerMethods
 
diff --git a/actionpack/lib/action_controller/metal/url_for.rb b/actionpack/lib/action_controller/metal/url_for.rb
index f6b6cb2ff4..10c7ca9021 100644
--- a/actionpack/lib/action_controller/metal/url_for.rb
+++ b/actionpack/lib/action_controller/metal/url_for.rb
@@ -3,7 +3,6 @@ module ActionController
     extend ActiveSupport::Concern
 
     include ActionDispatch::Routing::UrlFor
-    include ActionController::RackDelegation
 
     def url_options
       super.reverse_merge(
diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb
index 7e0a833dfa..c43e560ac1 100644
--- a/actionpack/lib/action_controller/test_case.rb
+++ b/actionpack/lib/action_controller/test_case.rb
@@ -339,9 +339,13 @@ module ActionController
     def build_request_uri(action, parameters)
       unless @request.env["PATH_INFO"]
         options = @controller.__send__(:url_options).merge(parameters)
-        options.update(:only_path => true, :action => action, :relative_url_root => nil)
-        rewriter = ActionController::UrlRewriter.new(@request, parameters)
+        options.update(
+          :only_path => true,
+          :action => action,
+          :relative_url_root => nil,
+          :_path_segments => @request.symbolized_path_parameters)
 
+        rewriter = ActionController::UrlRewriter
         url, query_string = rewriter.rewrite(@router, options).split("?", 2)
 
         @request.env["SCRIPT_NAME"] = @controller.config.relative_url_root
diff --git a/actionpack/lib/action_controller/url_rewriter.rb b/actionpack/lib/action_controller/url_rewriter.rb
index 981865517f..b387d49593 100644
--- a/actionpack/lib/action_controller/url_rewriter.rb
+++ b/actionpack/lib/action_controller/url_rewriter.rb
@@ -2,37 +2,16 @@ require 'active_support/core_ext/hash/except'
 
 module ActionController
   # Rewrites URLs for Base.redirect_to and Base.url_for in the controller.
-  class UrlRewriter #:nodoc:
+  module UrlRewriter #:nodoc:
     RESERVED_OPTIONS = [:anchor, :params, :only_path, :host, :protocol, :port, :trailing_slash, :skip_relative_url_root]
 
-    def initialize(request, parameters)
-      @request, @parameters = request, parameters
-    end
-
-    def rewrite(router, options = {})
-      options[:host]     ||= @request.host_with_port
-      options[:protocol] ||= @request.protocol
-
-      self.class.rewrite(router, options, @request.symbolized_path_parameters) do |options|
-        process_path_options(options)
-      end
-    end
-
-    def to_str
-      "#{@request.protocol}, #{@request.host_with_port}, #{@request.path}, #{@parameters[:controller]}, #{@parameters[:action]}, #{@request.parameters.inspect}"
-    end
-
-    alias_method :to_s, :to_str
-
     # ROUTES TODO: Class method code smell
-    def self.rewrite(router, options, path_segments=nil)
+    def self.rewrite(router, options)
       handle_positional_args(options)
 
       rewritten_url = ""
 
-      # ROUTES TODO: Fix the tests
-      segments = options.delete(:_path_segments)
-      path_segments = path_segments ? path_segments.merge(segments || {}) : segments
+      path_segments = options.delete(:_path_segments)
 
       unless options[:only_path]
         rewritten_url << (options[:protocol] || "http")
@@ -81,18 +60,5 @@ module ActionController
       end
     end
 
-    # Given a Hash of options, generates a route
-    def process_path_options(options)
-      options = options.symbolize_keys
-      options.update(options[:params].symbolize_keys) if options[:params]
-
-      if (overwrite = options.delete(:overwrite_params))
-        options.update(@parameters.symbolize_keys)
-        options.update(overwrite.symbolize_keys)
-      end
-
-      options
-    end
-
   end
 end
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 8ebad1a9b8..e99e39269b 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -276,116 +276,138 @@ module ActionDispatch
         route
       end
 
-      def options_as_params(options)
-        # If an explicit :controller was given, always make :action explicit
-        # too, so that action expiry works as expected for things like
-        #
-        #   generate({:controller => 'content'}, {:controller => 'content', :action => 'show'})
-        #
-        # (the above is from the unit tests). In the above case, because the
-        # controller was explicitly given, but no action, the action is implied to
-        # be "index", not the recalled action of "show".
-        #
-        # great fun, eh?
-
-        options_as_params = options.clone
-        options_as_params[:action] ||= 'index' if options[:controller]
-        options_as_params[:action] = options_as_params[:action].to_s if options_as_params[:action]
-        options_as_params
-      end
+      class Generator
+        attr_reader :options, :recall, :set, :script_name, :named_route
+
+        def initialize(options, recall, set, extras = false)
+          @script_name = options.delete(:script_name)
+          @named_route = options.delete(:use_route)
+          @options     = options.dup
+          @recall      = recall.dup
+          @set         = set
+          @extras      = extras
+
+          normalize_options!
+          normalize_controller_action_id!
+          use_relative_controller!
+          controller.sub!(%r{^/}, '') if controller
+          handle_nil_action!
+        end
 
-      def build_expiry(options, recall)
-        recall.inject({}) do |expiry, (key, recalled_value)|
-          expiry[key] = (options.key?(key) && options[key].to_param != recalled_value.to_param)
-          expiry
+        def controller
+          @controller ||= @options[:controller]
         end
-      end
 
-      # Generate the path indicated by the arguments, and return an array of
-      # the keys that were not used to generate it.
-      def extra_keys(options, recall={})
-        generate_extras(options, recall).last
-      end
+        def current_controller
+          @recall[:controller]
+        end
 
-      def generate_extras(options, recall={})
-        generate(options, recall, :generate_extras)
-      end
+        def use_recall_for(key)
+          if @recall[key] && (!@options.key?(key) || @options[key] == @recall[key])
+            @options[key] = @recall.delete(key)
+          end
+        end
 
-      def generate(options, recall = {}, method = :generate)
-        options, recall = options.dup, recall.dup
-        script_name = options.delete(:script_name)
-        named_route = options.delete(:use_route)
+        def normalize_options!
+          # If an explicit :controller was given, always make :action explicit
+          # too, so that action expiry works as expected for things like
+          #
+          #   generate({:controller => 'content'}, {:controller => 'content', :action => 'show'})
+          #
+          # (the above is from the unit tests). In the above case, because the
+          # controller was explicitly given, but no action, the action is implied to
+          # be "index", not the recalled action of "show".
 
-        options = options_as_params(options)
-        expire_on = build_expiry(options, recall)
+          if options[:controller]
+            options[:action]     ||= 'index'
+            options[:controller]   = options[:controller].to_s
+          end
 
-        recall[:action] ||= 'index' if options[:controller] || recall[:controller]
+          if options[:action]
+            options[:action] = options[:action].to_s
+          end
+        end
 
-        if recall[:controller] && (!options.has_key?(:controller) || options[:controller] == recall[:controller])
-          options[:controller] = recall.delete(:controller)
+        # This pulls :controller, :action, and :id out of the recall.
+        # The recall key is only used if there is no key in the options
+        # or if the key in the options is identical. If any of
+        # :controller, :action or :id is not found, don't pull any
+        # more keys from the recall.
+        def normalize_controller_action_id!
+          @recall[:action] ||= 'index' if current_controller
+
+          use_recall_for(:controller) or return
+          use_recall_for(:action) or return
+          use_recall_for(:id)
+        end
 
-          if recall[:action] && (!options.has_key?(:action) || options[:action] == recall[:action])
-            options[:action] = recall.delete(:action)
+        # if the current controller is "foo/bar/baz" and :controller => "baz/bat"
+        # is specified, the controller becomes "foo/baz/bat"
+        def use_relative_controller!
+          if !named_route && different_controller?
+            old_parts = current_controller.split('/')
+            size = controller.count("/") + 1
+            parts = old_parts[0...-size] << controller
+            @controller = @options[:controller] = parts.join("/")
+          end
+        end
 
-            if recall[:id] && (!options.has_key?(:id) || options[:id] == recall[:id])
-              options[:id] = recall.delete(:id)
-            end
+        # This handles the case of :action => nil being explicitly passed.
+        # It is identical to :action => "index"
+        def handle_nil_action!
+          if options.has_key?(:action) && options[:action].nil?
+            options[:action] = 'index'
           end
+          recall[:action] = options.delete(:action) if options[:action] == 'index'
         end
 
-        options[:controller] = options[:controller].to_s if options[:controller]
+        def generate
+          error = ActionController::RoutingError.new("No route matches #{options.inspect}")
+          path, params = @set.generate(:path_info, named_route, options, recall, opts)
 
-        if !named_route && expire_on[:controller] && options[:controller] && options[:controller][0] != ?/
-          old_parts = recall[:controller].split('/')
-          new_parts = options[:controller].split('/')
-          parts = old_parts[0..-(new_parts.length + 1)] + new_parts
-          options[:controller] = parts.join('/')
-        end
+          raise error unless path
 
-        options[:controller] = options[:controller][1..-1] if options[:controller] && options[:controller][0] == ?/
+          params.reject! {|k,v| !v }
 
-        merged = options.merge(recall)
-        if options.has_key?(:action) && options[:action].nil?
-          options.delete(:action)
-          recall[:action] = 'index'
-        end
-        recall[:action] = options.delete(:action) if options[:action] == 'index'
-
-        opts = {}
-        opts[:parameterize] = lambda { |name, value|
-          if name == :controller
-            value
-          elsif value.is_a?(Array)
-            value.map { |v| Rack::Mount::Utils.escape_uri(v.to_param) }.join('/')
-          else
-            Rack::Mount::Utils.escape_uri(value.to_param)
-          end
-        }
+          return [path, params.keys] if @extras
 
-        unless result = @set.generate(:path_info, named_route, options, recall, opts)
-          raise ActionController::RoutingError, "No route matches #{options.inspect}"
+          path << "?#{params.to_query}" if params.any?
+          "#{script_name}#{path}"
+        rescue Rack::Mount::RoutingError
+          raise error
         end
 
-        path, params = result
-        params.each do |k, v|
-          if v
-            params[k] = v
-          else
-            params.delete(k)
+        def opts
+          parameterize = lambda do |name, value|
+            if name == :controller
+              value
+            elsif value.is_a?(Array)
+              value.map { |v| Rack::Mount::Utils.escape_uri(v.to_param) }.join('/')
+            else
+              Rack::Mount::Utils.escape_uri(value.to_param)
+            end
           end
+          {:parameterize => parameterize}
         end
 
-        if path && method == :generate_extras
-          [path, params.keys]
-        elsif path
-          path << "?#{params.to_query}" if params.any?
-          path = "#{script_name}#{path}"
-          path
-        else
-          raise ActionController::RoutingError, "No route matches #{options.inspect}"
+        def different_controller?
+          return false unless current_controller
+          controller.to_param != current_controller.to_param
         end
-      rescue Rack::Mount::RoutingError
-        raise ActionController::RoutingError, "No route matches #{options.inspect}"
+      end
+
+      # Generate the path indicated by the arguments, and return an array of
+      # the keys that were not used to generate it.
+      def extra_keys(options, recall={})
+        generate_extras(options, recall).last
+      end
+
+      def generate_extras(options, recall={})
+        generate(options, recall, true)
+      end
+
+      def generate(options, recall = {}, extras = false)
+        Generator.new(options, recall, @set, extras).generate
       end
 
       def call(env)
diff --git a/actionpack/test/controller/base_test.rb b/actionpack/test/controller/base_test.rb
index 3e9f8ae048..f047e7da30 100644
--- a/actionpack/test/controller/base_test.rb
+++ b/actionpack/test/controller/base_test.rb
@@ -76,6 +76,9 @@ class UrlOptionsController < ActionController::Base
   end
 end
 
+class RecordIdentifierController < ActionController::Base
+end
+
 class ControllerClassTests < ActiveSupport::TestCase
   def test_controller_path
     assert_equal 'empty', EmptyController.controller_path
@@ -102,6 +105,11 @@ class ControllerClassTests < ActiveSupport::TestCase
 
     assert_equal [:password], parameters
   end
+
+  def test_record_identifier
+    assert_respond_to RecordIdentifierController.new, :dom_id
+    assert_respond_to RecordIdentifierController.new, :dom_class
+  end
 end
 
 class ControllerInstanceTests < Test::Unit::TestCase
diff --git a/actionpack/test/controller/caching_test.rb b/actionpack/test/controller/caching_test.rb
index 11ef3cdd92..2be91893d4 100644
--- a/actionpack/test/controller/caching_test.rb
+++ b/actionpack/test/controller/caching_test.rb
@@ -64,7 +64,7 @@ class PageCachingTest < ActionController::TestCase
     @controller.cache_store = :file_store, FILE_STORE_PATH
 
     @params = {:controller => 'posts', :action => 'index', :only_path => true, :skip_relative_url_root => true}
-    @rewriter = ActionController::UrlRewriter.new(@request, @params)
+    @rewriter = ActionController::UrlRewriter
 
     FileUtils.rm_rf(File.dirname(FILE_STORE_PATH))
     FileUtils.mkdir_p(FILE_STORE_PATH)
diff --git a/actionpack/test/controller/url_rewriter_test.rb b/actionpack/test/controller/url_rewriter_test.rb
index 199b824a7a..3bc8bec3fb 100644
--- a/actionpack/test/controller/url_rewriter_test.rb
+++ b/actionpack/test/controller/url_rewriter_test.rb
@@ -4,10 +4,24 @@ require 'controller/fake_controllers'
 ActionController::UrlRewriter
 
 class UrlRewriterTests < ActionController::TestCase
+  class Rewriter
+    def initialize(request)
+      @options = {
+        :host => request.host_with_port,
+        :protocol => request.protocol
+      }
+    end
+
+    def rewrite(router, options)
+      options = @options.merge(options)
+      ActionController::UrlRewriter.rewrite(router, options)
+    end
+  end
+
   def setup
     @request = ActionController::TestRequest.new
     @params = {}
-    @rewriter = ActionController::UrlRewriter.new(@request, @params)
+    @rewriter = Rewriter.new(@request) #.new(@request, @params)
   end
 
   def test_port
@@ -61,34 +75,6 @@ class UrlRewriterTests < ActionController::TestCase
     )
   end
 
-  def test_overwrite_params
-    @params[:controller] = 'hi'
-    @params[:action] = 'bye'
-    @params[:id] = '2'
-
-    assert_equal '/hi/hi/2', @rewriter.rewrite(@router, :only_path => true, :overwrite_params => {:action => 'hi'})
-    u = @rewriter.rewrite(@router, :only_path => false, :overwrite_params => {:action => 'hi'})
-    assert_match %r(/hi/hi/2$), u
-  end
-
-  def test_overwrite_removes_original
-    @params[:controller] = 'search'
-    @params[:action] = 'list'
-    @params[:list_page] = 1
-
-    assert_equal '/search/list?list_page=2', @rewriter.rewrite(@router, :only_path => true, :overwrite_params => {"list_page" => 2})
-    u = @rewriter.rewrite(@router, :only_path => false, :overwrite_params => {:list_page => 2})
-    assert_equal 'http://test.host/search/list?list_page=2', u
-  end
-
-  def test_to_str
-    @params[:controller] = 'hi'
-    @params[:action] = 'bye'
-    @request.parameters[:id] = '2'
-
-    assert_equal 'http://, test.host, /, hi, bye, {"id"=>"2"}', @rewriter.to_str
-  end
-
   def test_trailing_slash
     options = {:controller => 'foo', :action => 'bar', :id => '3', :only_path => true}
     assert_equal '/foo/bar/3', @rewriter.rewrite(@router, options)
diff --git a/actionpack/test/template/url_helper_test.rb b/actionpack/test/template/url_helper_test.rb
index 38ab5521fe..165cb655da 100644
--- a/actionpack/test/template/url_helper_test.rb
+++ b/actionpack/test/template/url_helper_test.rb
@@ -122,7 +122,6 @@ class UrlHelperTest < ActionView::TestCase
     url = {:controller => 'weblog', :action => 'show'}
     @controller = ActionController::Base.new
     @controller.request = ActionController::TestRequest.new
-    @controller.url = ActionController::UrlRewriter.new(@controller.request, url)
     assert_dom_equal(%q{<a href="/weblog/show">Test Link</a>}, link_to('Test Link', url))
   end
 
@@ -131,7 +130,6 @@ class UrlHelperTest < ActionView::TestCase
     url = {:controller => 'weblog', :action => 'show', :host => 'www.example.com'}
     @controller = ActionController::Base.new
     @controller.request = ActionController::TestRequest.new
-    @controller.url = ActionController::UrlRewriter.new(@controller.request, url)
     assert_dom_equal(%q{<a href="http://www.example.com/weblog/show">Test Link</a>}, link_to('Test Link', url))
   end