allow_nil should work for casted value in NumericalityValidator
#40765
Conversation
It is a regression for 4cc438a. `NumericalityValidator` basically takes the value before typecasting, but `allow_nil` should work for the typecasted value for the compatibility. Fixes rails#40750.
| @@ -167,7 +168,9 @@ def check_validity! | |||
|
|
|||
| private | |||
| def read_attribute_for_validation(record, attr_name) | |||
| record.read_attribute_for_validation(attr_name) | |||
| value = record.read_attribute_for_validation(attr_name) | |||
| throw(:allowed) if (value.nil? && options[:allow_nil]) || (value.blank? && options[:allow_blank]) | |||
There was a problem hiding this comment.
Moving the checks here actually make the API for custom validations worse. Users need to know that they need to deal with those options inside read_attribute_for_validation while before they didn't. I believe we should continue doing this check inside validate to keep users not needing to worry about checking for those options.
Can we split the job of read_attribute_for_validation in two? One which does record.read_attribute_for_validation(attr_name) and we use for the options checks and the other than calculate value that we will pass to validate_each? This will also avoid the throw/abort.
There was a problem hiding this comment.
I agree that it is not ideal, the intent of the implementation was to avoid extra value argument for read_attribute_for_validation.
I've given up that finally, splited the job and add value argument for read_attribute_for_validation in 720a60e (perhaps read_attribute_for_validation no longer appropriate name now?).
There was a problem hiding this comment.
Agree the name is not matching the behaviour anymore. I'd call it prepare_value_for_validation.
…d_value `allow_nil` should work for casted value in `NumericalityValidator`
It is a regression for 4cc438a.
NumericalityValidatorbasically takes the value before typecasting,but
allow_nilshould work for the typecasted value for thecompatibility.
Fixes #40750.