Don't enable validations when passing false hash values to ActiveModel.validates #6516

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@purcell
Contributor
purcell commented May 28, 2012

Passing a falsey option value for a validator currently causes that validator to
be enabled, just like "true":

ActiveModel.validates :foo, :presence => false

This is rather counterintuitive, and makes it inconvenient to wrap validates in
methods which may conditionally enable different validators.

As an example, one is currently forced to write:

  def has_slug(source_field, options={:unique => true})
    slugger = Proc.new { |r| r[:slug] = self.class.sluggify(r[source_field]) if r[:slug].blank? }
    before_validation slugger
    validations = { :presence => true, :slug => true }
    if options[:unique]
      validations[:uniqueness] = true
    end
    validates :slug, validations
  end

because the following reasonable-looking alternative fails to work as expected:

  def has_slug(source_field, options={:unique => true})
    slugger = Proc.new { |r| r[:slug] = self.class.sluggify(r[source_field]) if r[:slug].blank? }
    before_validation slugger
    validates :slug, :presence => true, :slug => true, :uniqueness => options[:unique]
  end

(This commit includes a test, and all activemodel and activerecord tests pass as before.)

@purcell purcell Don't enable validations when passing false hash values to ActiveMode…
…l.validates

Passing a falsey option value for a validator currently causes that validator to
be enabled, just like "true":

    ActiveModel.validates :foo, :presence => false

This is rather counterintuitive, and makes it inconvenient to wrap `validates` in
methods which may conditionally enable different validators.

As an example, one is currently forced to write:

      def has_slug(source_field, options={:unique => true})
        slugger = Proc.new { |r| r[:slug] = self.class.sluggify(r[source_field]) if r[:slug].blank? }
        before_validation slugger
        validations = { :presence => true, :slug => true }
        if options[:unique]
          validations[:uniqueness] = true
        end
        validates :slug, validations
      end

because the following reasonable-looking alternative fails to work as expected:

      def has_slug(source_field, options={:unique => true})
        slugger = Proc.new { |r| r[:slug] = self.class.sluggify(r[source_field]) if r[:slug].blank? }
        before_validation slugger
        validates :slug, :presence => true, :slug => true, :uniqueness => options[:unique]
      end

(This commit includes a test, and all activemodel and activerecord tests pass as before.)
7a7f007
@purcell
Contributor
purcell commented May 28, 2012

P.S. This pull request is against 3-2-stable; please advise if this causes any difficulties.

@josevalim
Member

Can you please send this for master? (as a rule, always send pull request for master, if there is a need, we will backport) Thanks!

@josevalim josevalim closed this May 28, 2012
@drogus
Member
drogus commented May 28, 2012

@purcell can you please open pull request for master? For me it's fine to merge it to 3-2-stable, but I would like to wait for comments from others.

Could you also add a CHANGELOG entry?

@drogus
Member
drogus commented May 28, 2012

Of course @josevalim was faster ;)

@josevalim
Member

TWSS :(

@purcell
Contributor
purcell commented May 28, 2012

Sorry - I had to create a new pull request for this because the base branch had changed. The new one (#6517) has a CHANGELOG entry as requested.

Cheers!

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