From abee0343686b476139c476952b1c68f1fba6b3d0 Mon Sep 17 00:00:00 2001 From: lulalala Date: Sat, 31 Mar 2018 22:42:40 +0800 Subject: Raise deprecation for calling `[:f] = 'b'` or `[:f] << 'b'` Revert some tests to ensure back compatibility --- activemodel/lib/active_model/errors.rb | 50 ++++++++++++++++-- activemodel/test/cases/errors_test.rb | 60 +++++++++++++++++++--- .../cases/validations/validations_context_test.rb | 4 +- .../test/cases/validations/with_validation_test.rb | 8 +-- activemodel/test/cases/validations_test.rb | 2 +- activemodel/test/models/person_with_validator.rb | 2 +- activemodel/test/models/reply.rb | 10 ++-- activemodel/test/validators/email_validator.rb | 5 +- 8 files changed, 116 insertions(+), 25 deletions(-) (limited to 'activemodel') diff --git a/activemodel/lib/active_model/errors.rb b/activemodel/lib/active_model/errors.rb index 7c6346f577..5440988d27 100644 --- a/activemodel/lib/active_model/errors.rb +++ b/activemodel/lib/active_model/errors.rb @@ -302,11 +302,11 @@ module ActiveModel # person.errors.to_hash(true) # => {:name=>["name cannot be nil"]} def to_hash(full_messages = false) hash = {} - message_method = full_messages ? :full_message : :message + message_method = full_messages ? :full_messages_for : :messages_for group_by_attribute.each do |attribute, errors| - hash[attribute] = errors.map(&message_method) + hash[attribute] = public_send(message_method, attribute) end - hash + DeprecationHandlingMessageHash.new(hash, self) end alias :messages :to_hash @@ -458,7 +458,7 @@ module ActiveModel end def messages_for(attribute) - where(attribute).map(&:message).freeze + DeprecationHandlingMessageArray.new(where(attribute).map(&:message).freeze, self, attribute) end # Returns a full message for a given attribute. @@ -619,6 +619,48 @@ module ActiveModel end end + class DeprecationHandlingMessageHash < SimpleDelegator + def initialize(content, errors) + @errors = errors + super(content) + end + + def []=(attribute, value) + ActiveSupport::Deprecation.warn("Calling `[]=` to an ActiveModel::Errors is deprecated. Please call `ActiveModel::Errors#add` instead.") + + Array(value).each do |message| + @errors.add(attribute, message) + end + __setobj__ @errors.messages + value + end + + def [](attribute) + messages = super(attribute) + + return messages if messages + + __getobj__[attribute]= DeprecationHandlingMessageArray.new([], @errors, attribute) + end + end + + class DeprecationHandlingMessageArray < SimpleDelegator + def initialize(content, errors, attribute) + @errors = errors + @attribute = attribute + super(content) + end + + def <<(message) + ActiveSupport::Deprecation.warn("Calling `<<` to an ActiveModel::Errors message array in order to add an error is deprecated. Please call `ActiveModel::Errors#add` instead.") + + @errors.add(@attribute, message) + __setobj__ @errors[@attribute] + self + end + end + + # Raised when a validation cannot be corrected by end users and are considered # exceptional. # diff --git a/activemodel/test/cases/errors_test.rb b/activemodel/test/cases/errors_test.rb index 5dbbf910de..513bd163fc 100644 --- a/activemodel/test/cases/errors_test.rb +++ b/activemodel/test/cases/errors_test.rb @@ -39,7 +39,7 @@ class ErrorsTest < ActiveModel::TestCase def test_include? errors = ActiveModel::Errors.new(Person.new) - errors[:foo] << "omg" + assert_deprecated { errors[:foo] << "omg" } assert_includes errors, :foo, "errors should include :foo" assert_includes errors, "foo", "errors should include 'foo' as :foo" end @@ -93,8 +93,8 @@ class ErrorsTest < ActiveModel::TestCase test "values returns an array of messages" do errors = ActiveModel::Errors.new(Person.new) - errors.messages[:foo] = "omg" - errors.messages[:baz] = "zomg" + assert_deprecated { errors.messages[:foo] = "omg" } + assert_deprecated { errors.messages[:baz] = "zomg" } assert_deprecated do assert_equal ["omg", "zomg"], errors.values @@ -113,11 +113,11 @@ class ErrorsTest < ActiveModel::TestCase test "keys returns the error keys" do errors = ActiveModel::Errors.new(Person.new) - errors.add(:name) - errors.add(:age) + assert_deprecated { errors.messages[:foo] << "omg" } + assert_deprecated { errors.messages[:baz] << "zomg" } assert_deprecated do - assert_equal [:name, :age], errors.keys + assert_equal [:foo, :baz], errors.keys end end @@ -153,6 +153,32 @@ class ErrorsTest < ActiveModel::TestCase assert_equal ["cannot be nil"], person.errors[:name] end + test "add an error message on a specific attribute (deprecated)" do + person = Person.new + person.errors.add(:name, "cannot be blank") + assert_equal ["cannot be blank"], person.errors[:name] + end + + test "add an error message on a specific attribute with a defined type (deprecated)" do + person = Person.new + person.errors.add(:name, :blank, message: "cannot be blank") + assert_equal ["cannot be blank"], person.errors[:name] + end + + test "add an error with a symbol (deprecated)" do + person = Person.new + person.errors.add(:name, :blank) + message = person.errors.generate_message(:name, :blank) + assert_equal [message], person.errors[:name] + end + + test "add an error with a proc (deprecated)" do + person = Person.new + message = Proc.new { "cannot be blank" } + person.errors.add(:name, message) + assert_equal ["cannot be blank"], person.errors[:name] + end + test "add creates an error object and returns it" do person = Person.new error = person.errors.add(:name, :blank) @@ -532,6 +558,16 @@ class ErrorsTest < ActiveModel::TestCase assert_empty person.errors.details end + test "copy errors (deprecated)" do + errors = ActiveModel::Errors.new(Person.new) + errors.add(:name, :invalid) + person = Person.new + person.errors.copy!(errors) + + assert_equal [:name], person.errors.messages.keys + assert_equal [:name], person.errors.details.keys + end + test "copy errors" do errors = ActiveModel::Errors.new(Person.new) errors.add(:name, :invalid) @@ -544,6 +580,18 @@ class ErrorsTest < ActiveModel::TestCase end end + test "merge errors (deprecated)" do + errors = ActiveModel::Errors.new(Person.new) + errors.add(:name, :invalid) + + person = Person.new + person.errors.add(:name, :blank) + person.errors.merge!(errors) + + assert_equal({ name: ["can't be blank", "is invalid"] }, person.errors.messages) + assert_equal({ name: [{ error: :blank }, { error: :invalid }] }, person.errors.details) + end + test "merge errors" do errors = ActiveModel::Errors.new(Person.new) errors.add(:name, :invalid) diff --git a/activemodel/test/cases/validations/validations_context_test.rb b/activemodel/test/cases/validations/validations_context_test.rb index 024eb1882f..3d2dea9828 100644 --- a/activemodel/test/cases/validations/validations_context_test.rb +++ b/activemodel/test/cases/validations/validations_context_test.rb @@ -14,13 +14,13 @@ class ValidationsContextTest < ActiveModel::TestCase class ValidatorThatAddsErrors < ActiveModel::Validator def validate(record) - record.errors[:base] << ERROR_MESSAGE + record.errors.add(:base, ERROR_MESSAGE) end end class AnotherValidatorThatAddsErrors < ActiveModel::Validator def validate(record) - record.errors[:base] << ANOTHER_ERROR_MESSAGE + record.errors.add(:base, ANOTHER_ERROR_MESSAGE) end end diff --git a/activemodel/test/cases/validations/with_validation_test.rb b/activemodel/test/cases/validations/with_validation_test.rb index 8239792c79..e6ae6603f2 100644 --- a/activemodel/test/cases/validations/with_validation_test.rb +++ b/activemodel/test/cases/validations/with_validation_test.rb @@ -14,13 +14,13 @@ class ValidatesWithTest < ActiveModel::TestCase class ValidatorThatAddsErrors < ActiveModel::Validator def validate(record) - record.errors[:base] << ERROR_MESSAGE + record.errors.add(:base, message: ERROR_MESSAGE) end end class OtherValidatorThatAddsErrors < ActiveModel::Validator def validate(record) - record.errors[:base] << OTHER_ERROR_MESSAGE + record.errors.add(:base, message: OTHER_ERROR_MESSAGE) end end @@ -32,14 +32,14 @@ class ValidatesWithTest < ActiveModel::TestCase class ValidatorThatValidatesOptions < ActiveModel::Validator def validate(record) if options[:field] == :first_name - record.errors[:base] << ERROR_MESSAGE + record.errors.add(:base, message: ERROR_MESSAGE) end end end class ValidatorPerEachAttribute < ActiveModel::EachValidator def validate_each(record, attribute, value) - record.errors[attribute] << "Value is #{value}" + record.errors.add(attribute, message: "Value is #{value}") end end diff --git a/activemodel/test/cases/validations_test.rb b/activemodel/test/cases/validations_test.rb index 7776233db5..82bb66b550 100644 --- a/activemodel/test/cases/validations_test.rb +++ b/activemodel/test/cases/validations_test.rb @@ -74,7 +74,7 @@ class ValidationsTest < ActiveModel::TestCase def test_errors_on_nested_attributes_expands_name t = Topic.new - t.errors["replies.name"] << "can't be blank" + assert_deprecated { t.errors["replies.name"] << "can't be blank" } assert_equal ["Replies name can't be blank"], t.errors.full_messages end diff --git a/activemodel/test/models/person_with_validator.rb b/activemodel/test/models/person_with_validator.rb index 44e78cbc29..fbb28d2a0f 100644 --- a/activemodel/test/models/person_with_validator.rb +++ b/activemodel/test/models/person_with_validator.rb @@ -5,7 +5,7 @@ class PersonWithValidator class PresenceValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - record.errors[attribute] << "Local validator#{options[:custom]}" if value.blank? + record.errors.add(attribute, message: "Local validator#{options[:custom]}") if value.blank? end end diff --git a/activemodel/test/models/reply.rb b/activemodel/test/models/reply.rb index 6bb18f95fe..f340b6fb14 100644 --- a/activemodel/test/models/reply.rb +++ b/activemodel/test/models/reply.rb @@ -11,24 +11,24 @@ class Reply < Topic validate :check_wrong_update, on: :update def check_empty_title - errors[:title] << "is Empty" unless title && title.size > 0 + errors.add(:title, "is Empty") unless title && title.size > 0 end def errors_on_empty_content - errors[:content] << "is Empty" unless content && content.size > 0 + errors.add(:content, "is Empty") unless content && content.size > 0 end def check_content_mismatch if title && content && content == "Mismatch" - errors[:title] << "is Content Mismatch" + errors.add(:title, "is Content Mismatch") end end def title_is_wrong_create - errors[:title] << "is Wrong Create" if title && title == "Wrong Create" + errors.add(:title, "is Wrong Create") if title && title == "Wrong Create" end def check_wrong_update - errors[:title] << "is Wrong Update" if title && title == "Wrong Update" + errors.add(:title, "is Wrong Update") if title && title == "Wrong Update" end end diff --git a/activemodel/test/validators/email_validator.rb b/activemodel/test/validators/email_validator.rb index 0c634d8659..774a10b2ba 100644 --- a/activemodel/test/validators/email_validator.rb +++ b/activemodel/test/validators/email_validator.rb @@ -2,7 +2,8 @@ class EmailValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) - record.errors[attribute] << (options[:message] || "is not an email") unless - /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i.match?(value) + unless /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i.match?(value) + record.errors.add(attribute, message: options[:message] || "is not an email") + end end end -- cgit v1.2.3