Activerecord::Base#clone does not set dirty bits

Sorry the code block didn't come correctly in previous comment.
Here is what I meant :

  user = User.new(:email => "someemail@email.com")
  u.email_changed? #=> true

verified +1

+1 Verified.

-1 A problem with the patch is illustrated by given test case:

  test "cloned object should threat as changed only modified attributes" do
    token = Topic.new :content => 'foo bar baz'
    assert !token.title_changed?
    
    token.save
    assert !token.title_changed?
    
    cloned_token = token.clone
    assert !cloned_token.title_changed?
  end

+1

@pleax I i disagree with your comment, as Federico and Arpit mentioned, the cloned objects shall be treated as new records then it follows that cloned objects should be dirty

@elcuervo But it doesn't! As you can see from test-case I provided, some attributes of cloned object behaves differently from new one.

Pleax if you read the thread in this ticket you will see what is the behavior that is bugging me and some of the other users.

What the thread starter and Armit are saying is that for them the clone method is doing is creating a new Object and copying all the attributes from the original. Even if those were blank in the first object.

So that's basically the idea of this ticket. To repeat Fian : One could argue that the behaviour is correct since #new() also results in #changed?() being false.

Doesn't make sense for me to copy the dirty attribute flag from the first object.

Still not clear to me.

You're trying to say that new objects doesn't treat modified attributes as changed?. But they do:

t = Topic.new :title => 'test'
t.changed? # => true
t.title_changed? # => true
t.content_changed? # => false (!)

And yes. There is no sence to copy dirty-flags from original object. There should be different implementation.

I'm trying to say that attributes of clonned object should be threated as changed? only if they differ from defaults.

Also I've found strange behavior with the patch applied:

t = Topic.new
t.changed? # => false
t.clone
t.changed? # => true (!)

I've attached a patch.

"I'm trying to say that attributes of clonned object should be threated as changed? only if they differ from defaults."

I think I can agree with that, I added a test and a fix. Please confirm. Also next time upload a patch so i can give you credit.

About the strange issue, I'm opening a new ticket now. Here : https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

I meant to say in the previous comment that the issue was present in master before my fix.

I added a test and a fix. Please confirm.

Don't see it.

I meant to say in the previous comment that the issue was present in master before my fix.

Agree.

Added forgotten patch

I'm not sure if check for blank? is acceptable here. Because model attributes may have non-empty defaults.

  test "non-empty defaults should not threated as changed" do
    topic = Topic.new
    assert_equal "non-empty-default", topic.group
    assert !topic.group_changed?
    
    cloned_topic = topic.clone
    assert_equal "non-empty-default", cloned_topic.group
    assert !topic.group_changed?
  end

Last assertion should be assert !cloned_topic.group_changed? offcourse.

We have updated the patch so that it takes default values into account. Cloning now involves creation of another model object against which attributes of the clone are checked (and marked as dirty if necessary), which is not great, but it was the best we could come up with.

I've attached patch with bunch of test-cases and my version of fix.

The patch itself :)

Please test this and report back so it can be committed.

+1

All tests pass.

gucci watches