Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Attach ConfirmationValidator to proper attribute #8123

Closed
wants to merge 1 commit into from

5 participants

@bcardarella

This is a follow up to fcc534e

This patch will attach the ConfirmationValidator to the _confirmation
attribute instead of the original attribute. I believe this should be
the preferred method instead of faking the desired behavior by just
rendering to the error message to the _confirmation attribute.

@bcardarella

/cc @carlosantoniodasilva I'm sure this will require some discussion but I believe this is better than what is currently being done.

activemodel/lib/active_model/validations/confirmation.rb
@@ -2,16 +2,23 @@ module ActiveModel
module Validations
class ConfirmationValidator < EachValidator # :nodoc:
- def validate_each(record, attribute, value)
- if (confirmed = record.send("#{attribute}_confirmation")) && (value != confirmed)
- human_attribute_name = record.class.human_attribute_name(attribute)
- record.errors.add(:"#{attribute}_confirmation", :confirmation, options.merge(:attribute => human_attribute_name))
+ def initialize(options)
+ options[:attributes].map! { |attribute| "#{attribute}_confirmation".to_sym }
+ super
+ end
+
+ def validate_each(record, attribute, confirm_value)
+ original_attribute = attribute.to_s.split(/_confirmation/).first

I think you can just use gsub('_confirmation', '') instead.

@rafaelfranca Owner

Is better to use tr. tr('_confirmation', '')

Hmm... Not sure tr is going to work in this case:

>> 'password_confirmation'.tr('_confirmation', '')
=> "psswd"

It'll replace all matching chars, including the ones in password.

@rafaelfranca Owner

True

Actually, would sub be preferred over gsub here? I'm sure the difference would be very small but I imagine sub is faster.

I think so, should work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva carlosantoniodasilva commented on the diff
activemodel/lib/active_model/validations/confirmation.rb
@@ -2,16 +2,23 @@ module ActiveModel
module Validations
class ConfirmationValidator < EachValidator # :nodoc:
- def validate_each(record, attribute, value)
- if (confirmed = record.send("#{attribute}_confirmation")) && (value != confirmed)
- human_attribute_name = record.class.human_attribute_name(attribute)
- record.errors.add(:"#{attribute}_confirmation", :confirmation, options.merge(:attribute => human_attribute_name))
+ def initialize(options)
+ options[:attributes].map! { |attribute| "#{attribute}_confirmation".to_sym }

It shouldn't be necessary to convert them to symbols here.

@rafaelfranca Owner

:+1:

The reason I am forcing to a symbol here is so the test suite doesn't go nuts. There are apparently a lot of expectations sprinkled throughout the test suite that require a symbol. If I don't do the typecasting the suite throws 479 errors. I figured I'd err on the side of less changes.

Let me know how I should handle.

Ok, I think it's fine, this could be handled separately if necessary.

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

@bcardarella thanks, I'll ping someone else to review.

@rafaelfranca
Owner

I think is worth to add some explanation about the original behavior and about the new behavior (with code) in the upgrade guide, and in the changelog message.

I really didn't understand what changed yet.

@carlosantoniodasilva
@bcardarella

@rafaelfranca should the explanation go into regular inline docs or into the guide?

@rafaelfranca
Owner

@bcardarella I prefer to put in the upgrading to 4.0 guides

@bcardarella

@rafaelfranca ok, updated for the requested change and updated the upgrade guide as well. Please let me know if there is anything else.

@bcardarella bcardarella Attach ConfirmationValidator to proper attribute
This is a follow up to fcc534e

This patch will attach the ConfirmationValidator to the _confirmation
attribute instead of the original attribute. I believe this should be
the preferred method instead of faking the desired behavior by just
rendering to the error message to the _confirmation attribute.
d567c7d
@steveklabnik
Collaborator

This now needs a rebase.

@rafaelfranca @carlosantoniodasilva are you guys happy with this, once it gets rebased?

@rafaelfranca
@gregmolnar

This issue is stale and can be closed I suppose. Also #10918 can be closed IMHO.

@rafaelfranca
Owner

I'm closing more because this will change the behavior and may break a lot of applications. Maybe in Rals 5 we can revisit it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 6, 2012
  1. @bcardarella

    Attach ConfirmationValidator to proper attribute

    bcardarella authored
    This is a follow up to fcc534e
    
    This patch will attach the ConfirmationValidator to the _confirmation
    attribute instead of the original attribute. I believe this should be
    the preferred method instead of faking the desired behavior by just
    rendering to the error message to the _confirmation attribute.
This page is out of date. Refresh to see the latest.
View
2  activemodel/CHANGELOG.md
@@ -68,7 +68,7 @@
* Passing false hash values to `validates` will no longer enable the corresponding validators *Steve Purcell*
-* `ConfirmationValidator` error messages will attach to `:#{attribute}_confirmation` instead of `attribute` *Brian Cardarella*
+* `ConfirmationValidator` will attach to `:#{attribute}_confirmation` instead of `attribute` *Brian Cardarella*
* Added ActiveModel::Model, a mixin to make Ruby objects work with AP out of box *Guillermo Iguaran*
View
17 activemodel/lib/active_model/validations/confirmation.rb
@@ -2,16 +2,23 @@ module ActiveModel
module Validations
class ConfirmationValidator < EachValidator # :nodoc:
- def validate_each(record, attribute, value)
- if (confirmed = record.send("#{attribute}_confirmation")) && (value != confirmed)
- human_attribute_name = record.class.human_attribute_name(attribute)
- record.errors.add(:"#{attribute}_confirmation", :confirmation, options.merge(:attribute => human_attribute_name))
+ def initialize(options)
+ options[:attributes].map! { |attribute| "#{attribute}_confirmation".to_sym }

It shouldn't be necessary to convert them to symbols here.

@rafaelfranca Owner

:+1:

The reason I am forcing to a symbol here is so the test suite doesn't go nuts. There are apparently a lot of expectations sprinkled throughout the test suite that require a symbol. If I don't do the typecasting the suite throws 479 errors. I figured I'd err on the side of less changes.

Let me know how I should handle.

Ok, I think it's fine, this could be handled separately if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ super
+ end
+
+ def validate_each(record, attribute, confirm_value)
+ original_attribute = attribute.to_s.sub('_confirmation', '')
+ value = record.send(original_attribute)
+ if value != confirm_value
+ human_attribute_name = record.class.human_attribute_name(original_attribute)
+ record.errors.add(attribute, :confirmation, options.merge(:attribute => human_attribute_name))
end
end
def setup(klass)
klass.send(:attr_accessor, *attributes.map do |attribute|
- :"#{attribute}_confirmation" unless klass.method_defined?(:"#{attribute}_confirmation")
+ attribute unless klass.method_defined?(attribute)
end.compact)
end
end
View
2  activemodel/test/cases/validations/confirmation_validation_test.rb
@@ -21,7 +21,7 @@ def test_no_title_confirmation
t.title_confirmation = nil
t.title = "Parallel Lives"
- assert t.valid?
+ assert t.invalid?
t.title_confirmation = "Parallel Lives"
assert t.valid?
View
9 guides/source/upgrading_ruby_on_rails.md
@@ -51,9 +51,12 @@ Rails 4.0 has changed `serialized_attributes` and `attr_readonly` to class metho
### Active Model
-Rails 4.0 has changed how errors attach with the `ActiveModel::Validations::ConfirmationValidator`.
-Now when confirmation validations fail the error will be attached to
-`:#{attribute}_confirmation` instead of `attribute`.
+Rails 4.0 has changed which attribute `ActiveModel::Validations::ConfirmationValidator`
+attaches to. Now the validator will attach to
+`:#{attribute}_confirmation` instead of `attribute`. This was done
+to improve the usability of form errors. The error message for a failed
+confirmation will render on the confirmation field instead of the
+original attribute field.
Rails 4.0 has changed `ActiveModel::Serializers::JSON.include_root_in_json` default
value to `false`. Now, Active Model Serializers and Active Record objects have the
Something went wrong with that request. Please try again.