Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Jamie Hill August 16th, 2009 @ 01:05 AM

  Apologies, that last line should have read:

  ActiveModel::Validations.register_validator :presence, RequiredValidator
  

• You flagged this item as spam.
  

  darkliquid August 19th, 2009 @ 12:17 PM

  Your patch seems to be missing your 'validates' class method, so your example doesn't actually work.

  A rough idea of the top of my head is that you meant to include a method something like this in ActiveModel::Validations::ClassMethods:

  def validates(*args)
  validators = args.extract_options!

  args.each do |attribute|

  validators.each do |name, configuration|
    klass = ActiveModel::Validations.validator_for(name)
    send(validation_method(configuration[:on]), configuration) do |record|
      klass.new(record, configuration.merge(:field => attribute)).validate
    end
  end
  

  end end

  Without this method or any modifications to existing validation macros, registering classes with friendly names basically does nothing.

• You flagged this item as spam.
  

  Jamie Hill August 19th, 2009 @ 12:30 PM

  Ahh, good spot. It looks like I didn't add the 2 most important files which also include the documentation.

  Attached is the ACTUAL patch.

• You flagged this item as spam.
  

  Jamie Hill August 19th, 2009 @ 12:51 PM

  I've just noticed that my email example validator was wrong too, should have been.

  class EmailValidator < ActiveRecord::Validator
    def validate
      field = options[:attr]
      record.errors[field] << "is not valid" unless record.send(field) =~ /^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$/i
    end
  end
  
  # Register the custom validator in an initializer file.
  ActiveModel::Validations.register_validator :email, EmailValidator
  

  Sorry guys, must have been having an off day.

• You flagged this item as spam.
  

  Jamie Hill August 19th, 2009 @ 12:55 PM

  Can I get thoughts on publicising the new read_attribute_for_validation method as then this could be used in custom validators meaning they would be super-flexible.

• 

  josh August 19th, 2009 @ 04:06 PM

  • State changed from “new” to “open”
  • Milestone cleared.

  "registration" hooks aren't necessary in Ruby. We have const_get.

• You flagged this item as spam.
  

  darkliquid August 19th, 2009 @ 04:16 PM

  Rather than using the registration hooks, I take you are proposing a something that takes the validation names and just constantizes them into validator classes, e.g :email becomes EmailValidator automagically?

• You flagged this item as spam.
  

  Jamie Hill August 19th, 2009 @ 04:20 PM

  Josh, are you suggesting I just look for a class in the global namespace?

  What are your thoughts on looking for any classes that are called "#{the_symbol.to_s.camelize}Validator" that inherit from ActiveModel::Validator?

  Also I'm thinking that ActiveRecord::Validator should be moved to ActiveModel::Validator so that this functionality is available outside of ActiveRecord.

• You flagged this item as spam.
  

  Jamie Hill August 19th, 2009 @ 10:56 PM

  Attached is an updated patch to use convention over configuration as suggested by Josh. The following is now possible:

  class EmailValidator < ActiveModel::Validator
    def validate
      field = options[:attr]
      record.errors[field] << "is not valid" unless
        record.read_attribute_for_validation(field) =~ /^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$/i
    end
  end
  
  class Person < ActiveModel::Base
    attr_accessor :name, :email
  
    validates :name, :presence => true, :uniqueness => true, :length => { :maximum => 100 }
    validates :email, :presence => true, :email => true
  end
  
  # Validator classes my also exist within the class being validated
  # allowing custom modules of validators to be included as needed e.g.
  module MyValidators
    class TitleValidator < ActiveModel::Validator
      def validate
        field = options[:attr]
        record.errors[field] << "must start with 'the'" unless
          record.read_attribute_for_validation(field) =~ /^the/i
      end
    end
  end
  
  class Person < ActiveModel::Base
    include MyValidators
  
    validates :title => true
  end
  

  I have also made a change to rectify the bug in this ticket: https://rails.lighthouseapp.com/projects/8994/tickets/3077-activere... as logged by darkliquid.

  One other modification I have made is to make read_attribute_for_validation public so that it may be used in custom validators.

  Ideally I would change validates_with and the Validator class so that it is not necessary to fetch the value for the attribute and instead pass the value in as an argument to the validate method in the Validator class but I didn't want to change the validates_with API. As validates_with is new to edge, what is the policy on changing it's API?

• You flagged this item as spam.
  

  Jamie Hill August 19th, 2009 @ 11:00 PM

  Just added a change to the documentation:

  class Person < ActiveModel::Base
    include MyValidators
   
    validates :name, :title => true
  end
  

• You flagged this item as spam.
  

  Elliot Winkler August 21st, 2009 @ 03:06 PM

  I know the idea's pretty much accepted already but I just want to say, big +1 from me. The ability to combine validations into one line is worth it alone, and automatically picking up on Validators makes Validators even more awesome than they were. Now if #750 and #401 could be resolved that would solve all the wishes I've had for validations.

• You flagged this item as spam.
  

  darkliquid August 21st, 2009 @ 03:43 PM

  +1 from me as well.

  There is one possible minor issue which I doubt would actually cause any problems. Since a hash is being used to specify validations, those validations will be defined in a random order in ruby 1.8. It's not an issue at all in ruby 1.9 as hashes are ordered I believe. You could get around the ordering issue in 1.8 (from a users perspective) by just calling validates with one key-value pair at a time if ordering is important to you.

  Elliot, regarding #750, some way to do full reflection on validations like you can on associations would be even more awesome as you could show all requirements in the view or do stuff like generate client-side validation based on the reflected server-side ones for example.

• You flagged this item as spam.
  

  Jamie Hill August 24th, 2009 @ 05:14 PM

  I was wondering if anyone has any more feedback on this as I'd like to get it accepted as soon as poss? Cheers.

• 

  josh August 31st, 2009 @ 07:48 PM

  • Assigned user cleared.

  A bunch guys started discussing completely rewriting the internal validation api to be more like validatable. I'd bring up in a Rails core post. Yehuda, Bryan H, and José V are the guys interested.

  (As for me, I think the current api is fine, so I'm removing myself)

• You flagged this item as spam.
  

  Jamie Hill September 2nd, 2009 @ 12:56 PM

  Josh,

  This implementation currently works without any changes to the current API, I was stating however that ideally the API for validator classes would be changed slightly to make things easier. Could this patch be applied as-is in addition to the discussions for a new API.

  Thanks,

  Jamie

• 

  José Valim September 4th, 2009 @ 01:27 PM

  Sorry for the delay to reply. @Jamie, I want to do a quite simple change: instead of giving a proc to validate, we could give the own class to validate:

    def validates_with(*args)
      configuration = args.extract_options!
  
      send(validation_method(configuration[:on]), configuration) do |record|
        args.each do |klass|
          klass.new(record, configuration.except(:on, :if, :unless)).validate
        end
      end
    end
  

  Could be changed to:

    def validates_with(*args)
      configuration = args.extract_options!
  
      args.each do |klass|
        send(validation_method(configuration[:on], klass.new(configuration), configuration)
      end
    end
  

  Why? If we do this, we can access all the classes that were given to validates_with. So if we eventually change all Rails validations to use validates_with (creating something like PresenceValidator), we would be able to retrieve those classes and have validations reflection (to generate dynamically javascript, for example).

  What would need to be changed? We just need to change Validator to receive the record as argument in the validate method, which in my opinion is also a better design (the validator does not need to be attached to any object when created).

  I already had a patch for this and it should get merged soon. After that we can continue working on sexy validations ;).

  And one other thing is the DSL. Which one do you prefer?

    # Hashes
    validates :name, :presence => true, :uniqueness => true, :length => { :maximum => 100 }
  
    # Blocks
    validations_for :name do |n|
      n.presence
      n.uniqueness
      n.length :maximum => 100
    end
  

  Both are great for me, just want to be sure we discuss it. :)

• You flagged this item as spam.
  

  Jamie Hill September 4th, 2009 @ 03:51 PM

  Hi José,

  Thanks for the reply.

  I am 100% agreed on this. With regards the hash vs. block I personally prefer the hash syntax as it is more concise and requires less DSL magic.

  Passing in the record to the validator definitely works. I have included a link below to iq-validator that we use at SonicIQ for validating non-activerecord models. Our validator classes have a valid? method that accept the value to validate. I do prefer your solution of giving the record to validators as validators such as a ConfirmValidator then have access to other fields.

  Another example of using the standalone validation implementation is in our iq-form gem which takes a different approach to forms so that you can create them in an object orientated manor similar to Django. This could act as a good testbed for the flexibility of Rails validation.

  Please note. I ideally want to remove the need for iq-validation, hence my patches to expand the existing functionality of validations in Rails. Also note that these gems are not at a release state, I have just put them up for reference.

  http://github.com/iq/validation/tree/master
  http://github.com/iq/form/tree/master

  Thanks again,

  Jamie

• 

  José Valim September 4th, 2009 @ 04:00 PM

  @Jamie,

  Exactly. As I discussed with Jeremy Kemper, besides the Validator, we should implement at least an EachValidator that exposes a less raw API. We could do this:

  class PresenceValidator < EachValidator
    def for_each(record, attribute, value)
      # logic
    end
  end
  

  So we wouldn't have any problem changing all rails validations to use classes. As soon as the patch gets applied, I will ping you again. :)

• You flagged this item as spam.
  

  Jeroen van Dijk September 7th, 2009 @ 04:17 PM

  +1 for this new functionality.

  I'm not sure if the following is directly related, but it might be. When you want do some custom validation which would not be generic enough for a validation class, it would be nice if the validation syntax could be shorter. See below:

  
  validates :age, :message => :something_very_small_but_custom do |record|
     record.age < 18
  end
  

  This would be equivalent to what you would could do now:

     validate_each :age do |record, attr, value|
        record.errors(attr, :something_very_small_but_custom) if age < 18
     end
  

  As stated before, not sure if this is related, but I can image that the validate method will accept a hash with options and optionally a block with the custom validation.

• You flagged this item as spam.
  

  Jeroen van Dijk September 7th, 2009 @ 04:22 PM

  Sorry I made some typos.

  The second code example should be:

  
     validates_each :age do |record, attr, value|
        record.errors(attr, :something_very_small_but_custom) if attr < 18
     end
  

• 

  josh October 7th, 2009 @ 05:23 AM

  • Assigned user set to “José Valim”
• 

  josh October 9th, 2009 @ 06:18 PM

  • Milestone set to “2.x”
• 

  josh October 9th, 2009 @ 06:18 PM

  • Milestone cleared.

• You flagged this item as spam.
  

  Jamie Hill October 10th, 2009 @ 08:23 AM

  @José Valim, has the patch you mentioned, been applied yet, if so I will update the patch. Jeroens suggestion for a short block syntax can also be included.

• 

  José Valim October 10th, 2009 @ 12:51 PM

  @Jamie, no, no yet. We've been working one other features that have higher priority. I'm not sure that the patch will be able to make it to Rails 3.

• You flagged this item as spam.
  

  Jamie Hill October 11th, 2009 @ 01:56 PM

  @José, in that case could the patch be applied as-is as it works with the existing functionality?

• You flagged this item as spam.
  

  Jamie Hill November 15th, 2009 @ 11:26 AM

  Sorry, to bug. Can this be applied?

• 

  José Valim January 2nd, 2010 @ 11:19 PM

  Ok, all validations are now Validators in core. There is just one concern left: I don't like the fact that we have two entry points: one as @_registered_validators and other as validates_presence_of. I think the lookup should be:

  1) validates#{method}to
  2) validates_#{method}
  3) raise no validation available

  I would also ask you to run this through the rails core list to see people and mainly rails core opinion on it.

  And sorry for the time taken to reply.

• You flagged this item as spam.
  

  Jamie Hill January 3rd, 2010 @ 04:01 AM

  Great news, thanks for your help on this.

  I am attaching a new patch that address your concerns by working by convention as suggested by Josh. I have also publicised the read_attribute_for_validation method as it may be needed when writing custom validators.

  Can I suggest that we merge ActiveModel::Validator and ActiveModel::EachValidator as there is no real use for what is currently ActiveModel::Validator (please correct me if I'm wrong). This way usage would be as follows:

  class EmailValidator < ActiveModel::Validator
    def validate_each(record, attribute, value)
      record.errors[attribute] << "is not an email" unless value =~ /^([^@\s]+)@((?:[-a-z0-9]+\.)+[a-z]{2,})$/i
    end
  end
  
  class MyClass
    include ActiveModel::Validations
    attr_accessor :from
    validates :from, :email => true
  end
  

  I am happy to update the patch to reflect this if you are in agreement.

• 

  José Valim January 3rd, 2010 @ 08:43 AM

  Validator it's an abstract and will be handy if someone needs, so I prefer if we keep it.

  About your patch, I would prefer if the common entry point was the validates#{method}of instead of the validator, if you check validates_associated_of or validates_confirmation_of you will know why. Unless we move that logic inside validators as well.

  +1 on publicizing read_attributes_for_validation.

• You flagged this item as spam.
  

  Jamie Hill January 3rd, 2010 @ 04:06 PM

  Agreed on the Validator abstract class, I couldn't initially see a use for it but may be useful in some circumstances.

  As for the entry point being via the validates#{method}of, I don't quite follow. Wouldn't this mean you would need to write custom class methods for validators e.g. validates_email_of etc? I much prefer being able to just create a validator class and use it via:

  validates :attribute_name, :my_validator => { :my => 'options' }
  

  I don't really see the problem with validates_confirmation_of, this will just become:

  validates :password, :confirmation => true
  

  Am I missing something obvious?

• 

  José Valim January 3rd, 2010 @ 04:10 PM

  If you check validates_confirmation_of, you will see that it needs to be in the class scope to set things like attr_accessor. The same for validates_acceptance_of. Unless we send the class to the validator, which would work, but I don't like because the validator should not be attached to a class.

• You flagged this item as spam.
  

  Jamie Hill January 3rd, 2010 @ 05:54 PM

  I see what you mean about the attr_accessor. I think ideally that validations shouldn't magically add methods to the model and instead you should be expected to add this where required, however this would obviously break backwards compatibility. I believe the validate#{method}of methods should simply be proxies to the validators, this could go as far as using the new validates method e.g.

    %w(presence length numericality etc).each do |validation|
      define_method "validates_#{validation}_of", *attr_names do
        options = attr_names.extract_options!
        validates *attr_names, :length => options
      end
    end
  
    # or
  
    %w(presence length numericality etc).each do |validation|
      define_method "validates_#{validation}_of", *attr_names do
        options = attr_names.extract_options!
        validates_with "#{validation.camelize}Validator".constantize, options.merge(:attributes => attr_names)
      end
    end
  

  I suppose what I am saying is that I envisaged the validators being completely self contained and the validates??of just being convenience methods for validates/validates_with. This poses a problem like you say with confirmation so I will have a think on this and see if I can come up with a cleaner solution.

  We could always go the (dare I say) Python route an pass in self as the first argument to the Validator initializer :/ but I don't really like this.

• You flagged this item as spam.
  

  Jamie Hill January 3rd, 2010 @ 05:56 PM

  Sorry, that first code snippet was wrong, should be:

   %w(presence length numericality etc).each do |validation|
      define_method "validates_#{validation}_of", *attr_names do
        options = attr_names.extract_options!
        validates *attr_names, validation.to_sym => options
      end
    end
  

• You flagged this item as spam.
  

  Jamie Hill January 5th, 2010 @ 09:49 AM

  I am attaching a new diff of what I believe to be an elegant solution to the whole attr_accessor problem.

  I have moved the remaining logic into Validator classes and have managed to get around the need to pass in the class to the validators as we were both not happy with that. Basically, there is now a virtual_attributes instance method on validators which simply defines what virtual attributes should exist for the validator to work. With this information, validates_with now adds those accessors as long as they are not columns (check needed for acceptance_of).

  By doing things this way we are now able to delegate all validates?of calls to validates meaning that if it works with validates?of, it works with validates. All validates?of methods are just one-line calls to a private method that extracts the options and calls validates with the right arguments. This could be done with define_method as per previous comment but not sure how to get rdoc to behave with that.

  I have gone through and fixed some of the documentation as it was still referencing ActiveRecord and fleshed out the Validator documentation a little.

  I think this is a nice clean solution but please let me know if there's anything you're not happy with.

• You flagged this item as spam.
  

  Jamie Hill January 5th, 2010 @ 10:50 AM

  Just noticed an error when running on 1.8.7, attaching new patch file.

• 

  José Valim January 5th, 2010 @ 10:58 PM

  Moving this logic to validates_with is wrong, mainly because validates_with is in ActiveModel, which knows nothing database columns. So we are not there yet. Besides that, tests and docs looks good.

• You flagged this item as spam.
  

  Jamie Hill January 5th, 2010 @ 11:57 PM

  All I have done is move that logic from the validates_acceptance_of method which it should really be in either. As mentionned above I don't thing validations should be adding methods to the model on the fly anyway really. I can remove the column check and instead check to see if the model responds to the setter method but I didn't know if whoever had written that column code had done it for a reason.

• You flagged this item as spam.
  

  Jamie Hill January 5th, 2010 @ 11:59 PM

  Leave it with me and I'll try a few options.

• You flagged this item as spam.
  

  Jamie Hill January 6th, 2010 @ 08:00 PM

  Right, think we're there. Attached is new patch which instead of referencing columns, validates_with looks to see if a setter instance method is already available and skips if so.

• You flagged this item as spam.
  

  Jamie Hill January 7th, 2010 @ 01:06 AM

  New diff attached which uses setup hook instead of virtual_attributes as well as adding _merge_attributes private method. Also added further documentation for setup hook.

• You flagged this item as spam.
  

  Jamie Hill January 7th, 2010 @ 01:13 AM

  Added some comments, new patch attached.

  • sexy-validations-final.diff 55.4 KB
• 

  Repository January 7th, 2010 @ 06:25 PM

  (from [0a79eb7889e7ac711ff171a453d65f3df57b9237]) Add validates method as shortcut to setup validators for a given set of attributes:

  class Person < ActiveRecord::Base
  include MyValidators

  validates :name, :presence => true, :uniqueness => true, :length => { :maximum => 100 } validates :email, :presence => true, :email => true end

  [#3058 status:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  http://github.com/rails/rails/commit/0a79eb7889e7ac711ff171a453d65f...

• 

  José Valim January 7th, 2010 @ 06:26 PM

  • State changed from “open” to “committed”

• You flagged this item as spam.
  

  Jamie Hill January 17th, 2010 @ 04:25 AM

  José,

  I've just noticed a problem in the additional code you added, I am getting:

  NoMethodError: undefined method `slice!' for {:presence=>true}:Hash
          from /Users/jamie/.gem/ruby/1.8/gems/activemodel-3.0.pre/lib/active_model/validations/validates.rb
  

• 

  José Valim January 17th, 2010 @ 09:09 AM

  That was fixed as well, thanks!

• 

  Ryan Bigg October 9th, 2010 @ 09:52 PM

  • Tag cleared.

  Automatic cleanup of spam.