Clean up routes inclusion and add some comments for the next soul that decides to adventure on this code.

4 files changed, 49 insertions, 60 deletions

diff --git a/actionpack/lib/action_dispatch/routing.rb b/actionpack/lib/action_dispatch/routing.rb
index fa300b4a16..2f6b9d266d 100644
--- a/actionpack/lib/action_dispatch/routing.rb
+++ b/actionpack/lib/action_dispatch/routing.rb
@@ -284,10 +284,5 @@ module ActionDispatch
 
     SEPARATORS = %w( / . ? ) #:nodoc:
     HTTP_METHODS = [:get, :head, :post, :put, :delete, :options] #:nodoc:
-
-    # A helper module to hold URL related helpers.
-    module Helpers #:nodoc:
-      include PolymorphicRoutes
-    end
   end
 end
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 10b3e212e6..9d0a3e9993 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -123,11 +123,6 @@ module ActionDispatch
           routes.length
         end
 
-        def install(destinations = [ActionController::Base, ActionView::Base])
-          helper = @module
-          destinations.each { |d| d.module_eval { include helper } }
-        end
-
         private
           def url_helper_name(name, kind = :url)
             :"#{name}_#{kind}"
@@ -276,14 +271,15 @@ module ActionDispatch
         @prepend.each { |blk| eval_block(blk) }
       end
 
-      def install_helpers(destinations)
-        destinations.each { |d| d.module_eval { include Helpers } }
-        named_routes.install(destinations)
-      end
-
-      module MountedHelpers
+      module MountedHelpers #:nodoc:
+        extend ActiveSupport::Concern
+        include UrlFor
       end
 
+      # Contains all the mounted helpers accross different
+      # engines and the `main_app` helper for the application.
+      # You can include this in your classes if you want to
+      # access routes for other engines.
       def mounted_helpers
         MountedHelpers
       end
@@ -294,7 +290,7 @@ module ActionDispatch
         routes = self
         MountedHelpers.class_eval do
           define_method "_#{name}" do
-            RoutesProxy.new(routes, self._routes_context)
+            RoutesProxy.new(routes, _routes_context)
           end
         end
 
@@ -306,29 +302,40 @@ module ActionDispatch
       end
 
       def url_helpers
-        routes = self
+        @url_helpers ||= begin
+          routes = self
+
+          Module.new do
+            extend ActiveSupport::Concern
+            include UrlFor
+
+            # Define url_for in the singleton level so one can do:
+            # Rails.application.routes.url_helpers.url_for(args)
+            @_routes = routes
+            class << self
+              delegate :url_for, :to => '@_routes'
+            end
 
-        @url_helpers ||= Module.new {
-          extend ActiveSupport::Concern
-          include UrlFor
+            # Make named_routes available in the module singleton
+            # as well, so one can do:
+            # Rails.application.routes.url_helpers.posts_path
+            extend routes.named_routes.module
 
-          @_routes = routes
-          def self.url_for(options)
-            @_routes.url_for options
-          end
+            # Any class that includes this module will get all
+            # named routes...
+            include routes.named_routes.module
 
-          extend routes.named_routes.module
+            # plus a singleton class method called _routes ...
+            included do
+              singleton_class.send(:redefine_method, :_routes) { routes }
+            end
 
-          # ROUTES TODO: install_helpers isn't great... can we make a module with the stuff that
-          # we can include?
-          # Yes plz - JP
-          included do
-            routes.install_helpers([self])
-            singleton_class.send(:redefine_method, :_routes) { routes }
+            # And an instance method _routes. Note that
+            # UrlFor (included in this module) add extra
+            # conveniences for working with @_routes.
+            define_method(:_routes) { @_routes || routes }
           end
-
-          define_method(:_routes) { @_routes || routes }
-        }
+        end
       end
 
       def empty?
diff --git a/actionpack/lib/action_dispatch/routing/url_for.rb b/actionpack/lib/action_dispatch/routing/url_for.rb
index 6c2a98ab15..ee6616c5d3 100644
--- a/actionpack/lib/action_dispatch/routing/url_for.rb
+++ b/actionpack/lib/action_dispatch/routing/url_for.rb
@@ -8,7 +8,8 @@ module ActionDispatch
     #
     # <b>Tip:</b> If you need to generate URLs from your models or some other place,
     # then ActionController::UrlFor is what you're looking for. Read on for
-    # an introduction.
+    # an introduction. In general, this module should not be included on its own,
+    # as it is usually included by url_helpers (as in Rails.application.routes.url_helpers).
     #
     # == URL generation from parameters
     #
@@ -84,7 +85,6 @@ module ActionDispatch
       include PolymorphicRoutes
 
       included do
-        # TODO: with_routing extends @controller with url_helpers, trickling down to including this module which overrides its default_url_options
         unless method_defined?(:default_url_options)
           # Including in a class uses an inheritable hash. Modules get a plain hash.
           if respond_to?(:class_attribute)
@@ -151,16 +151,17 @@ module ActionDispatch
       end
 
       protected
-        def _with_routes(routes)
-          old_routes, @_routes = @_routes, routes
-          yield
-        ensure
-          @_routes = old_routes
-        end
 
-        def _routes_context
-          self
-        end
+      def _with_routes(routes)
+        old_routes, @_routes = @_routes, routes
+        yield
+      ensure
+        @_routes = old_routes
+      end
+
+      def _routes_context
+        self
+      end
     end
   end
 end
diff --git a/actionpack/test/controller/action_pack_assertions_test.rb b/actionpack/test/controller/action_pack_assertions_test.rb
index fab70c71d6..bdbf158b36 100644
--- a/actionpack/test/controller/action_pack_assertions_test.rb
+++ b/actionpack/test/controller/action_pack_assertions_test.rb
@@ -159,20 +159,6 @@ class ActionPackAssertionsControllerTest < ActionController::TestCase
     assert_equal @response.body, 'request method: GET'
   end
 
-  def test_redirect_to_named_route
-    with_routing do |set|
-      set.draw do
-        match 'route_one', :to => 'action_pack_assertions#nothing', :as => :route_one
-        match ':controller/:action'
-      end
-      set.install_helpers([ActionController::Base, ActionView::Base])
-
-      process :redirect_to_named_route
-      assert_redirected_to 'http://test.host/route_one'
-      assert_redirected_to route_one_url
-    end
-  end
-
   def test_string_constraint
     with_routing do |set|
       set.draw do