search

petehamilton / citier

CITIER (Class Inheritance & Table Inheritance Embeddings for Rails) is a solution for simple Multiple Class Inheritance in Rails.
88 stars 24 forks source link

new_record? is not being properly overridden in core_ext.rb #7

Open ryanchuleff opened 13 years ago

ryanchuleff commented 13 years ago

Peter,

The way your overriding the @new_record in in your is_new_record method is not correctly causing it to take. When the @object.save method is called from the controller, it returns successfully, but on the next line if i call @object.new_record? it still returns true, even though @object.id is non-nil. From what I can tell, the core code may have changed, but right now in rails 3.0.x, the new_record? method is located in ActiveRecord::Persistence class and the @new_record in there is not getting set when you override it in ActiveRecord::Base.

As a workaround, I've added a method to your core_ext called is_new_record? which returns the @new_record variable that your setting and that seems to work correctly even after I've bubbled up to the controller again, but this is clearly a hack as the core new_record? method should be transparent at the controller level.

I tried a number of different approaches without luck and I've run out of time to continue messing with it. The workaround is doing suitably for me for the time being so I'm moving ahead with that for now.

code excerpt below:

... def is_new_record(state) @new_record = state end

ryanchuleff

new_record? method is not getting set, because at no point do we actually persist

self to the database. The method above should be overriding that persistence layer

but its not. It looks like as of rails 3.0.x the native new_record? method is looking

to the ActiveBase::Persistence module to determine new_record status rather than to the

ActiveRecord::Base class where it previously went. For now, this new method is a hack

workaround to allow the controller methods above to know if we succeeded in saving.

Ideally this should be fixed so that the new_record? method is properly overridden.

def is_new_record?
  @new_record
end

end patch

def self.all(*args) ...

Good luck, and nice job so far on the gem, its working great otherwise!

-Ryan

petehamilton commented 13 years ago

Hi Ryan,

Thanks for making this an issue, I completely forgot I had to hack round that one!

I identified this issue while creating the gem and I'm afraid I had a bit of a brain blank on this one. It was causing some issues when trying to save up the hierarchy of models since the parent was incorrectly saying it hadn't been saved, which triggered errors preventing the child from saving as well and it all went wrong from there. You are right in thinking I just wrote my own version as a workaround but obviously this is less than ideal!

I attempted to simply override the method in my ActiveRecord class but obviously I should have been overriding in ActiveRecord::Persistence? If so I apologise for my oversight, it's been a tough few weeks of exams!

Thanks for your added change, if you fork and add it I will pull back in? I am really busy until Friday but if you feel like contributing even further by implementing it correctly and simply changing the few references to my workaround method to the correct one then please go ahead and I will pull in your changes. It's little things like this that are left niggling and making it not quite complete.

Pete

ryanchuleff commented 13 years ago

Hey Pete,

I found it in Altrabio's code:

def is_new_record(state)

@new_record = state

@persisted = !state

end

Just make that change and your good to go. No other changes are required elsewhere in your gem.

-Ryan