Remove deprecated `:if` and `:unless` string filter for callbacks
This commit is contained in:
parent
216965e926
commit
c792354adc
|
@ -43,48 +43,6 @@ class ConditionalValidationTest < ActiveModel::TestCase
|
||||||
assert_equal ["hoo 5"], t.errors["title"]
|
assert_equal ["hoo 5"], t.errors["title"]
|
||||||
end
|
end
|
||||||
|
|
||||||
def test_if_validation_using_string_true
|
|
||||||
# When the evaluated string returns true
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", if: "a = 1; a == 1")
|
|
||||||
end
|
|
||||||
t = Topic.new("title" => "uhohuhoh", "content" => "whatever")
|
|
||||||
assert t.invalid?
|
|
||||||
assert t.errors[:title].any?
|
|
||||||
assert_equal ["hoo 5"], t.errors["title"]
|
|
||||||
end
|
|
||||||
|
|
||||||
def test_unless_validation_using_string_true
|
|
||||||
# When the evaluated string returns true
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", unless: "a = 1; a == 1")
|
|
||||||
end
|
|
||||||
t = Topic.new("title" => "uhohuhoh", "content" => "whatever")
|
|
||||||
assert t.valid?
|
|
||||||
assert_empty t.errors[:title]
|
|
||||||
end
|
|
||||||
|
|
||||||
def test_if_validation_using_string_false
|
|
||||||
# When the evaluated string returns false
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", if: "false")
|
|
||||||
end
|
|
||||||
t = Topic.new("title" => "uhohuhoh", "content" => "whatever")
|
|
||||||
assert t.valid?
|
|
||||||
assert_empty t.errors[:title]
|
|
||||||
end
|
|
||||||
|
|
||||||
def test_unless_validation_using_string_false
|
|
||||||
# When the evaluated string returns false
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}", unless: "false")
|
|
||||||
end
|
|
||||||
t = Topic.new("title" => "uhohuhoh", "content" => "whatever")
|
|
||||||
assert t.invalid?
|
|
||||||
assert t.errors[:title].any?
|
|
||||||
assert_equal ["hoo 5"], t.errors["title"]
|
|
||||||
end
|
|
||||||
|
|
||||||
def test_if_validation_using_block_true
|
def test_if_validation_using_block_true
|
||||||
# When the block returns true
|
# When the block returns true
|
||||||
Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}",
|
Topic.validates_length_of(:title, maximum: 5, too_long: "hoo %{count}",
|
||||||
|
@ -122,28 +80,4 @@ class ConditionalValidationTest < ActiveModel::TestCase
|
||||||
assert t.errors[:title].any?
|
assert t.errors[:title].any?
|
||||||
assert_equal ["hoo 5"], t.errors["title"]
|
assert_equal ["hoo 5"], t.errors["title"]
|
||||||
end
|
end
|
||||||
|
|
||||||
# previous implementation of validates_presence_of eval'd the
|
|
||||||
# string with the wrong binding, this regression test is to
|
|
||||||
# ensure that it works correctly
|
|
||||||
def test_validation_with_if_as_string
|
|
||||||
Topic.validates_presence_of(:title)
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_presence_of(:author_name, if: "title.to_s.match('important')")
|
|
||||||
end
|
|
||||||
|
|
||||||
t = Topic.new
|
|
||||||
assert t.invalid?, "A topic without a title should not be valid"
|
|
||||||
assert_empty t.errors[:author_name], "A topic without an 'important' title should not require an author"
|
|
||||||
|
|
||||||
t.title = "Just a title"
|
|
||||||
assert t.valid?, "A topic with a basic title should be valid"
|
|
||||||
|
|
||||||
t.title = "A very important title"
|
|
||||||
assert t.invalid?, "A topic with an important title, but without an author, should not be valid"
|
|
||||||
assert t.errors[:author_name].any?, "A topic with an 'important' title should require an author"
|
|
||||||
|
|
||||||
t.author_name = "Hubert J. Farnsworth"
|
|
||||||
assert t.valid?, "A topic with an important title and author should be valid"
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|
|
@ -70,51 +70,15 @@ class ValidatesWithTest < ActiveModel::TestCase
|
||||||
assert_includes topic.errors[:base], OTHER_ERROR_MESSAGE
|
assert_includes topic.errors[:base], OTHER_ERROR_MESSAGE
|
||||||
end
|
end
|
||||||
|
|
||||||
test "with if statements that return false" do
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_with(ValidatorThatAddsErrors, if: "1 == 2")
|
|
||||||
end
|
|
||||||
topic = Topic.new
|
|
||||||
assert topic.valid?
|
|
||||||
end
|
|
||||||
|
|
||||||
test "with if statements that return true" do
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_with(ValidatorThatAddsErrors, if: "1 == 1")
|
|
||||||
end
|
|
||||||
topic = Topic.new
|
|
||||||
assert topic.invalid?
|
|
||||||
assert_includes topic.errors[:base], ERROR_MESSAGE
|
|
||||||
end
|
|
||||||
|
|
||||||
test "with unless statements that return true" do
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_with(ValidatorThatAddsErrors, unless: "1 == 1")
|
|
||||||
end
|
|
||||||
topic = Topic.new
|
|
||||||
assert topic.valid?
|
|
||||||
end
|
|
||||||
|
|
||||||
test "with unless statements that returns false" do
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
Topic.validates_with(ValidatorThatAddsErrors, unless: "1 == 2")
|
|
||||||
end
|
|
||||||
topic = Topic.new
|
|
||||||
assert topic.invalid?
|
|
||||||
assert_includes topic.errors[:base], ERROR_MESSAGE
|
|
||||||
end
|
|
||||||
|
|
||||||
test "passes all configuration options to the validator class" do
|
test "passes all configuration options to the validator class" do
|
||||||
topic = Topic.new
|
topic = Topic.new
|
||||||
validator = Minitest::Mock.new
|
validator = Minitest::Mock.new
|
||||||
validator.expect(:new, validator, [{ foo: :bar, if: "1 == 1", class: Topic }])
|
validator.expect(:new, validator, [{ foo: :bar, if: :condition_is_true, class: Topic }])
|
||||||
validator.expect(:validate, nil, [topic])
|
validator.expect(:validate, nil, [topic])
|
||||||
validator.expect(:is_a?, false, [Symbol])
|
validator.expect(:is_a?, false, [Symbol])
|
||||||
validator.expect(:is_a?, false, [String])
|
validator.expect(:is_a?, false, [String])
|
||||||
|
|
||||||
ActiveSupport::Deprecation.silence do
|
Topic.validates_with(validator, if: :condition_is_true, foo: :bar)
|
||||||
Topic.validates_with(validator, if: "1 == 1", foo: :bar)
|
|
||||||
end
|
|
||||||
assert topic.valid?
|
assert topic.valid?
|
||||||
validator.verify
|
validator.verify
|
||||||
end
|
end
|
||||||
|
|
|
@ -98,9 +98,9 @@ module ActiveRecord
|
||||||
# == Types of callbacks
|
# == Types of callbacks
|
||||||
#
|
#
|
||||||
# There are four types of callbacks accepted by the callback macros: Method references (symbol), callback objects,
|
# There are four types of callbacks accepted by the callback macros: Method references (symbol), callback objects,
|
||||||
# inline methods (using a proc), and inline eval methods (using a string). Method references and callback objects
|
# inline methods (using a proc). Method references and callback objects
|
||||||
# are the recommended approaches, inline methods using a proc are sometimes appropriate (such as for
|
# are the recommended approaches, inline methods using a proc are sometimes appropriate (such as for
|
||||||
# creating mix-ins), and inline eval methods are deprecated.
|
# creating mix-ins).
|
||||||
#
|
#
|
||||||
# The method reference callbacks work by specifying a protected or private method available in the object, like this:
|
# The method reference callbacks work by specifying a protected or private method available in the object, like this:
|
||||||
#
|
#
|
||||||
|
|
|
@ -285,7 +285,7 @@ module ActiveRecord
|
||||||
fire_on = Array(options[:on])
|
fire_on = Array(options[:on])
|
||||||
assert_valid_transaction_action(fire_on)
|
assert_valid_transaction_action(fire_on)
|
||||||
options[:if] = Array(options[:if])
|
options[:if] = Array(options[:if])
|
||||||
options[:if].unshift("transaction_include_any_action?(#{fire_on})")
|
options[:if].unshift(-> { transaction_include_any_action?(fire_on) })
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
|
@ -1,3 +1,7 @@
|
||||||
|
* Remove deprecated `:if` and `:unless` string filter for callbacks.
|
||||||
|
|
||||||
|
*Rafael Mendonça França*
|
||||||
|
|
||||||
* `Hash#slice` now falls back to Ruby 2.5+'s built-in definition if defined.
|
* `Hash#slice` now falls back to Ruby 2.5+'s built-in definition if defined.
|
||||||
|
|
||||||
*Akira Matsuda*
|
*Akira Matsuda*
|
||||||
|
|
|
@ -298,8 +298,8 @@ module ActiveSupport
|
||||||
@kind = kind
|
@kind = kind
|
||||||
@filter = filter
|
@filter = filter
|
||||||
@key = compute_identifier filter
|
@key = compute_identifier filter
|
||||||
@if = Array(options[:if])
|
@if = check_conditionals(Array(options[:if]))
|
||||||
@unless = Array(options[:unless])
|
@unless = check_conditionals(Array(options[:unless]))
|
||||||
end
|
end
|
||||||
|
|
||||||
def filter; @key; end
|
def filter; @key; end
|
||||||
|
@ -323,7 +323,7 @@ module ActiveSupport
|
||||||
|
|
||||||
def duplicates?(other)
|
def duplicates?(other)
|
||||||
case @filter
|
case @filter
|
||||||
when Symbol, String
|
when Symbol
|
||||||
matches?(other.kind, other.filter)
|
matches?(other.kind, other.filter)
|
||||||
else
|
else
|
||||||
false
|
false
|
||||||
|
@ -350,9 +350,21 @@ module ActiveSupport
|
||||||
end
|
end
|
||||||
|
|
||||||
private
|
private
|
||||||
|
def check_conditionals(conditionals)
|
||||||
|
if conditionals.any? { |c| c.is_a?(String) }
|
||||||
|
raise ArgumentError, <<-MSG.squish
|
||||||
|
Passing string to be evaluated in :if and :unless conditional
|
||||||
|
options is not supported. Pass a symbol for an instance method,
|
||||||
|
or a lambda, proc or block, instead.
|
||||||
|
MSG
|
||||||
|
end
|
||||||
|
|
||||||
|
conditionals
|
||||||
|
end
|
||||||
|
|
||||||
def compute_identifier(filter)
|
def compute_identifier(filter)
|
||||||
case filter
|
case filter
|
||||||
when String, ::Proc
|
when ::Proc
|
||||||
filter.object_id
|
filter.object_id
|
||||||
else
|
else
|
||||||
filter
|
filter
|
||||||
|
@ -427,7 +439,6 @@ module ActiveSupport
|
||||||
# Filters support:
|
# Filters support:
|
||||||
#
|
#
|
||||||
# Symbols:: A method to call.
|
# Symbols:: A method to call.
|
||||||
# Strings:: Some content to evaluate.
|
|
||||||
# Procs:: A proc to call with the object.
|
# Procs:: A proc to call with the object.
|
||||||
# Objects:: An object with a <tt>before_foo</tt> method on it to call.
|
# Objects:: An object with a <tt>before_foo</tt> method on it to call.
|
||||||
#
|
#
|
||||||
|
@ -437,8 +448,6 @@ module ActiveSupport
|
||||||
case filter
|
case filter
|
||||||
when Symbol
|
when Symbol
|
||||||
new(nil, filter, [], nil)
|
new(nil, filter, [], nil)
|
||||||
when String
|
|
||||||
new(nil, :instance_exec, [:value], compile_lambda(filter))
|
|
||||||
when Conditionals::Value
|
when Conditionals::Value
|
||||||
new(filter, :call, [:target, :value], nil)
|
new(filter, :call, [:target, :value], nil)
|
||||||
when ::Proc
|
when ::Proc
|
||||||
|
@ -455,10 +464,6 @@ module ActiveSupport
|
||||||
new(filter, method_to_call, [:target], nil)
|
new(filter, method_to_call, [:target], nil)
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
def self.compile_lambda(filter)
|
|
||||||
eval("lambda { |value| #{filter} }")
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
|
|
||||||
# Execute before and after filters in a sequence instead of
|
# Execute before and after filters in a sequence instead of
|
||||||
|
@ -651,26 +656,17 @@ module ActiveSupport
|
||||||
#
|
#
|
||||||
# ===== Options
|
# ===== Options
|
||||||
#
|
#
|
||||||
# * <tt>:if</tt> - A symbol, a string (deprecated) or an array of symbols,
|
# * <tt>:if</tt> - A symbol or an array of symbols, each naming an instance
|
||||||
# each naming an instance method or a proc; the callback will be called
|
# method or a proc; the callback will be called only when they all return
|
||||||
# only when they all return a true value.
|
# a true value.
|
||||||
# * <tt>:unless</tt> - A symbol, a string (deprecated) or an array of symbols,
|
# * <tt>:unless</tt> - A symbol or an array of symbols, each naming an
|
||||||
# each naming an instance method or a proc; the callback will be called
|
# instance method or a proc; the callback will be called only when they
|
||||||
# only when they all return a false value.
|
# all return a false value.
|
||||||
# * <tt>:prepend</tt> - If +true+, the callback will be prepended to the
|
# * <tt>:prepend</tt> - If +true+, the callback will be prepended to the
|
||||||
# existing chain rather than appended.
|
# existing chain rather than appended.
|
||||||
def set_callback(name, *filter_list, &block)
|
def set_callback(name, *filter_list, &block)
|
||||||
type, filters, options = normalize_callback_params(filter_list, block)
|
type, filters, options = normalize_callback_params(filter_list, block)
|
||||||
|
|
||||||
if options[:if].is_a?(String) || options[:unless].is_a?(String)
|
|
||||||
ActiveSupport::Deprecation.warn(<<-MSG.squish)
|
|
||||||
Passing string to be evaluated in :if and :unless conditional
|
|
||||||
options is deprecated and will be removed in Rails 5.2 without
|
|
||||||
replacement. Pass a symbol for an instance method, or a lambda,
|
|
||||||
proc or block, instead.
|
|
||||||
MSG
|
|
||||||
end
|
|
||||||
|
|
||||||
self_chain = get_callbacks name
|
self_chain = get_callbacks name
|
||||||
mapped = filters.map do |filter|
|
mapped = filters.map do |filter|
|
||||||
Callback.build(self_chain, filter, type, options)
|
Callback.build(self_chain, filter, type, options)
|
||||||
|
@ -695,13 +691,6 @@ module ActiveSupport
|
||||||
def skip_callback(name, *filter_list, &block)
|
def skip_callback(name, *filter_list, &block)
|
||||||
type, filters, options = normalize_callback_params(filter_list, block)
|
type, filters, options = normalize_callback_params(filter_list, block)
|
||||||
|
|
||||||
if options[:if].is_a?(String) || options[:unless].is_a?(String)
|
|
||||||
ActiveSupport::Deprecation.warn(<<-MSG.squish)
|
|
||||||
Passing string to :if and :unless conditional options is deprecated
|
|
||||||
and will be removed in Rails 5.2 without replacement.
|
|
||||||
MSG
|
|
||||||
end
|
|
||||||
|
|
||||||
options[:raise] = true unless options.key?(:raise)
|
options[:raise] = true unless options.key?(:raise)
|
||||||
|
|
||||||
__update_callbacks(name) do |target, chain|
|
__update_callbacks(name) do |target, chain|
|
||||||
|
|
|
@ -193,13 +193,6 @@ module CallbacksTest
|
||||||
before_save Proc.new { |r| r.history << "b00m" }, if: :no
|
before_save Proc.new { |r| r.history << "b00m" }, if: :no
|
||||||
before_save Proc.new { |r| r.history << [:before_save, :symbol] }, unless: :no
|
before_save Proc.new { |r| r.history << [:before_save, :symbol] }, unless: :no
|
||||||
before_save Proc.new { |r| r.history << "b00m" }, unless: :yes
|
before_save Proc.new { |r| r.history << "b00m" }, unless: :yes
|
||||||
# string
|
|
||||||
ActiveSupport::Deprecation.silence do
|
|
||||||
before_save Proc.new { |r| r.history << [:before_save, :string] }, if: "yes"
|
|
||||||
before_save Proc.new { |r| r.history << "b00m" }, if: "no"
|
|
||||||
before_save Proc.new { |r| r.history << [:before_save, :string] }, unless: "no"
|
|
||||||
before_save Proc.new { |r| r.history << "b00m" }, unless: "yes"
|
|
||||||
end
|
|
||||||
# Combined if and unless
|
# Combined if and unless
|
||||||
before_save Proc.new { |r| r.history << [:before_save, :combined_symbol] }, if: :yes, unless: :no
|
before_save Proc.new { |r| r.history << [:before_save, :combined_symbol] }, if: :yes, unless: :no
|
||||||
before_save Proc.new { |r| r.history << "b00m" }, if: :yes, unless: :yes
|
before_save Proc.new { |r| r.history << "b00m" }, if: :yes, unless: :yes
|
||||||
|
@ -592,8 +585,6 @@ module CallbacksTest
|
||||||
[:before_save, :proc],
|
[:before_save, :proc],
|
||||||
[:before_save, :symbol],
|
[:before_save, :symbol],
|
||||||
[:before_save, :symbol],
|
[:before_save, :symbol],
|
||||||
[:before_save, :string],
|
|
||||||
[:before_save, :string],
|
|
||||||
[:before_save, :combined_symbol],
|
[:before_save, :combined_symbol],
|
||||||
], person.history
|
], person.history
|
||||||
end
|
end
|
||||||
|
@ -1182,14 +1173,15 @@ module CallbacksTest
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
class DeprecatedWarningTest < ActiveSupport::TestCase
|
class NotSupportedStringConditionalTest < ActiveSupport::TestCase
|
||||||
def test_deprecate_string_conditional_options
|
def test_string_conditional_options
|
||||||
klass = Class.new(Record)
|
klass = Class.new(Record)
|
||||||
|
|
||||||
assert_deprecated { klass.before_save :tweedle, if: "true" }
|
assert_raises(ArgumentError) { klass.before_save :tweedle, if: ["true"] }
|
||||||
assert_deprecated { klass.after_save :tweedle, unless: "false" }
|
assert_raises(ArgumentError) { klass.before_save :tweedle, if: "true" }
|
||||||
assert_deprecated { klass.skip_callback :save, :before, :tweedle, if: "true" }
|
assert_raises(ArgumentError) { klass.after_save :tweedle, unless: "false" }
|
||||||
assert_deprecated { klass.skip_callback :save, :after, :tweedle, unless: "false" }
|
assert_raises(ArgumentError) { klass.skip_callback :save, :before, :tweedle, if: "true" }
|
||||||
|
assert_raises(ArgumentError) { klass.skip_callback :save, :after, :tweedle, unless: "false" }
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|
Loading…
Reference in New Issue