Merge pull request #38576 from Edouard-chin/ec-activejob-deprecation

Fix AJ wrong deprecation message on `after_callbacks_if_terminated`:

actionpack/lib/action_controller/metal/instrumentation.rb

@@ -72,7 +72,7 @@ def redirect_to(*)
 
   private
     # A hook invoked every time a before callback is halted.
-    def halted_callback_hook(filter)
+    def halted_callback_hook(filter, _)
       ActiveSupport::Notifications.instrument("halted_callback.action_controller", filter: filter)
     end
 

activejob/lib/active_job/callbacks.rb

@@ -179,14 +179,19 @@ def around_enqueue(*filters, &blk)
     end
 
     private
-      def warn_against_after_callbacks_execution_deprecation(callbacks) # :nodoc:
+      def halted_callback_hook(_filter, name) # :nodoc:
+        return super unless %i(enqueue perform).include?(name.to_sym)
+        callbacks = public_send("_#{name}_callbacks")
+
         if !self.class.skip_after_callbacks_if_terminated && callbacks.any? { |c| c.kind == :after }
           ActiveSupport::Deprecation.warn(<<~EOM)
             In Rails 6.2, `after_enqueue`/`after_perform` callbacks no longer run if `before_enqueue`/`before_perform` respectively halts with `throw :abort`.
             To enable this behavior, uncomment the `config.active_job.skip_after_callbacks_if_terminated` config
             in the new 6.1 framework defaults initializer.
           EOM
         end
+
+        super
       end
   end
 end

activejob/lib/active_job/enqueuing.rb

@@ -65,8 +65,6 @@ def enqueue(options = {})
       if successfully_enqueued
         self
       else
-        warn_against_after_callbacks_execution_deprecation(_enqueue_callbacks)
-
         if self.class.return_false_on_aborted_enqueue
           false
         else

activejob/lib/active_job/execution.rb

@@ -43,14 +43,10 @@ def perform_now
       self.executions = (executions || 0) + 1
 
       deserialize_arguments_if_needed
-      successfully_performed = false
 
-      job = run_callbacks :perform do
-        perform(*arguments).tap { successfully_performed = true }
+      run_callbacks :perform do
+        perform(*arguments)
       end
-
-      warn_against_after_callbacks_execution_deprecation(_perform_callbacks) unless successfully_performed
-      job
     rescue => exception
       rescue_with_handler(exception) || raise
     end

activejob/lib/active_job/instrumentation.rb

@@ -29,7 +29,7 @@ def instrument(operation, payload = {}, &block)
           "#{operation}.active_job", payload.merge(adapter: queue_adapter, job: self), &enhanced_block
       end
 
-      def halted_callback_hook(_)
+      def halted_callback_hook(*)
         super
         @_halted_callback_hook_called = true
       end

activejob/test/cases/callbacks_test.rb

@@ -116,6 +116,29 @@ def perform
     ActiveJob::Base.skip_after_callbacks_if_terminated = prev
   end
 
+  test "#enqueue does not throw a deprecation warning when skip_after_callbacks_if_terminated_is false and job did not throw an abort" do
+    prev = ActiveJob::Base.skip_after_callbacks_if_terminated
+    ActiveJob::Base.skip_after_callbacks_if_terminated = false
+
+    job = Class.new(ActiveJob::Base) do
+      after_enqueue { nil }
+
+      around_enqueue do |_, block|
+        block.call
+      rescue ArgumentError
+        nil
+      end
+
+      before_enqueue { raise ArgumentError }
+    end
+
+    assert_not_deprecated do
+      job.perform_later
+    end
+  ensure
+    ActiveJob::Base.skip_after_callbacks_if_terminated = prev
+  end
+
   test "#perform does not run after_perform callbacks when skip_after_callbacks_if_terminated is true" do
     prev = ActiveJob::Base.skip_after_callbacks_if_terminated
     ActiveJob::Base.skip_after_callbacks_if_terminated = true
@@ -157,6 +180,29 @@ def perform
     ActiveJob::Base.skip_after_callbacks_if_terminated = prev
   end
 
+  test "#perform does not throw a deprecation warning when skip_after_callbacks_if_terminated_is false and job did not throw an abort" do
+    prev = ActiveJob::Base.skip_after_callbacks_if_terminated
+    ActiveJob::Base.skip_after_callbacks_if_terminated = false
+
+    job = Class.new(ActiveJob::Base) do
+      after_perform { nil }
+
+      around_perform do |_, block|
+        block.call
+      rescue ArgumentError
+        nil
+      end
+
+      before_perform { raise ArgumentError }
+    end
+
+    assert_not_deprecated do
+      job.perform_now
+    end
+  ensure
+    ActiveJob::Base.skip_after_callbacks_if_terminated = prev
+  end
+
   test "#enqueue returns self when the job was enqueued" do
     job = CallbackJob.new
     assert_equal job, job.enqueue

activesupport/CHANGELOG.md

@@ -1,3 +1,26 @@
+*   [Breaking change] `ActiveSupport::Callbacks#halted_callback_hook` now receive a 2nd argument:
+
+    `ActiveSupport::Callbacks#halted_callback_hook` now receive the name of the callback
+    being halted as second argument.
+    This change will allow you to differentiate which callbacks halted the chain
+    and act accordingly.
+
+    ```ruby
+      class Book < ApplicationRecord
+        before_save { throw(:abort) }
+        before_create { throw(:abort) }
+
+        def halted_callback_hook(filter, callback_name)
+          Rails.logger.info("Book couldn't be #{callback_name}d")
+        end
+
+        Book.create # => "Book couldn't be created"
+        book.save # => "Book couldn't be saved"
+      end
+    ```
+
+    *Edouard Chin*
+
 *   Support `prepend` with `ActiveSupport::Concern`.
 
     Allows a module with `extend ActiveSupport::Concern` to be prepended.
@@ -14,7 +37,7 @@
           prepend Imposter
         end
 
-    Class methods are prepended to the base class, concerning is also 
+    Class methods are prepended to the base class, concerning is also
     updated: `concerning :Imposter, prepend: true do`.
 
     *Jason Karns*, *Elia Schito*

activesupport/lib/active_support/callbacks.rb

@@ -143,7 +143,7 @@ def run_callbacks(kind)
       # A hook invoked every time a before callback is halted.
       # This can be overridden in ActiveSupport::Callbacks implementors in order
       # to provide better debugging/logging.
-      def halted_callback_hook(filter)
+      def halted_callback_hook(filter, name)
       end
 
       module Conditionals # :nodoc:
@@ -159,17 +159,17 @@ module Filters
         Environment = Struct.new(:target, :halted, :value)
 
         class Before
-          def self.build(callback_sequence, user_callback, user_conditions, chain_config, filter)
+          def self.build(callback_sequence, user_callback, user_conditions, chain_config, filter, name)
             halted_lambda = chain_config[:terminator]
 
             if user_conditions.any?
-              halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter)
+              halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter, name)
             else
-              halting(callback_sequence, user_callback, halted_lambda, filter)
+              halting(callback_sequence, user_callback, halted_lambda, filter, name)
             end
           end
 
-          def self.halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter)
+          def self.halting_and_conditional(callback_sequence, user_callback, user_conditions, halted_lambda, filter, name)
             callback_sequence.before do |env|
               target = env.target
               value  = env.value
@@ -179,7 +179,7 @@ def self.halting_and_conditional(callback_sequence, user_callback, user_conditio
                 result_lambda = -> { user_callback.call target, value }
                 env.halted = halted_lambda.call(target, result_lambda)
                 if env.halted
-                  target.send :halted_callback_hook, filter
+                  target.send :halted_callback_hook, filter, name
                 end
               end
 
@@ -188,7 +188,7 @@ def self.halting_and_conditional(callback_sequence, user_callback, user_conditio
           end
           private_class_method :halting_and_conditional
 
-          def self.halting(callback_sequence, user_callback, halted_lambda, filter)
+          def self.halting(callback_sequence, user_callback, halted_lambda, filter, name)
             callback_sequence.before do |env|
               target = env.target
               value  = env.value
@@ -197,9 +197,8 @@ def self.halting(callback_sequence, user_callback, halted_lambda, filter)
               unless halted
                 result_lambda = -> { user_callback.call target, value }
                 env.halted = halted_lambda.call(target, result_lambda)
-
                 if env.halted
-                  target.send :halted_callback_hook, filter
+                  target.send :halted_callback_hook, filter, name
                 end
               end
 
@@ -337,7 +336,7 @@ def apply(callback_sequence)
 
           case kind
           when :before
-            Filters::Before.build(callback_sequence, user_callback.make_lambda, user_conditions, chain_config, @filter)
+            Filters::Before.build(callback_sequence, user_callback.make_lambda, user_conditions, chain_config, @filter, name)
           when :after
             Filters::After.build(callback_sequence, user_callback.make_lambda, user_conditions, chain_config)
           when :around

activesupport/test/callbacks_test.rb

@@ -609,7 +609,7 @@ def self.set_save_callbacks
       set_callback :save, :after, :third
     end
 
-    attr_reader :history, :saved, :halted
+    attr_reader :history, :saved, :halted, :callback_name
     def initialize
       @history = []
     end
@@ -639,8 +639,9 @@ def save
       end
     end
 
-    def halted_callback_hook(filter)
+    def halted_callback_hook(filter, name)
       @halted = filter
+      @callback_name = name
     end
   end
 
@@ -823,6 +824,7 @@ def test_termination_invokes_hook
       terminator = CallbackTerminator.new
       terminator.save
       assert_equal :second, terminator.halted
+      assert_equal :save, terminator.callback_name
     end
 
     def test_block_never_called_if_terminated

guides/source/upgrading_ruby_on_rails.md

@@ -127,6 +127,25 @@ which formats your action accepts, i.e.
   format.any(:xml, :json) { render request.format.to_sym => @people }
 ```
 
+### `ActiveSupport::Callbacks#halted_callback_hook` now receive a second argument
+
+Active Support allows you to override the `halted_callback_hook` whenever a callback
+halts the chain. This method now receive a second argument which is the name of the callback being halted.
+If you have classes that override this method, make sure it accepts two arguments. Note that this is a breaking
+change without a prior deprecation cycle (for performance reasons).
+
+Example:
+
+```ruby
+  class Book < ApplicationRecord
+    before_save { throw(:abort) }
+    before_create { throw(:abort) }
+
+    def halted_callback_hook(filter, callback_name) # => This method now accepts 2 arguments instead of 1
+      Rails.logger.info("Book couldn't be #{callback_name}d")
+    end
+  end
+```
 
 Upgrading from Rails 5.2 to Rails 6.0
 -------------------------------------