From 65f2180f19b042346b0a2c5482a1ae6208428ade Mon Sep 17 00:00:00 2001 From: Enrique Gonzalez Date: Thu, 29 Aug 2019 22:41:39 -0400 Subject: [PATCH] Fix FrozenError on ActiveModel::Error clear and delete This PR fixes a `FrozenError` when attempting to clear or delete `ActiveModel::Errors` through the deprecated array methods. In particular the error happens in the following situations: # calling `clear` on the deprecated array errors[:some_attribute].clear # calling `delete` on the deprecated messages hash errors.messages.delete(:some_attribute) Following the recent introduction of `ActiveModel::Error`, this PR adds deprecation warnings for those two messages. --- activemodel/lib/active_model/errors.rb | 12 ++++++++++++ activemodel/test/cases/errors_test.rb | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/activemodel/lib/active_model/errors.rb b/activemodel/lib/active_model/errors.rb index c91ac69603..87201c62d0 100644 --- a/activemodel/lib/active_model/errors.rb +++ b/activemodel/lib/active_model/errors.rb @@ -569,6 +569,12 @@ module ActiveModel __setobj__ prepare_content end + def delete(attribute) + ActiveSupport::Deprecation.warn("Calling `delete` to an ActiveModel::Errors messages hash is deprecated. Please call `ActiveModel::Errors#delete` instead.") + + @errors.delete(attribute) + end + private def prepare_content content = @errors.to_hash @@ -599,6 +605,12 @@ module ActiveModel __setobj__ @errors.messages_for(@attribute) self end + + def clear + ActiveSupport::Deprecation.warn("Calling `clear` to an ActiveModel::Errors message array in order to delete all errors is deprecated. Please call `ActiveModel::Errors#delete` instead.") + + @errors.delete(@attribute) + end end class DeprecationHandlingDetailsHash < SimpleDelegator diff --git a/activemodel/test/cases/errors_test.rb b/activemodel/test/cases/errors_test.rb index a6cd1da717..3c636745a4 100644 --- a/activemodel/test/cases/errors_test.rb +++ b/activemodel/test/cases/errors_test.rb @@ -103,6 +103,15 @@ class ErrorsTest < ActiveModel::TestCase assert_empty person.errors end + test "clear errors by key" do + person = Person.new + person.validate! + + assert_equal 1, person.errors.count + assert_deprecated { person.errors[:name].clear } + assert_empty person.errors + end + test "error access is indifferent" do errors = ActiveModel::Errors.new(Person.new) errors.add(:name, "omg") @@ -619,6 +628,15 @@ class ErrorsTest < ActiveModel::TestCase ) end + test "messages delete (deprecated)" do + person = Person.new + person.validate! + + assert_equal 1, person.errors.count + assert_deprecated { person.errors.messages.delete(:name) } + assert_empty person.errors + end + test "group_by_attribute" do person = Person.new error = person.errors.add(:name, :invalid, message: "is bad")