clean the erros if an object that includes validation is duped. #6284

Merged
merged 1 commit into from May 15, 2012

Conversation

Projects
None yet
4 participants
Contributor

acapilleri commented May 12, 2012

If an istance that include validation is duped it keep track of errors of its 'parent', because both errors instances point to the same ActiveModel::Errors instance. This happen when before to call dup, save or valid? is called. This Fixes #5953 and it was discussed on the pull request #5958

@erichmenge erichmenge commented on an outdated diff May 12, 2012

activemodel/lib/active_model/validations.rb
@@ -177,6 +177,11 @@ def inherited(base)
super
end
end
+
+ #Clean errors if istance is duped
@erichmenge

erichmenge May 12, 2012

Minor spelling and space problem here.

Contributor

acapilleri commented May 12, 2012

@erichmenge thanks

@rafaelfranca rafaelfranca commented on an outdated diff May 12, 2012

activemodel/lib/active_model/validations.rb
@@ -177,6 +177,11 @@ def inherited(base)
super
end
end
+
+ #Clean the +Errors+ if instance is duped
@rafaelfranca

rafaelfranca May 12, 2012

Owner

I think that this method should have a :nodoc: mark.

# Clean the errors if instance is duped
def initialize_dup(other) # :nodoc:
Contributor

acapilleri commented May 12, 2012

Thanks, done.

@carlosantoniodasilva carlosantoniodasilva commented on the diff May 13, 2012

activemodel/lib/active_model/validations.rb
@@ -177,6 +177,11 @@ def inherited(base)
super
end
end
+
+ # Clean the +Errors+ object if instance is duped
+ def initialize_dup(other) # :nodoc:
+ @errors = nil
@carlosantoniodasilva

carlosantoniodasilva May 13, 2012

Owner

Make sure you call super here after doing your work. (read more).

I'm leaning toward actually cloning the @errors object instead of nullifying it, would look something like:

@errors = other.errors.dup
@errors.instance_variable_set(:@base, self)

Seems more what initialize_dup is supposed to do, I just dislike a bit the instance_variable_set call (although the test would break in case anything changes in Errors). And in case we go this path, the test would need to be updated to reflect that the errors stay there after calling dup.

@josevalim @rafaelfranca how does that look for you guys?

Thanks!

Contributor

acapilleri commented May 13, 2012

ops... i just pushed it with super call..

Contributor

acapilleri commented May 13, 2012

I disagree about errors dup, because when you dup an activerecord you not dup the id, it could be inconsistent ( id duplicate for example)

@carlosantoniodasilva carlosantoniodasilva added a commit that referenced this pull request May 15, 2012

@carlosantoniodasilva carlosantoniodasilva Merge pull request #6284 from acapilleri/dup_validation
clean the erros if an object that includes validation  is duped.
3d1b078

@carlosantoniodasilva carlosantoniodasilva merged commit 3d1b078 into rails:master May 15, 2012

Contributor

acapilleri commented May 15, 2012

@carlosantoniodasilva do you think this could be merged also on the current 3-2?

I think so, don't see any harm on having this fixed there as well. If you want you can send another pull to 3-2, otherwise I can just merge here later, thanks.

@acapilleri acapilleri added a commit to acapilleri/rails that referenced this pull request May 15, 2012

@acapilleri acapilleri clean the errors if an object that includes validations errors is dup…
…ed,for 3-2-stable

It Fixes #5953 in 3-2-stable, it's the same pull request of #6284
396e383

acapilleri referenced this pull request May 15, 2012

Merged

Dup validation 3 2 #6324

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