Clean up routes inclusion and add some comments for the next soul that decides...

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

actionpack/lib/action_dispatch/routing.rb

@@ -284,10 +284,5 @@ module Routing
 
     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

actionpack/lib/action_dispatch/routing/route_set.rb

@@ -123,11 +123,6 @@ def length
           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 @@ def clear!
         @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 @@ def define_mounted_helper(name)
         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 @@ def #{name}
       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?

actionpack/lib/action_dispatch/routing/url_for.rb

@@ -8,7 +8,8 @@ module Routing
     #
     # <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 UrlFor
       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 @@ def url_for(options = nil)
       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

actionpack/test/controller/action_pack_assertions_test.rb

@@ -159,20 +159,6 @@ def test_get_post_request_switch
     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