Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

validates_length with minimum 0 and allow_nil: false accept nil #7222

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
Contributor

acapilleri commented Aug 1, 2012

Given a class that include ActiveModel::Validations with a validation like the following:
validates_length_of(:title, :minimum => 0, :allow_nil => false).
This makes incosistent the validation because it not raise error when
title is nil.It should accepts only the empty string
With this patch title is nil is handled and the validation accepts only the empty string ""

validates_length with minimum 0 and allow_nil: false accept nil
Given a class that include ActiveModel::Validations with a validation like the following:
validates_length_of(:title, :minimum => 0, :allow_nil => false).
This makes incosistent the validation because it not raise error when
title is *nil*.It should accepts only the empty string
With this patch title is *nil* is handled and the validation accepts only the empty string *""*
It should fix #7180

@josevalim josevalim commented on the diff Aug 1, 2012

activemodel/lib/active_model/validations/length.rb
@@ -53,6 +55,11 @@ def validate_each(record, attribute, value)
private
+ def min_zero_val_nil_allow_nil_false?(options, value)
@josevalim

josevalim Aug 1, 2012

Contributor

Sorry, this method name doesn't make any sense.

@josevalim josevalim commented on the diff Aug 1, 2012

activemodel/lib/active_model/validations/length.rb
@@ -53,6 +55,11 @@ def validate_each(record, attribute, value)
private
+ def min_zero_val_nil_allow_nil_false?(options, value)
+ empty_string_is_allowed ||= options[:minimum] == 0 && value.nil? && options[:allow_nil] == false
@josevalim

josevalim Aug 1, 2012

Contributor

We don't need to define a variable here. Nor we need to explicitly check if :allow_nil is false (the default is for it to be false/nil).

@josevalim josevalim commented on the diff Aug 1, 2012

activemodel/lib/active_model/validations/length.rb
- errors_options = options.except(*RESERVED_OPTIONS)
+ errors_options ||= options.except(*RESERVED_OPTIONS)
@josevalim

josevalim Aug 1, 2012

Contributor

Why this change?

@acapilleri

acapilleri Aug 2, 2012

Contributor

because it is the same during the main cycle, so it could be cached

Contributor

josevalim commented Aug 1, 2012

I have added some comments but on second thoughts I don't think we should merge this patch. It is no secret that length is going to convert the value to a string if it isn't one. Writing validates_length_of(:title, :minimum => 0, :allow_nil => false) seems to be just a confusing way to write validates_presence_of(:title).

Contributor

acapilleri commented Aug 1, 2012

Yes I agree with your about the confusing way, #7180, but if this pull request will not merged, it could be better that :minimummust be >= 1 rasing an exception?

Contributor

josevalim commented Aug 1, 2012

Why not use validates_presence_of?

@josevalim josevalim closed this Aug 1, 2012

Contributor

acapilleri commented Aug 1, 2012

yes but

Topic.validates_presence_of(:title)

 t = Topic.new(:title=>"")
 assert t.valid? 

fails

Contributor

acapilleri commented Aug 2, 2012

If could be helpful to someone,the test above do not fails with the following validation

validates_presence_of :title , :unless => Proc.new { |a| a.title == "" }

🏄 not fails :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment