Change transaction callbacks to not swallowing errors.

Before this change any error raised inside a transaction callback
are rescued and printed in the logs.

Now these errors are not rescue anymore and just bubble up,
as the other callbacks.

6 files changed, 25 insertions, 80 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index 521d5fdb39..3a05a9b020 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,16 @@
+*   Deprecate `ActiveRecord::Base.errors_in_transactional_callbacks=`.
+
+    *Rafael Mendonça França*
+
+*   Change transaction callbacks to not swallowing errors.
+
+    Before this change any error raised inside a transaction callback are
+    rescued and printed in the logs.
+
+    Now these errors are not rescue anymore and just bubble up, as the other callbacks.
+
+    *Rafael Mendonça França*
+
 *   Remove deprecated `sanitize_sql_hash_for_conditions`.
 
     *Rafael Mendonça França*
diff --git a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
index fd666c8c39..f6ef3b0675 100644
--- a/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
+++ b/activerecord/lib/active_record/connection_adapters/abstract/transaction.rb
@@ -69,12 +69,7 @@ module ActiveRecord
       def rollback_records
         ite = records.uniq
         while record = ite.shift
-          begin
-            record.rolledback! full_rollback?
-          rescue => e
-            raise if ActiveRecord::Base.raise_in_transactional_callbacks
-            record.logger.error(e) if record.respond_to?(:logger) && record.logger
-          end
+          record.rolledback! full_rollback?
         end
       ensure
         ite.each do |i|
@@ -89,12 +84,7 @@ module ActiveRecord
       def commit_records
         ite = records.uniq
         while record = ite.shift
-          begin
-            record.committed!
-          rescue => e
-            raise if ActiveRecord::Base.raise_in_transactional_callbacks
-            record.logger.error(e) if record.respond_to?(:logger) && record.logger
-          end
+          record.committed!
         end
       ensure
         ite.each do |i|
diff --git a/activerecord/lib/active_record/transactions.rb b/activerecord/lib/active_record/transactions.rb
index 31ca90fb58..1c48196762 100644
--- a/activerecord/lib/active_record/transactions.rb
+++ b/activerecord/lib/active_record/transactions.rb
@@ -4,23 +4,10 @@ module ActiveRecord
     extend ActiveSupport::Concern
     #:nodoc:
     ACTIONS = [:create, :destroy, :update]
-    #:nodoc:
-    CALLBACK_WARN_MESSAGE = "Currently, Active Record suppresses errors raised " \
-      "within `after_rollback`/`after_commit` callbacks and only print them to " \
-      "the logs. In the next version, these errors will no longer be suppressed. " \
-      "Instead, the errors will propagate normally just like in other Active " \
-      "Record callbacks.\n" \
-      "\n" \
-      "You can opt into the new behavior and remove this warning by setting:\n" \
-      "\n" \
-      "  config.active_record.raise_in_transactional_callbacks = true\n\n"
 
     included do
       define_callbacks :commit, :rollback,
                        scope: [:kind, :name]
-
-      mattr_accessor :raise_in_transactional_callbacks, instance_writer: false
-      self.raise_in_transactional_callbacks = false
     end
 
     # = Active Record Transactions
@@ -236,9 +223,6 @@ module ActiveRecord
       def after_commit(*args, &block)
         set_options_for_callbacks!(args)
         set_callback(:commit, :after, *args, &block)
-        unless ActiveRecord::Base.raise_in_transactional_callbacks
-          ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE)
-        end
       end
 
       # This callback is called after a create, update, or destroy are rolled back.
@@ -247,9 +231,16 @@ module ActiveRecord
       def after_rollback(*args, &block)
         set_options_for_callbacks!(args)
         set_callback(:rollback, :after, *args, &block)
-        unless ActiveRecord::Base.raise_in_transactional_callbacks
-          ActiveSupport::Deprecation.warn(CALLBACK_WARN_MESSAGE)
-        end
+      end
+
+      def raise_in_transactional_callbacks
+        ActiveSupport::Deprecation.warn('ActiveRecord::Base.raise_in_transactional_callbacks is deprecated and will be removed without replacement.')
+        true
+      end
+
+      def raise_in_transactional_callbacks=(value)
+        ActiveSupport::Deprecation.warn('ActiveRecord::Base.raise_in_transactional_callbacks= is deprecated, has no effect and will be removed without replacement.')
+        value
       end
 
       private
diff --git a/activerecord/test/cases/helper.rb b/activerecord/test/cases/helper.rb
index 80ac57ec7c..925491acbd 100644
--- a/activerecord/test/cases/helper.rb
+++ b/activerecord/test/cases/helper.rb
@@ -24,9 +24,6 @@ ActiveSupport::Deprecation.debug = true
 # Disable available locale checks to avoid warnings running the test suite.
 I18n.enforce_available_locales = false
 
-# Enable raise errors in after_commit and after_rollback.
-ActiveRecord::Base.raise_in_transactional_callbacks = true
-
 # Connect to the database
 ARTest.connect
 
diff --git a/activerecord/test/cases/transaction_callbacks_test.rb b/activerecord/test/cases/transaction_callbacks_test.rb
index 0f5caa52e3..185fc22e98 100644
--- a/activerecord/test/cases/transaction_callbacks_test.rb
+++ b/activerecord/test/cases/transaction_callbacks_test.rb
@@ -266,47 +266,6 @@ class TransactionCallbacksTest < ActiveRecord::TestCase
     assert_equal 2, @first.rollbacks
   end
 
-  def test_after_transaction_callbacks_should_prevent_callbacks_from_being_called
-    old_transaction_config = ActiveRecord::Base.raise_in_transactional_callbacks
-    ActiveRecord::Base.raise_in_transactional_callbacks = false
-
-    def @first.last_after_transaction_error=(e); @last_transaction_error = e; end
-    def @first.last_after_transaction_error; @last_transaction_error; end
-    @first.after_commit_block{|r| r.last_after_transaction_error = :commit; raise "fail!";}
-    @first.after_rollback_block{|r| r.last_after_transaction_error = :rollback; raise "fail!";}
-
-    second = TopicWithCallbacks.find(3)
-    second.after_commit_block{|r| r.history << :after_commit}
-    second.after_rollback_block{|r| r.history << :after_rollback}
-
-    Topic.transaction do
-      @first.save!
-      second.save!
-    end
-    assert_equal :commit, @first.last_after_transaction_error
-    assert_equal [:after_commit], second.history
-
-    second.history.clear
-    Topic.transaction do
-      @first.save!
-      second.save!
-      raise ActiveRecord::Rollback
-    end
-    assert_equal :rollback, @first.last_after_transaction_error
-    assert_equal [:after_rollback], second.history
-  ensure
-    ActiveRecord::Base.raise_in_transactional_callbacks = old_transaction_config
-  end
-
-  def test_after_commit_should_not_raise_when_raise_in_transactional_callbacks_false
-    old_transaction_config = ActiveRecord::Base.raise_in_transactional_callbacks
-    ActiveRecord::Base.raise_in_transactional_callbacks = false
-    @first.after_commit_block{ fail "boom" }
-    Topic.transaction { @first.save! }
-  ensure
-    ActiveRecord::Base.raise_in_transactional_callbacks = old_transaction_config
-  end
-
   def test_after_commit_callback_should_not_swallow_errors
     @first.after_commit_block{ fail "boom" }
     assert_raises(RuntimeError) do
diff --git a/railties/lib/rails/generators/rails/app/templates/config/application.rb b/railties/lib/rails/generators/rails/app/templates/config/application.rb
index dc0cc07888..c59ffb3491 100644
--- a/railties/lib/rails/generators/rails/app/templates/config/application.rb
+++ b/railties/lib/rails/generators/rails/app/templates/config/application.rb
@@ -31,10 +31,5 @@ module <%= app_const_base %>
     # The default locale is :en and all translations from config/locales/*.rb,yml are auto loaded.
     # config.i18n.load_path += Dir[Rails.root.join('my', 'locales', '*.{rb,yml}').to_s]
     # config.i18n.default_locale = :de
-    <%- unless options.skip_active_record? -%>
-
-    # Do not swallow errors in after_commit/after_rollback callbacks.
-    config.active_record.raise_in_transactional_callbacks = true
-    <%- end -%>
   end
 end