From d217daf6a740de7e4925872abe632982cfaab89b Mon Sep 17 00:00:00 2001
From: claudiob <claudiob@gmail.com>
Date: Sun, 14 Dec 2014 22:46:23 -0800
Subject: Deprecate `false` as the way to halt AS callbacks

After this commit, returning `false` in a callback will display a deprecation
warning to make developers aware of the fact that they need to explicitly
`throw(:abort)` if their intention is to halt a callback chain.

This commit also patches two internal uses of AS::Callbacks (inside
ActiveRecord and ActionDispatch) which sometimes return `false` but whose
returned value is not meaningful for the purpose of execution.

In both cases, the returned value is set to `true`, which does not affect the
execution of the callbacks but prevents unrequested deprecation warnings from
showing up.
---
 activerecord/lib/active_record/autosave_association.rb |  8 +++++++-
 activesupport/CHANGELOG.md                             |  8 ++++++++
 activesupport/lib/active_support/callbacks.rb          | 16 ++++++++++++++--
 activesupport/lib/active_support/i18n_railtie.rb       |  8 +++++++-
 4 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb
index c39b045a5e..5045e3065e 100644
--- a/activerecord/lib/active_record/autosave_association.rb
+++ b/activerecord/lib/active_record/autosave_association.rb
@@ -206,7 +206,13 @@ module ActiveRecord
 
           if reflection.validate? && !method_defined?(validation_method)
             method = (collection ? :validate_collection_association : :validate_single_association)
-            define_non_cyclic_method(validation_method) { send(method, reflection) }
+            define_non_cyclic_method(validation_method) do
+              send(method, reflection)
+              # TODO: remove the following line as soon as the return value of
+              # callbacks is ignored, that is, returning `false` does not
+              # display a deprecation warning or halts the callback chain.
+              true
+            end
             validate validation_method
           end
         end
diff --git a/activesupport/CHANGELOG.md b/activesupport/CHANGELOG.md
index 01bb816923..30e2bdd1e1 100644
--- a/activesupport/CHANGELOG.md
+++ b/activesupport/CHANGELOG.md
@@ -1,3 +1,11 @@
+*   Deprecate returning `false` as a way to halt callback chains.
+
+    Returning `false` in a callback will display a deprecation warning
+    explaining that the preferred method to halt a callback chain is to
+    explicitly `throw(:abort)`.
+
+    *claudiob*
+
 *   Changes arguments and default value of CallbackChain's :terminator option
 
     Chains of callbacks defined without an explicit `:terminator` option will
diff --git a/activesupport/lib/active_support/callbacks.rb b/activesupport/lib/active_support/callbacks.rb
index 3a3061e536..7dd97eb1ab 100644
--- a/activesupport/lib/active_support/callbacks.rb
+++ b/activesupport/lib/active_support/callbacks.rb
@@ -4,6 +4,7 @@ require 'active_support/core_ext/array/extract_options'
 require 'active_support/core_ext/class/attribute'
 require 'active_support/core_ext/kernel/reporting'
 require 'active_support/core_ext/kernel/singleton_class'
+require 'active_support/core_ext/string/filters'
 require 'thread'
 
 module ActiveSupport
@@ -595,12 +596,23 @@ module ActiveSupport
         Proc.new do |target, result_lambda|
           terminate = true
           catch(:abort) do
-            result_lambda.call if result_lambda.is_a?(Proc)
-            terminate = false
+            result = result_lambda.call if result_lambda.is_a?(Proc)
+            if result == false
+              display_deprecation_warning_for_false_terminator
+            else
+              terminate = false
+            end
           end
           terminate
         end
       end
+
+      def display_deprecation_warning_for_false_terminator
+        ActiveSupport::Deprecation.warn(<<-MSG.squish)
+          Returning `false` in a callback will not implicitly halt a callback chain in the next release of Rails.
+          To explicitly halt a callback chain, please use `throw :abort` instead.
+        MSG
+      end
     end
 
     module ClassMethods
diff --git a/activesupport/lib/active_support/i18n_railtie.rb b/activesupport/lib/active_support/i18n_railtie.rb
index affcfb7398..9e742b1917 100644
--- a/activesupport/lib/active_support/i18n_railtie.rb
+++ b/activesupport/lib/active_support/i18n_railtie.rb
@@ -55,7 +55,13 @@ module I18n
 
       reloader = ActiveSupport::FileUpdateChecker.new(I18n.load_path.dup){ I18n.reload! }
       app.reloaders << reloader
-      ActionDispatch::Reloader.to_prepare { reloader.execute_if_updated }
+      ActionDispatch::Reloader.to_prepare do
+        reloader.execute_if_updated
+        # TODO: remove the following line as soon as the return value of
+        # callbacks is ignored, that is, returning `false` does not
+        # display a deprecation warning or halts the callback chain.
+        true
+      end
       reloader.execute
 
       @i18n_inited = true
-- 
cgit v1.2.3