From abee0343686b476139c476952b1c68f1fba6b3d0 Mon Sep 17 00:00:00 2001
From: lulalala <mark@goodlife.tw>
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
---
.../test/template/active_model_helper_test.rb | 10 ++--
.../test/template/form_options_helper_test.rb | 14 ++++-
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 +-
.../has_many_through_associations_test.rb | 16 +++---
activerecord/test/models/reply.rb | 12 ++---
guides/source/active_record_validations.md | 12 ++---
13 files changed, 155 insertions(+), 50 deletions(-)
diff --git a/actionview/test/template/active_model_helper_test.rb b/actionview/test/template/active_model_helper_test.rb
index 36afed6dd6..c6a3c9064f 100644
--- a/actionview/test/template/active_model_helper_test.rb
+++ b/actionview/test/template/active_model_helper_test.rb
@@ -20,11 +20,11 @@ class ActiveModelHelperTest < ActionView::TestCase
super
@post = Post.new
- @post.errors[:author_name] << "can't be empty"
- @post.errors[:body] << "foo"
- @post.errors[:category] << "must exist"
- @post.errors[:published] << "must be accepted"
- @post.errors[:updated_at] << "bar"
+ assert_deprecated { @post.errors[:author_name] << "can't be empty" }
+ assert_deprecated { @post.errors[:body] << "foo" }
+ assert_deprecated { @post.errors[:category] << "must exist" }
+ assert_deprecated { @post.errors[:published] << "must be accepted" }
+ assert_deprecated { @post.errors[:updated_at] << "bar" }
@post.author_name = ""
@post.body = "Back to the hill and over it again!"
diff --git a/actionview/test/template/form_options_helper_test.rb b/actionview/test/template/form_options_helper_test.rb
index 4ccd3ae336..c9c36917d6 100644
--- a/actionview/test/template/form_options_helper_test.rb
+++ b/actionview/test/template/form_options_helper_test.rb
@@ -29,10 +29,20 @@ class FormOptionsHelperTest < ActionView::TestCase
end
Continent = Struct.new("Continent", :continent_name, :countries)
Country = Struct.new("Country", :country_id, :country_name)
- Firm = Struct.new("Firm", :time_zone)
Album = Struct.new("Album", :id, :title, :genre)
end
+ class Firm
+ include ActiveModel::Validations
+ extend ActiveModel::Naming
+
+ attr_accessor :time_zone
+
+ def initialize(time_zone = nil)
+ @time_zone = time_zone
+ end
+ end
+
module FakeZones
FakeZone = Struct.new(:name) do
def to_s; name; end
@@ -1294,7 +1304,7 @@ class FormOptionsHelperTest < ActionView::TestCase
def test_time_zone_select_with_priority_zones_and_errors
@firm = Firm.new("D")
@firm.extend ActiveModel::Validations
- @firm.errors[:time_zone] << "invalid"
+ assert_deprecated { @firm.errors[:time_zone] << "invalid" }
zones = [ ActiveSupport::TimeZone.new("A"), ActiveSupport::TimeZone.new("D") ]
html = time_zone_select("firm", "time_zone", zones)
assert_dom_equal "<div class=\"field_with_errors\">" \
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
diff --git a/activerecord/test/cases/associations/has_many_through_associations_test.rb b/activerecord/test/cases/associations/has_many_through_associations_test.rb
index fa540c9dab..620c89ddca 100644
--- a/activerecord/test/cases/associations/has_many_through_associations_test.rb
+++ b/activerecord/test/cases/associations/has_many_through_associations_test.rb
@@ -746,10 +746,14 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
firm = companies(:first_firm)
lifo = Developer.new(name: "lifo")
- assert_raises(ActiveRecord::RecordInvalid) { firm.developers << lifo }
+ assert_raises(ActiveRecord::RecordInvalid) do
+ assert_deprecated { firm.developers << lifo }
+ end
lifo = Developer.create!(name: "lifo")
- assert_raises(ActiveRecord::RecordInvalid) { firm.developers << lifo }
+ assert_raises(ActiveRecord::RecordInvalid) do
+ assert_deprecated { firm.developers << lifo }
+ end
end
end
@@ -1160,7 +1164,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
def test_create_should_not_raise_exception_when_join_record_has_errors
repair_validations(Categorization) do
Categorization.validate { |r| r.errors[:base] << "Invalid Categorization" }
- Category.create(name: "Fishing", authors: [Author.first])
+ assert_deprecated { Category.create(name: "Fishing", authors: [Author.first]) }
end
end
@@ -1173,7 +1177,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
repair_validations(Categorization) do
Categorization.validate { |r| r.errors[:base] << "Invalid Categorization" }
assert_raises(ActiveRecord::RecordInvalid) do
- Category.create!(name: "Fishing", authors: [Author.first])
+ assert_deprecated { Category.create!(name: "Fishing", authors: [Author.first]) }
end
end
end
@@ -1183,7 +1187,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
Categorization.validate { |r| r.errors[:base] << "Invalid Categorization" }
c = Category.new(name: "Fishing", authors: [Author.first])
assert_raises(ActiveRecord::RecordInvalid) do
- c.save!
+ assert_deprecated { c.save! }
end
end
end
@@ -1192,7 +1196,7 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
repair_validations(Categorization) do
Categorization.validate { |r| r.errors[:base] << "Invalid Categorization" }
c = Category.new(name: "Fishing", authors: [Author.first])
- assert_not c.save
+ assert_deprecated { assert_not c.save }
end
end
diff --git a/activerecord/test/models/reply.rb b/activerecord/test/models/reply.rb
index b35623a344..f6ab9c8a8f 100644
--- a/activerecord/test/models/reply.rb
+++ b/activerecord/test/models/reply.rb
@@ -34,29 +34,29 @@ class WrongReply < Reply
validate :check_author_name_is_secret, on: :special_case
def check_empty_title
- errors[:title] << "Empty" unless attribute_present?("title")
+ errors.add(:title, "Empty") unless attribute_present?("title")
end
def errors_on_empty_content
- errors[:content] << "Empty" unless attribute_present?("content")
+ errors.add(:content, "Empty") unless attribute_present?("content")
end
def check_content_mismatch
if attribute_present?("title") && attribute_present?("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 attribute_present?("title") && title == "Wrong Create"
+ errors.add(:title, "is Wrong Create") if attribute_present?("title") && title == "Wrong Create"
end
def check_wrong_update
- errors[:title] << "is Wrong Update" if attribute_present?("title") && title == "Wrong Update"
+ errors.add(:title, "is Wrong Update") if attribute_present?("title") && title == "Wrong Update"
end
def check_author_name_is_secret
- errors[:author_name] << "Invalid" unless author_name == "secret"
+ errors.add(:author_name, "Invalid") unless author_name == "secret"
end
end
diff --git a/guides/source/active_record_validations.md b/guides/source/active_record_validations.md
index e68f34dd5d..f904d4de65 100644
--- a/guides/source/active_record_validations.md
+++ b/guides/source/active_record_validations.md
@@ -664,7 +664,7 @@ This helper passes the record to a separate class for validation.
class GoodnessValidator < ActiveModel::Validator
def validate(record)
if record.first_name == "Evil"
- record.errors[:base] << "This person is evil"
+ record.errors.add :base, "This person is evil"
end
end
end
@@ -692,7 +692,7 @@ validator class as `options`:
class GoodnessValidator < ActiveModel::Validator
def validate(record)
if options[:fields].any?{|field| record.send(field) == "Evil" }
- record.errors[:base] << "This person is evil"
+ record.errors.add :base, "This person is evil"
end
end
end
@@ -723,7 +723,7 @@ class GoodnessValidator
def validate
if some_complex_condition_involving_ivars_and_private_methods?
- @person.errors[:base] << "This person is evil"
+ @person.errors.add :base, "This person is evil"
end
end
@@ -1004,7 +1004,7 @@ and performs the validation on it. The custom validator is called using the
class MyValidator < ActiveModel::Validator
def validate(record)
unless record.name.starts_with? 'X'
- record.errors[:name] << 'Need a name starting with X please!'
+ record.errors.add :name, "Need a name starting with X please!"
end
end
end
@@ -1026,7 +1026,7 @@ instance.
class EmailValidator < ActiveModel::EachValidator
def validate_each(record, attribute, value)
unless value =~ /\A([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})\z/i
- record.errors[attribute] << (options[:message] || "is not an email")
+ record.errors.add attribute, (options[:message] || "is not an email")
end
end
end
@@ -1203,7 +1203,7 @@ You can add error messages that are related to the object's state as a whole, in
```ruby
class Person < ApplicationRecord
def a_method_used_for_validation_purposes
- errors[:base] << "This person is invalid because ..."
+ errors.add :base, "This person is invalid because ..."
end
end
```
--
cgit v1.2.3