let mailer templates generate URLs by default [Xavier Noria, Richard Schneeman]

7 files changed, 107 insertions, 12 deletions

diff --git a/actionmailer/CHANGELOG.md b/actionmailer/CHANGELOG.md
index 5685871ac9..1b3e802cb2 100644
--- a/actionmailer/CHANGELOG.md
+++ b/actionmailer/CHANGELOG.md
@@ -1,3 +1,10 @@
+*   `link_to` and `url_for` generate URLs by default in templates, it is no
+    longer needed to pass `only_path: false`.
+
+    Fixes #16497 and #16589.
+
+    *Xavier Noria*, *Richard Schneeman*
+
 *   Attachments can be added while rendering the mail template.
 
     Fixes #16974.
diff --git a/actionmailer/test/fixtures/url_test_mailer/exercise_url_for.erb b/actionmailer/test/fixtures/url_test_mailer/exercise_url_for.erb
new file mode 100644
index 0000000000..0322c1191e
--- /dev/null
+++ b/actionmailer/test/fixtures/url_test_mailer/exercise_url_for.erb
@@ -0,0 +1 @@
+<%= url_for(@options) %> <%= @url %>
diff --git a/actionmailer/test/url_test.rb b/actionmailer/test/url_test.rb
index be7532d42f..8fce7c3827 100644
--- a/actionmailer/test/url_test.rb
+++ b/actionmailer/test/url_test.rb
@@ -23,9 +23,32 @@ class UrlTestMailer < ActionMailer::Base
     mail(to: recipient, subject: "[Signed up] Welcome #{recipient}",
       from: "system@loudthinking.com", date: Time.local(2004, 12, 12))
   end
+
+  def exercise_url_for(options)
+    @options = options
+    @url = url_for(@options)
+    mail(from: 'from@example.com', to: 'to@example.com', subject: 'subject')
+  end
 end
 
 class ActionMailerUrlTest < ActionMailer::TestCase
+  class DummyModel
+    def self.model_name
+      OpenStruct.new(route_key: 'dummy_model')
+    end
+
+    def persisted?
+      false
+    end
+
+    def model_name
+      self.class.model_name
+    end
+
+    def to_model
+      self
+    end
+  end
 
   def encode( text, charset="UTF-8" )
     quoted_printable( text, charset )
@@ -40,10 +63,47 @@ class ActionMailerUrlTest < ActionMailer::TestCase
     mail
   end
 
+  def assert_url_for(expected, options, relative: false)
+    expected = "http://www.basecamphq.com#{expected}" if expected.start_with?('/') && !relative
+    urls = UrlTestMailer.exercise_url_for(options).body.to_s.chomp.split
+
+    assert_equal expected, urls.first
+    assert_equal expected, urls.second
+  end
+
   def setup
     @recipient = 'test@localhost'
   end
 
+  def test_url_for
+    UrlTestMailer.delivery_method = :test
+
+    AppRoutes.draw do
+      get ':controller(/:action(/:id))'
+      get '/welcome'  => 'foo#bar', as: 'welcome'
+      get '/dummy_model' => 'foo#baz', as: 'dummy_model'
+    end
+
+    # string
+    assert_url_for 'http://foo/', 'http://foo/'
+
+    # symbol
+    assert_url_for '/welcome', :welcome
+
+    # hash
+    assert_url_for '/a/b/c', controller: 'a', action: 'b', id: 'c'
+    assert_url_for '/a/b/c', {controller: 'a', action: 'b', id: 'c', only_path: true}, relative: true
+
+    # model
+    assert_url_for '/dummy_model', DummyModel.new
+
+    # class
+    assert_url_for '/dummy_model', DummyModel
+
+    # array
+    assert_url_for '/dummy_model' , [DummyModel]
+  end
+
   def test_signed_up_with_url
     UrlTestMailer.delivery_method = :test
 
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index a641ea3ea9..d2ae2a496f 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -457,7 +457,7 @@ module ActionDispatch
         RUBY
       end
 
-      def url_helpers(include_path_helpers = true)
+      def url_helpers(supports_path = true)
         routes = self
 
         Module.new do
@@ -484,7 +484,7 @@ module ActionDispatch
           # named routes...
           include url_helpers
 
-          if include_path_helpers
+          if supports_path
             path_helpers = routes.named_routes.path_helpers_module
           else
             path_helpers = routes.named_routes.path_helpers_module(true)
@@ -502,6 +502,10 @@ module ActionDispatch
           # UrlFor (included in this module) add extra
           # conveniences for working with @_routes.
           define_method(:_routes) { @_routes || routes }
+
+          define_method(:_generate_paths_by_default) do
+            supports_path
+          end
         end
       end
 
diff --git a/actionpack/lib/action_dispatch/routing/url_for.rb b/actionpack/lib/action_dispatch/routing/url_for.rb
index eb554ec383..dca86858cc 100644
--- a/actionpack/lib/action_dispatch/routing/url_for.rb
+++ b/actionpack/lib/action_dispatch/routing/url_for.rb
@@ -184,6 +184,12 @@ module ActionDispatch
       def _routes_context
         self
       end
+
+      private
+
+        def _generate_paths_by_default
+          true
+        end
     end
   end
 end
diff --git a/actionview/lib/action_view/rendering.rb b/actionview/lib/action_view/rendering.rb
index 5cbdfdf6c0..abd3b77c67 100644
--- a/actionview/lib/action_view/rendering.rb
+++ b/actionview/lib/action_view/rendering.rb
@@ -35,13 +35,13 @@ module ActionView
     module ClassMethods
       def view_context_class
         @view_context_class ||= begin
-          include_path_helpers = supports_path?
+          supports_path = supports_path?
           routes  = respond_to?(:_routes)  && _routes
           helpers = respond_to?(:_helpers) && _helpers
 
           Class.new(ActionView::Base) do
             if routes
-              include routes.url_helpers(include_path_helpers)
+              include routes.url_helpers(supports_path)
               include routes.mounted_helpers
             end
 
diff --git a/actionview/lib/action_view/routing_url_for.rb b/actionview/lib/action_view/routing_url_for.rb
index 75febb8652..f281333a41 100644
--- a/actionview/lib/action_view/routing_url_for.rb
+++ b/actionview/lib/action_view/routing_url_for.rb
@@ -80,21 +80,38 @@ module ActionView
       when String
         options
       when nil
-        super({:only_path => true})
+        super(only_path: _generate_paths_by_default)
       when Hash
         options = options.symbolize_keys
-        options[:only_path] = options[:host].nil? unless options.key?(:only_path)
+        unless options.key?(:only_path)
+          if options[:host].nil?
+            options[:only_path] = _generate_paths_by_default
+          else
+            options[:only_path] = false
+          end
+        end
+
         super(options)
       when :back
         _back_url
-      when Symbol
-        ActionDispatch::Routing::PolymorphicRoutes::HelperMethodBuilder.path.handle_string_call self, options
       when Array
-        polymorphic_path(options, options.extract_options!)
-      when Class
-        ActionDispatch::Routing::PolymorphicRoutes::HelperMethodBuilder.path.handle_class_call self, options
+        if _generate_paths_by_default
+          polymorphic_path(options, options.extract_options!)
+        else
+          polymorphic_url(options, options.extract_options!)
+        end
       else
-        ActionDispatch::Routing::PolymorphicRoutes::HelperMethodBuilder.path.handle_model_call self, options
+        method = _generate_paths_by_default ? :path : :url
+        builder = ActionDispatch::Routing::PolymorphicRoutes::HelperMethodBuilder.send(method)
+
+        case options
+        when Symbol
+          builder.handle_string_call(self, options)
+        when Class
+          builder.handle_class_call(self, options)
+        else
+          builder.handle_model_call(self, options)
+        end
       end
     end