Add MailDeliveryJob for unified mail delivery

Add `MailDeliveryJob` for delivering both regular and parameterized mail.
Deprecate using `DeliveryJob` and `Parameterized::DeliveryJob`.

actionmailer/CHANGELOG.md

@@ -1,4 +1,4 @@
-*   Deliver parameterized mail with `ActionMailer::DeliveryJob` and remove `ActionMailer::Parameterized::DeliveryJob`.
+*   Add `MailDeliveryJob` for delivering both regular and parameterized mail. Deprecate using `DeliveryJob` and `Parameterized::DeliveryJob`.
 
     *Gannon McGibbon*
 

actionmailer/lib/action_mailer.rb

@@ -52,6 +52,7 @@ module ActionMailer
   autoload :TestHelper
   autoload :MessageDelivery
   autoload :DeliveryJob
+  autoload :MailDeliveryJob
 
   def self.eager_load!
     super

actionmailer/lib/action_mailer/base.rb

@@ -461,7 +461,7 @@ module ActionMailer
 
     helper ActionMailer::MailHelper
 
-    class_attribute :delivery_job, default: ::ActionMailer::DeliveryJob
+    class_attribute :delivery_job, default: ::ActionMailer::MailDeliveryJob
     class_attribute :default_params, default: {
       mime_version: "1.0",
       charset:      "UTF-8",

actionmailer/lib/action_mailer/delivery_job.rb

@@ -12,9 +12,16 @@ module ActionMailer
 
     rescue_from StandardError, with: :handle_exception_with_mailer_class
 
-    def perform(mailer, mail_method, delivery_method, params, *args) #:nodoc:
+    before_perform do
-      mailer_class = params ? mailer.constantize.with(params) : mailer.constantize
+      ActiveSupport::Deprecation.warn <<~MSG.squish
-      mailer_class.public_send(mail_method, *args).send(delivery_method)
+        Sending mail with DeliveryJob and Parameterized::DeliveryJob
+        is deprecated and will be removed in Rails 6.1.
+        Please use MailDeliveryJob instead.
+      MSG
+    end
+
+    def perform(mailer, mail_method, delivery_method, *args) #:nodoc:
+      mailer.constantize.public_send(mail_method, *args).send(delivery_method)
     end
 
     private

actionmailer/lib/action_mailer/mail_delivery_job.rb

@@ -0,0 +1,38 @@
+# frozen_string_literal: true
+
+require "active_job"
+
+module ActionMailer
+  # The <tt>ActionMailer::NewDeliveryJob</tt> class is used when you
+  # want to send emails outside of the request-response cycle. It supports
+  # sending either parameterized or normal mail.
+  #
+  # Exceptions are rescued and handled by the mailer class.
+  class MailDeliveryJob < ActiveJob::Base # :nodoc:
+    queue_as { ActionMailer::Base.deliver_later_queue_name }
+
+    rescue_from StandardError, with: :handle_exception_with_mailer_class
+
+    def perform(mailer, mail_method, delivery_method, args:, params: nil) #:nodoc:
+      mailer_class = params ? mailer.constantize.with(params) : mailer.constantize
+      mailer_class.public_send(mail_method, *args).send(delivery_method)
+    end
+
+    private
+      # "Deserialize" the mailer class name by hand in case another argument
+      # (like a Global ID reference) raised DeserializationError.
+      def mailer_class
+        if mailer = Array(@serialized_arguments).first || Array(arguments).first
+          mailer.constantize
+        end
+      end
+
+      def handle_exception_with_mailer_class(exception)
+        if klass = mailer_class
+          klass.handle_exception exception
+        else
+          raise exception
+        end
+      end
+  end
+end

actionmailer/lib/action_mailer/message_delivery.rb

@@ -135,10 +135,18 @@ module ActionMailer
             "#deliver_later, 2. only touch the message *within your mailer " \
             "method*, or 3. use a custom Active Job instead of #deliver_later."
         else
-          args = @mailer_class.name, @action.to_s, delivery_method.to_s, nil, *@args
           job = @mailer_class.delivery_job
+          args = arguments_for(job, delivery_method)
           job.set(options).perform_later(*args)
         end
       end
+
+      def arguments_for(delivery_job, delivery_method)
+        if delivery_job <= MailDeliveryJob
+          [@mailer_class.name, @action.to_s, delivery_method.to_s, args: @args]
+        else
+          [@mailer_class.name, @action.to_s, delivery_method.to_s, *@args]
+        end
+      end
   end
 end

actionmailer/lib/action_mailer/parameterized.rb

@@ -121,6 +121,12 @@ module ActionMailer
         end
     end
 
+    class DeliveryJob < ActionMailer::DeliveryJob # :nodoc:
+      def perform(mailer, mail_method, delivery_method, params, *args)
+        mailer.constantize.with(params).public_send(mail_method, *args).send(delivery_method)
+      end
+    end
+
     class MessageDelivery < ActionMailer::MessageDelivery # :nodoc:
       def initialize(mailer_class, action, params, *args)
         super(mailer_class, action, *args)
@@ -139,11 +145,19 @@ module ActionMailer
           if processed?
             super
           else
-            args = @mailer_class.name, @action.to_s, delivery_method.to_s, @params, *@args
             job  = @mailer_class.delivery_job
+            args = arguments_for(job, delivery_method)
             job.set(options).perform_later(*args)
           end
         end
+
+        def arguments_for(delivery_job, delivery_method)
+          if delivery_job <= MailDeliveryJob
+            [@mailer_class.name, @action.to_s, delivery_method.to_s, params: @params, args: @args]
+          else
+            [@mailer_class.name, @action.to_s, delivery_method.to_s, @params, *@args]
+          end
+        end
     end
   end
 end

actionmailer/lib/action_mailer/test_helper.rb

@@ -125,9 +125,9 @@ module ActionMailer
     #   end
     def assert_enqueued_email_with(mailer, method, args: nil, queue: "mailers", &block)
       args = if args.is_a?(Hash)
-        [mailer.to_s, method.to_s, "deliver_now", args]
+        [mailer.to_s, method.to_s, "deliver_now", params: args, args: []]
       else
-        [mailer.to_s, method.to_s, "deliver_now", nil, *args]
+        [mailer.to_s, method.to_s, "deliver_now", args: Array(args)]
       end
       assert_enqueued_with(job: mailer.delivery_job, args: args, queue: queue, &block)
     end

actionmailer/test/legacy_delivery_job_test.rb

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+
+require "abstract_unit"
+require "active_job"
+require "mailers/params_mailer"
+require "mailers/delayed_mailer"
+
+class LegacyDeliveryJobTest < ActiveSupport::TestCase
+  include ActiveJob::TestHelper
+
+  class LegacyDeliveryJob < ActionMailer::DeliveryJob
+  end
+
+  class LegacyParmeterizedDeliveryJob < ActionMailer::Parameterized::DeliveryJob
+  end
+
+  setup do
+    @previous_logger = ActiveJob::Base.logger
+    ActiveJob::Base.logger = Logger.new(nil)
+
+    @previous_delivery_method = ActionMailer::Base.delivery_method
+    ActionMailer::Base.delivery_method = :test
+
+    @previous_deliver_later_queue_name = ActionMailer::Base.deliver_later_queue_name
+    ActionMailer::Base.deliver_later_queue_name = :test_queue
+  end
+
+  teardown do
+    ActiveJob::Base.logger = @previous_logger
+    ParamsMailer.deliveries.clear
+
+    ActionMailer::Base.delivery_method = @previous_delivery_method
+    ActionMailer::Base.deliver_later_queue_name = @previous_deliver_later_queue_name
+  end
+
+  test "should send parameterized mail correctly" do
+    mail = ParamsMailer.with(inviter: "david@basecamp.com", invitee: "jason@basecamp.com").invitation
+    args = [
+      "ParamsMailer",
+      "invitation",
+      "deliver_now",
+      { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" },
+    ]
+
+    with_delivery_job(LegacyParmeterizedDeliveryJob) do
+      assert_deprecated do
+        assert_performed_with(job: LegacyParmeterizedDeliveryJob, args: args) do
+          mail.deliver_later
+        end
+      end
+    end
+  end
+
+  test "should send mail correctly" do
+    mail = DelayedMailer.test_message(1, 2, 3)
+    args = [
+      "DelayedMailer",
+      "test_message",
+      "deliver_now",
+      1,
+      2,
+      3,
+    ]
+
+    with_delivery_job(LegacyDeliveryJob) do
+      assert_deprecated do
+        assert_performed_with(job: LegacyDeliveryJob, args: args) do
+          mail.deliver_later
+        end
+      end
+    end
+  end
+
+  private
+
+    def with_delivery_job(job)
+      old_params_delivery_job = ParamsMailer.delivery_job
+      old_regular_delivery_job = DelayedMailer.delivery_job
+      ParamsMailer.delivery_job = job
+      DelayedMailer.delivery_job = job
+      yield
+    ensure
+      ParamsMailer.delivery_job = old_params_delivery_job
+      DelayedMailer.delivery_job = old_regular_delivery_job
+    end
+end

actionmailer/test/message_delivery_test.rb

@@ -64,20 +64,20 @@ class MessageDeliveryTest < ActiveSupport::TestCase
   end
 
   test "should enqueue the email with :deliver_now delivery method" do
-    assert_performed_with(job: ActionMailer::DeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3]) do
+    assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do
       @mail.deliver_later
     end
   end
 
   test "should enqueue the email with :deliver_now! delivery method" do
-    assert_performed_with(job: ActionMailer::DeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now!", nil, 1, 2, 3]) do
+    assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now!", args: [1, 2, 3]]) do
       @mail.deliver_later!
     end
   end
 
   test "should enqueue a delivery with a delay" do
     travel_to Time.new(2004, 11, 24, 01, 04, 44) do
-      assert_performed_with(job: ActionMailer::DeliveryJob, at: Time.current + 10.minutes, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3]) do
+      assert_performed_with(job: ActionMailer::MailDeliveryJob, at: Time.current + 10.minutes, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do
         @mail.deliver_later wait: 10.minutes
       end
     end
@@ -85,13 +85,13 @@ class MessageDeliveryTest < ActiveSupport::TestCase
 
   test "should enqueue a delivery at a specific time" do
     later_time = Time.current + 1.hour
-    assert_performed_with(job: ActionMailer::DeliveryJob, at: later_time, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3]) do
+    assert_performed_with(job: ActionMailer::MailDeliveryJob, at: later_time, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do
       @mail.deliver_later wait_until: later_time
     end
   end
 
   test "should enqueue the job on the correct queue" do
-    assert_performed_with(job: ActionMailer::DeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3], queue: "test_queue") do
+    assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]], queue: "test_queue") do
       @mail.deliver_later
     end
   end
@@ -100,17 +100,17 @@ class MessageDeliveryTest < ActiveSupport::TestCase
     old_delivery_job = DelayedMailer.delivery_job
     DelayedMailer.delivery_job = DummyJob
 
-    assert_performed_with(job: DummyJob, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3]) do
+    assert_performed_with(job: DummyJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]]) do
       @mail.deliver_later
     end
 
     DelayedMailer.delivery_job = old_delivery_job
   end
 
-  class DummyJob < ActionMailer::DeliveryJob; end
+  class DummyJob < ActionMailer::MailDeliveryJob; end
 
   test "can override the queue when enqueuing mail" do
-    assert_performed_with(job: ActionMailer::DeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", nil, 1, 2, 3], queue: "another_queue") do
+    assert_performed_with(job: ActionMailer::MailDeliveryJob, args: ["DelayedMailer", "test_message", "deliver_now", args: [1, 2, 3]], queue: "another_queue") do
       @mail.deliver_later(queue: :another_queue)
     end
   end

actionmailer/test/parameterized_test.rb

@@ -7,7 +7,7 @@ require "mailers/params_mailer"
 class ParameterizedTest < ActiveSupport::TestCase
   include ActiveJob::TestHelper
 
-  class DummyDeliveryJob < ActionMailer::DeliveryJob
+  class DummyDeliveryJob < ActionMailer::MailDeliveryJob
   end
 
   setup do
@@ -42,9 +42,10 @@ class ParameterizedTest < ActiveSupport::TestCase
       "ParamsMailer",
       "invitation",
       "deliver_now",
-      { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" },
+      params: { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" },
+      args: [],
     ]
-    assert_performed_with(job: ActionMailer::DeliveryJob, args: args) do
+    assert_performed_with(job: ActionMailer::MailDeliveryJob, args: args) do
       @mail.deliver_later
     end
   end
@@ -68,7 +69,8 @@ class ParameterizedTest < ActiveSupport::TestCase
       "ParamsMailer",
       "invitation",
       "deliver_now",
-      { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" },
+      params: { inviter: "david@basecamp.com", invitee: "jason@basecamp.com" },
+      args: [],
     ]
 
     with_delivery_job DummyDeliveryJob do

actionmailer/test/test_helper_test.rb

@@ -24,7 +24,7 @@ class TestHelperMailer < ActionMailer::Base
   end
 end
 
-class CustomDeliveryJob < ActionMailer::DeliveryJob
+class CustomDeliveryJob < ActionMailer::MailDeliveryJob
 end
 
 class CustomDeliveryMailer < TestHelperMailer