Deprecate `ActiveModel::Errors` `get`, `set` and `[]=` methods.

They have inconsistent behaviour currently.

3 files changed, 54 insertions, 24 deletions

diff --git a/activemodel/CHANGELOG.md b/activemodel/CHANGELOG.md
index 4c78fc91a5..233d9b175c 100644
--- a/activemodel/CHANGELOG.md
+++ b/activemodel/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Deprecate `ActiveModel::Errors#get`, `ActiveModel::Errors#set` and
+    `ActiveModel::Errors#[]=` methods that have inconsistent behaviour.
+
+    *Wojciech Wnętrzak*
+
 *   Change the default error message from `can't be blank` to `must exist` for
     the presence validator of the `:required` option on `belongs_to`/`has_one` associations.
 
diff --git a/activemodel/lib/active_model/errors.rb b/activemodel/lib/active_model/errors.rb
index a809c72ccd..b1d090b830 100644
--- a/activemodel/lib/active_model/errors.rb
+++ b/activemodel/lib/active_model/errors.rb
@@ -3,6 +3,7 @@
 require 'active_support/core_ext/array/conversions'
 require 'active_support/core_ext/string/inflections'
 require 'active_support/core_ext/object/deep_dup'
+require 'active_support/deprecation'
 
 module ActiveModel
   # == Active \Model \Errors
@@ -72,7 +73,7 @@ module ActiveModel
     #   end
     def initialize(base)
       @base     = base
-      @messages = {}
+      @messages = Hash.new { |messages, attribute| messages[attribute] = [] }
       @details  = Hash.new { |details, attribute| details[attribute] = [] }
     end
 
@@ -110,8 +111,14 @@ module ActiveModel
     #
     #   person.errors.messages   # => {:name=>["cannot be nil"]}
     #   person.errors.get(:name) # => ["cannot be nil"]
-    #   person.errors.get(:age)  # => nil
+    #   person.errors.get(:age)  # => []
     def get(key)
+      ActiveSupport::Deprecation.warn(<<-MESSAGE.squish)
+        ActiveModel::Errors#get is deprecated and will be removed in Rails 5.1
+
+        To achieve the same use messages[:#{key}]
+      MESSAGE
+
       messages[key]
     end
 
@@ -121,6 +128,12 @@ module ActiveModel
     #   person.errors.set(:name, ["can't be nil"])
     #   person.errors.get(:name) # => ["can't be nil"]
     def set(key, value)
+      ActiveSupport::Deprecation.warn(<<-MESSAGE.squish)
+        ActiveModel::Errors#set is deprecated and will be removed in Rails 5.1
+
+        To achieve the same use messages[:#{key}] = "#{value}"
+      MESSAGE
+
       messages[key] = value
     end
 
@@ -128,7 +141,7 @@ module ActiveModel
     #
     #   person.errors.get(:name)    # => ["cannot be nil"]
     #   person.errors.delete(:name) # => ["cannot be nil"]
-    #   person.errors.get(:name)    # => nil
+    #   person.errors.get(:name)    # => []
     def delete(key)
       messages.delete(key)
       details.delete(key)
@@ -140,7 +153,7 @@ module ActiveModel
     #   person.errors[:name]  # => ["cannot be nil"]
     #   person.errors['name'] # => ["cannot be nil"]
     def [](attribute)
-      get(attribute.to_sym) || set(attribute.to_sym, [])
+      messages[attribute.to_sym]
     end
 
     # Adds to the supplied attribute the supplied error message.
@@ -148,7 +161,13 @@ module ActiveModel
     #   person.errors[:name] = "must be set"
     #   person.errors[:name] # => ['must be set']
     def []=(attribute, error)
-      self[attribute] << error
+      ActiveSupport::Deprecation.warn(<<-MESSAGE.squish)
+        ActiveModel::Errors#[]= is deprecated and will be removed in Rails 5.1
+
+        To achieve the same use messages[:#{attribute}] << "#{error}"
+      MESSAGE
+
+      messages[attribute.to_sym] << error
     end
 
     # Iterates through each error key, value pair in the error messages hash.
@@ -167,7 +186,7 @@ module ActiveModel
     #   end
     def each
       messages.each_key do |attribute|
-        self[attribute].each { |error| yield attribute, error }
+        messages[attribute].each { |error| yield attribute, error }
       end
     end
 
@@ -317,8 +336,8 @@ module ActiveModel
         raise exception, full_message(attribute, message)
       end
 
-      details[attribute.to_sym] << detail
-      self[attribute] << message
+      details[attribute.to_sym]  << detail
+      messages[attribute.to_sym] << message
     end
 
     # Will add an error message to each of the attributes in +attributes+
@@ -384,7 +403,7 @@ module ActiveModel
     #   person.errors.full_messages_for(:name)
     #   # => ["Name is too short (minimum is 5 characters)", "Name can't be blank"]
     def full_messages_for(attribute)
-      (get(attribute) || []).map { |message| full_message(attribute, message) }
+      messages[attribute].map { |message| full_message(attribute, message) }
     end
 
     # Returns a full message for a given attribute.
diff --git a/activemodel/test/cases/errors_test.rb b/activemodel/test/cases/errors_test.rb
index 17dbb817d0..e736ccf6a0 100644
--- a/activemodel/test/cases/errors_test.rb
+++ b/activemodel/test/cases/errors_test.rb
@@ -29,28 +29,28 @@ class ErrorsTest < ActiveModel::TestCase
 
   def test_delete
     errors = ActiveModel::Errors.new(self)
-    errors[:foo] = 'omg'
+    errors[:foo] << 'omg'
     errors.delete(:foo)
     assert_empty errors[:foo]
   end
 
   def test_include?
     errors = ActiveModel::Errors.new(self)
-    errors[:foo] = 'omg'
+    errors[:foo] << 'omg'
     assert errors.include?(:foo), 'errors should include :foo'
   end
 
   def test_dup
     errors = ActiveModel::Errors.new(self)
-    errors[:foo] = 'bar'
+    errors[:foo] << 'bar'
     errors_dup = errors.dup
-    errors_dup[:bar] = 'omg'
+    errors_dup[:bar] << 'omg'
     assert_not_same errors_dup.messages, errors.messages
   end
 
   def test_has_key?
     errors = ActiveModel::Errors.new(self)
-    errors[:foo] = 'omg'
+    errors[:foo] << 'omg'
     assert_equal true, errors.has_key?(:foo), 'errors should have key :foo'
   end
 
@@ -61,7 +61,7 @@ class ErrorsTest < ActiveModel::TestCase
 
   def test_key?
     errors = ActiveModel::Errors.new(self)
-    errors[:foo] = 'omg'
+    errors[:foo] << 'omg'
     assert_equal true, errors.key?(:foo), 'errors should have key :foo'
   end
 
@@ -81,37 +81,41 @@ class ErrorsTest < ActiveModel::TestCase
 
   test "get returns the errors for the provided key" do
     errors = ActiveModel::Errors.new(self)
-    errors[:foo] = "omg"
+    errors[:foo] << "omg"
 
-    assert_equal ["omg"], errors.get(:foo)
+    assert_deprecated do
+      assert_equal ["omg"], errors.get(:foo)
+    end
   end
 
   test "sets the error with the provided key" do
     errors = ActiveModel::Errors.new(self)
-    errors.set(:foo, "omg")
+    assert_deprecated do
+      errors.set(:foo, "omg")
+    end
 
     assert_equal({ foo: "omg" }, errors.messages)
   end
 
   test "error access is indifferent" do
     errors = ActiveModel::Errors.new(self)
-    errors[:foo] = "omg"
+    errors[:foo] << "omg"
 
     assert_equal ["omg"], errors["foo"]
   end
 
   test "values returns an array of messages" do
     errors = ActiveModel::Errors.new(self)
-    errors.set(:foo, "omg")
-    errors.set(:baz, "zomg")
+    errors.messages[:foo] = "omg"
+    errors.messages[:baz] = "zomg"
 
     assert_equal ["omg", "zomg"], errors.values
   end
 
   test "keys returns the error keys" do
     errors = ActiveModel::Errors.new(self)
-    errors.set(:foo, "omg")
-    errors.set(:baz, "zomg")
+    errors.messages[:foo] << "omg"
+    errors.messages[:baz] << "zomg"
 
     assert_equal [:foo, :baz], errors.keys
   end
@@ -133,7 +137,9 @@ class ErrorsTest < ActiveModel::TestCase
 
   test "assign error" do
     person = Person.new
-    person.errors[:name] = 'should not be nil'
+    assert_deprecated do
+      person.errors[:name] = 'should not be nil'
+    end
     assert_equal ["should not be nil"], person.errors[:name]
   end