Routes refactoring:

* added more tests for prefix generation
* fixed bug with generating host for both prefix and url
* refactored url_for method
* organized tests for prefix generation

4 files changed, 77 insertions, 55 deletions

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 0e138524cd..88152ac290 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -288,8 +288,7 @@ module ActionDispatch
             _router = @set
             app.routes.class_eval do
               define_method :_generate_prefix do |options|
-                keys = _route.segment_keys + ActionDispatch::Routing::RouteSet::RESERVED_OPTIONS
-                prefix_options = options.slice(*keys)
+                prefix_options = options.slice(*_route.segment_keys)
                 # we must actually delete prefix segment keys to avoid passing them to next url_for
                 _route.segment_keys.each { |k| options.delete(k) }
                 _router.url_helpers.send("#{name}_path", prefix_options)
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index c94b00257b..b3945a4963 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -452,7 +452,7 @@ module ActionDispatch
         Generator.new(options, recall, self, extras).generate
       end
 
-      RESERVED_OPTIONS = [:anchor, :params, :only_path, :host, :protocol, :port, :trailing_slash, :script_name, :routes]
+      RESERVED_OPTIONS = [:anchor, :params, :only_path, :host, :protocol, :port, :trailing_slash, :script_name]
 
       def _generate_prefix(options = {})
         nil
diff --git a/actionpack/lib/action_dispatch/routing/url_for.rb b/actionpack/lib/action_dispatch/routing/url_for.rb
index edcb7f9cbe..30b456f3df 100644
--- a/actionpack/lib/action_dispatch/routing/url_for.rb
+++ b/actionpack/lib/action_dispatch/routing/url_for.rb
@@ -123,19 +123,17 @@ module ActionDispatch
       #    url_for :controller => 'tasks', :action => 'testing', :host=>'somehost.org', :anchor => 'ok', :only_path => true    # => '/tasks/testing#ok'
       #    url_for :controller => 'tasks', :action => 'testing', :trailing_slash=>true  # => 'http://somehost.org/tasks/testing/'
       #    url_for :controller => 'tasks', :action => 'testing', :host=>'somehost.org', :number => '33'  # => 'http://somehost.org/tasks/testing?number=33'
-      def url_for(*args)
-        if args.first.respond_to?(:routes)
-          app = args.shift
-          _with_routes(app.routes) do
+      def url_for(options = nil, *args)
+        if options.respond_to?(:routes)
+          _with_routes(options.routes) do
             if args.first.is_a? Symbol
               named_route = args.shift
-              url_for _routes.url_helpers.__send__("hash_for_#{named_route}", *args)
+              url_for _routes.url_helpers.send("hash_for_#{named_route}", *args)
             else
               url_for(*args)
             end
           end
         else
-          options = args.first
           case options
           when String
             options
diff --git a/actionpack/test/dispatch/prefix_generation_test.rb b/actionpack/test/dispatch/prefix_generation_test.rb
index c681642eda..2eb592c8d0 100644
--- a/actionpack/test/dispatch/prefix_generation_test.rb
+++ b/actionpack/test/dispatch/prefix_generation_test.rb
@@ -10,7 +10,8 @@ module TestGenerationPrefix
         @routes ||= begin
           routes = ActionDispatch::Routing::RouteSet.new
           routes.draw do
-            match "/posts/:id", :to => "inside_engine_generating#index", :as => :post
+            match "/posts/:id", :to => "inside_engine_generating#show", :as => :post
+            match "/posts", :to => "inside_engine_generating#index", :as => :posts
             match "/url_to_application", :to => "inside_engine_generating#url_to_application"
           end
 
@@ -47,13 +48,19 @@ module TestGenerationPrefix
     end
 
     class ::InsideEngineGeneratingController < ActionController::Base
+      include BlogEngine.routes.url_helpers
+
       def index
-        render :text => url_for(BlogEngine, :post_path, :id => params[:id])
+        render :text => posts_path
+      end
+
+      def show
+        render :text => post_path(:id => params[:id])
       end
 
       def url_to_application
-        path = url_for( :routes => RailsApplication.routes,
-                        :controller => "outside_engine_generating", 
+        path = url_for( RailsApplication,
+                        :controller => "outside_engine_generating",
                         :action => "index",
                         :only_path => true)
         render :text => path
@@ -61,100 +68,118 @@ module TestGenerationPrefix
     end
 
     class ::OutsideEngineGeneratingController < ActionController::Base
-      include BlogEngine.routes.url_helpers
       def index
         render :text => url_for(BlogEngine, :post_path, :id => 1)
       end
     end
 
-    class Foo
+    class EngineObject
       include ActionDispatch::Routing::UrlFor
       include BlogEngine.routes.url_helpers
-
-      def foo
-        post_path(42)
-      end
     end
 
-    class Bar
+    class AppObject
       include ActionDispatch::Routing::UrlFor
       include RailsApplication.routes.url_helpers
-
-      def bar
-        root_path
-      end
     end
 
-    RailsApplication.routes # force draw
-    include BlogEngine.routes.url_helpers
+    # force draw
+    RailsApplication.routes
 
     def app
       RailsApplication
     end
 
+    def engine_object
+      @engine_object ||= EngineObject.new
+    end
+
+    def app_object
+      @app_object ||= AppObject.new
+    end
+
     def setup
       RailsApplication.routes.default_url_options = {}
     end
 
-    test "generating URL with prefix" do
-      assert_equal "/awesome/blog/posts/1", post_path(:id => 1)
+    # Inside Engine
+    test "[ENGINE] generating engine's url use SCRIPT_NAME from request" do
+      get "/pure-awesomeness/blog/posts/1"
+      assert_equal "/pure-awesomeness/blog/posts/1", last_response.body
     end
 
-    test "use SCRIPT_NAME inside the engine" do
-      get "/pure-awesomness/blog/posts/1"
-      assert_equal "/pure-awesomness/blog/posts/1", last_response.body
+    test "[ENGINE] generating application's url never uses SCRIPT_NAME from request" do
+      get "/pure-awesomeness/blog/url_to_application"
+      assert_equal "/generate", last_response.body
     end
 
-    test "prepend prefix outside the engine" do
+    test "[ENGINE] generating application's url includes default_url_options[:script_name]" do
+      RailsApplication.routes.default_url_options = {:script_name => "/something"}
+      get "/pure-awesomeness/blog/url_to_application"
+      assert_equal "/something/generate", last_response.body
+    end
+
+    test "[ENGINE] generating application's url should give higher priority to default_url_options[:script_name]" do
+      RailsApplication.routes.default_url_options = {:script_name => "/something"}
+      get "/pure-awesomeness/blog/url_to_application", {}, 'SCRIPT_NAME' => '/foo'
+      assert_equal "/something/generate", last_response.body
+    end
+    
+    # Inside Application
+    test "[APP] generating engine's route includes prefix" do
       get "/generate"
       assert_equal "/awesome/blog/posts/1", last_response.body
     end
 
-    test "prepend prefix outside the engine and use default_url_options[:script_name]" do
+    test "[APP] generating engine's route includes default_url_options[:script_name]" do
       RailsApplication.routes.default_url_options = {:script_name => "/something"}
       get "/generate"
       assert_equal "/something/awesome/blog/posts/1", last_response.body
     end
 
-    test "give higher priority to default_url_options[:script_name]" do
+    test "[APP] generating engine's route should give higher priority to default_url_options[:script_name]" do
       RailsApplication.routes.default_url_options = {:script_name => "/something"}
       get "/generate", {}, 'SCRIPT_NAME' => "/foo"
       assert_equal "/something/awesome/blog/posts/1", last_response.body
     end
 
-    test "generating urls with options for prefix and named_route" do
-      assert_equal "/pure-awesomness/blog/posts/3", post_path(:id => 3, :omg => "pure-awesomness")
+    # Inside any Object
+    test "[OBJECT] generating engine's route includes prefix" do
+      assert_equal "/awesome/blog/posts/1", engine_object.post_path(:id => 1)
     end
 
-    test "generating urls with url_for should prepend the prefix" do
-      path = BlogEngine.routes.url_for(:omg => 'omg', :controller => "inside_engine_generating", :action => "index", :id => 1, :only_path => true)
-      assert_equal "/omg/blog/posts/1", path
+    test "[OBJECT] generating engine's route includes dynamic prefix" do
+      assert_equal "/pure-awesomeness/blog/posts/3", engine_object.post_path(:id => 3, :omg => "pure-awesomeness")
     end
 
-    test "generating urls from a regular class" do
-      assert_equal "/awesome/blog/posts/42", Foo.new.foo
+    test "[OBJECT] generating engine's route includes default_url_options[:script_name]" do
+      RailsApplication.routes.default_url_options = {:script_name => "/something"}
+      assert_equal "/something/pure-awesomeness/blog/posts/3", engine_object.post_path(:id => 3, :omg => "pure-awesomeness")
     end
 
-    test "generating application's url from engine" do
-      get "/pure-awesomness/blog/url_to_application"
-      assert_equal "/generate", last_response.body
+    test "[OBJECT] generating application's route" do
+      assert_equal "/", app_object.root_path
     end
 
-    test "generating application's url from engine with default_url_options[:script_name]" do
+    test "[OBJECT] generating application's route includes default_url_options[:script_name]" do
       RailsApplication.routes.default_url_options = {:script_name => "/something"}
-      get "/pure-awesomness/blog/url_to_application"
-      assert_equal "/something/generate", last_response.body
+      assert_equal "/something/", app_object.root_path
     end
 
-    test "generating application's url from engine should give higher priority to default_url_options[:script_name]" do
-      RailsApplication.routes.default_url_options = {:script_name => "/something"}
-      get "/pure-awesomness/blog/url_to_application", {}, 'SCRIPT_NAME' => '/foo'
-      assert_equal "/something/generate", last_response.body
-    end
+    test "[OBJECT] generating engine's route with url_for" do
+      path = engine_object.url_for(BlogEngine,
+                                   :controller => "inside_engine_generating",
+                                   :action => "show",
+                                   :only_path => true,
+                                   :omg => "omg",
+                                   :id => 1)
+      assert_equal "/omg/blog/posts/1", path
 
-    test "using default_url_options[:script_name] in regular classes" do
-      RailsApplication.routes.default_url_options = {:script_name => "/something"}
-      assert_equal "/something/", Bar.new.bar
+      path = engine_object.url_for(BlogEngine, :posts_path)
+      assert_equal "/awesome/blog/posts", path
+
+      path = engine_object.url_for(BlogEngine, :posts_url, :host => "example.com")
+      assert_equal "http://example.com/awesome/blog/posts", path
     end
   end
 end