Length validation handles correctly nil. Fix #7180 #7282

Merged
merged 1 commit into from Nov 26, 2012

Projects

None yet

4 participants

@xHire

When nil or empty string are not allowed, they are not valid.

@rafaelfranca rafaelfranca commented on an outdated diff Aug 7, 2012
activemodel/lib/active_model/validations/length.rb
@@ -40,7 +44,10 @@ def validate_each(record, attribute, value)
CHECKS.each do |key, validity_check|
next unless check_value = options[key]
- next if value_length.send(validity_check, check_value)
+
+ if !value.nil? || key == :maximum && options[:allow_nil].nil? && options[:allow_blank].nil?
@rafaelfranca
rafaelfranca Aug 7, 2012

Do you think that we can extract these conditionals to a method with a good name? This will make easier to understand.

@frodsan

@xHire Hey! Any news on this? :)

@xHire

@frodsan Thank you for reminding me of it! I was a bit busy and then I forgot to finish this.

What about this change? https://gist.github.com/3985883 Would be such naming fine?

@acapilleri ad overwriting options: I'm actually not overwriting it but adding one more. :c) I understand that you probably meant that I should not anyhow alter the options hash, however how else would you enforce the condition of not being empty as the validation works on processing given options? So, I understand your objection, but I still don't have any better idea how it could be done.

@schneems
Ruby on Rails member

@xhire I would extract the majority of that logic into a method called skip_nil_check? and then I would call

if !value.nil? || skip_nil_check?(value, key)

I believe you don't have to pass the options variable as it is an attr_reader on the instance. Does that work for you?

@xHire

@schneems Thank you for your suggestions! Your if is really clever -- short, clear and makes sense. I didn't realise the fact about options, but you are right.

@schneems
Ruby on Rails member

@rafaelfranca can you give this another look?

@rafaelfranca
Ruby on Rails member

Seems good. We need a CHANGELOG entry

@rafaelfranca
Ruby on Rails member

Could you also squash your commits?

Ping me when done please.

@xHire xHire Length validation handles correctly nil. Fix #7180
When nil or empty string are not allowed, they are not valid.
ea76e9a
@xHire

@rafaelfranca Ping. It's done. (I didn't know I can update pull request this way! That's pretty cool.) CHANGELOG entry included.

@schneems
Ruby on Rails member

👍 i'm happy with this

@rafaelfranca rafaelfranca merged commit ea76e9a into rails:master Nov 26, 2012
@rafaelfranca
Ruby on Rails member

@xHire thank you so much

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