Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Tests and fix for validates_presence_of :allow_nil, :allow_blank #8622

Closed
wants to merge 1 commit into from

2 participants

@ColinDKelley

Tests and fix for this issue:

#8621

@rafaelfranca rafaelfranca was assigned
@rafaelfranca rafaelfranca referenced this pull request from a commit in rafaelfranca/omg-rails
@rafaelfranca rafaelfranca Revert the change at ActiveModel::Errors#add_on_blank and fix in the
right place.

The EachValidator#validate already handle :allow_blank and :allow_nil,
correctly.

Closes #8622.

Fix #8621.
1e00ab3
@rafaelfranca

@ColinDKelley thank you so much. As you can see above I applied your commit in my branch and changed the implementation to something I think is better.

I'm waiting feedback from @josevalim and @carlosantoniodasilva to see if I'm not missing something.

@rafaelfranca rafaelfranca closed this pull request from a commit
@rafaelfranca rafaelfranca Revert the change at ActiveModel::Errors#add_on_blank and fix in the
right place.

The EachValidator#validate already handle :allow_blank and :allow_nil,
correctly.

Closes #8622.

Fix #8621.
78fd14c
@ColinDKelley

Thanks Rafael! I didn't realize :allow_nil was handled at the lower level; I agree that your version is cleaner.

I noticed you changed the tests to require 1.9 for its new hash syntax. Is there a reason to not leave them runnable in 1.8? This changes is going to Rails 3 which I am pretty sure still supports 1.8. I would certainly appreciate if it does--my company hasn't finished the changeover to 1.9 yet! :)

Also a co-worker pointed out that the Rails Guides currently document this inconsistency as a TIP: http://guides.rubyonrails.org/active_record_validations_callbacks.html (search for "ignore").

I've made a corresponding fix to remove that TIP as it is no longer applicable: rails/docrails#122 .

@rafaelfranca

@ColinDKelley the commit with ruby 1.9 hash syntax is only for master. I applied another commit on 3-2-stable using ruby 1.8 syntax.

About the TIP, well, this means it is the desirable behavior although I think it is wrong. This also means I can't apply this change in 3-2-stable branch since it will change behavior in a stable release.

@ColinDKelley

Ok, thanks for the clarification. I think there's a good case to make that it wasn't ever desirable since a) there was no other way to allow_nil on that validator and b) it was not documented in the rdocs. It seems unlikely to destabilize an app since these options were simply ignored before, hence very unlikely to be used.

In any case, the fact that this change restores orthogonal behavior by removing a special case that no longer needs to be documented in the Rails Guide is proof that it's valuable! :)

@rafaelfranca

Some core team members raised a question why an user would need to allow nil values in a presence validation. What is your use case?

@ColinDKelley

We use this pattern a handful of times in our application for unique, optional names. In SQL NULL values are exempt from unique index enforcement, but empty strings are not. So NULL/nil is the appropriate "unset" value. We use validates_presence_of ... :allow_nil => true so that Rails catches anyone trying to set the name to an empty or blank string.

But even without that example, I think the change makes sense because it achieves an orthogonal interface that follows the Principle of Least Surprise. Simply the benefit of removing an unnecessary special case from the Guide would convince me! (I argue that it was unnecessary because ignoring these options in the case of this one validator did not benefit anyone.)

@rafaelfranca

ok. @ColinDKelley thank you so much for the pull request.

@sgerrand sgerrand referenced this pull request from a commit in sgerrand/rails
@ColinDKelley ColinDKelley removed TIP: :allow_nil/:allow_blank is ignored by the presence valid…
…ator

These tips were documenting an inconsistency issue rails#8621. That issue is resolved by rails#8622.
52691c3
@wkj wkj referenced this pull request from a commit
@tenderlove tenderlove Merge branch 'master' into jobs
* master:
  fix time typcasting on group counts in PG
  mysql does not return alias names, so fall back
  Test that assert_not returns true. Use assert_raises instead of doing begin/rescue/else.
  fix PG typecasting errors
  small refactoring, added blob_or_text_colum? in AbstractMysqlAdapter
  work off FIXME comments in AR rename_column_test.rb
  Revert "Install binstubs by default"
  Revert "Detect rbenv and update the shebang"
  Revert "Add rake test description"
  Revert "Add rake default to description"
  Revert "reminder to run bundle after setting up rails-dev-box"
  Remove 'assigned but unused variable' warning
  Introduce assert_not to replace 'assert !foo'
  Fixed couple of typos
  removed TIP: :allow_nil/:allow_blank is ignored by the presence validator These tips were documenting an inconsistency issue #8621. That issue is resolved by #8622.
  reminder to run bundle after setting up rails-dev-box
  Add rake test description
  Add rake default to description
d7cb0ae
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
7 activemodel/lib/active_model/errors.rb
@@ -250,9 +250,14 @@ def add_on_empty(attributes, options = {})
# Will add an error message to each of the attributes in +attributes+ that is blank (using Object#blank?).
def add_on_blank(attributes, options = {})
+ return if options[:allow_blank]
[attributes].flatten.each do |attribute|
value = @base.send(:read_attribute_for_validation, attribute)
- add(attribute, :blank, options) if value.blank?
+ if value.nil?
+ add(attribute, :blank, options) unless options[:allow_nil]
+ elsif value.blank?
+ add(attribute, :blank, options)
+ end
end
end
View
34 activemodel/test/cases/validations/presence_validation_test.rb
@@ -70,4 +70,38 @@ def test_validates_presence_of_for_ruby_class_with_custom_reader
p[:karma] = "Cold"
assert p.valid?
end
+
+ def test_allow_nil
+ Topic.validates_presence_of(:title, :allow_nil => true)
+
+ t = Topic.new(:title => "something")
+ assert t.valid?, t.errors.full_messages
+
+ t.title = ""
+ assert t.invalid?
+ assert_equal ["can't be blank"], t.errors[:title]
+
+ t.title = " "
+ assert t.invalid?, t.errors.full_messages
+ assert_equal ["can't be blank"], t.errors[:title]
+
+ t.title = nil
+ assert t.valid?, t.errors.full_messages
+ end
+
+ def test_allow_blank
+ Topic.validates_presence_of(:title, :allow_blank => true)
+
+ t = Topic.new(:title => "something")
+ assert t.valid?, t.errors.full_messages
+
+ t.title = ""
+ assert t.valid?, t.errors.full_messages
+
+ t.title = " "
+ assert t.valid?, t.errors.full_messages
+
+ t.title = nil
+ assert t.valid?, t.errors.full_messages
+ end
end
Something went wrong with that request. Please try again.