Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Run callbacks from object's metaclass

  • Loading branch information...
commit e0846c8417093853f4f7f62732983e990c28d669 1 parent 98dd722
@josh josh authored
View
2  activesupport/CHANGELOG
@@ -1,5 +1,7 @@
*2.1.1 (next release)*
+* Run callbacks from object's metaclass [Josh Peek]
+
* TimeWithZone: when crossing DST boundary, treat Durations of days, months or years as variable-length, and all other values as absolute length. A time + 24.hours will advance exactly 24 hours, but a time + 1.day will advance 23-25 hours, depending on the day. Ensure consistent behavior across all advancing methods [Geoff Buesing]
* Fix TimeWithZone unmarshaling: coerce unmarshaled Time instances to utc, because Ruby's marshaling of Time instances doesn't respect the zone [Geoff Buesing]
View
10 activesupport/lib/active_support/callbacks.rb
@@ -269,7 +269,15 @@ def self.#{callback}_callback_chain
# pass
# stop
def run_callbacks(kind, options = {}, &block)
- self.class.send("#{kind}_callback_chain").run(self, options, &block)
+ callback_chain_method = "#{kind}_callback_chain"
+
+ # Meta class inherits Class so we don't have to merge it in 1.9
+ if RUBY_VERSION >= '1.9'
+ metaclass.send(callback_chain_method).run(self, options, &block)
+ else
+ callbacks = self.class.send(callback_chain_method) | metaclass.send(callback_chain_method)
+ callbacks.run(self, options, &block)
+ end
end
end
end
View
24 activesupport/test/callbacks_test.rb
@@ -84,6 +84,30 @@ def test_save_person
end
end
+class MetaclassCallbacksTest < Test::Unit::TestCase
+ module ModuleWithCallbacks
+ def self.extended(object)
+ object.metaclass.before_save :raise_metaclass_callback_called
+ end
+
+ def module_callback_called?
+ @module_callback_called ||= false
+ end
+
+ def raise_metaclass_callback_called
+ @module_callback_called = true
+ end
+ end
+
+ def test_metaclass_callbacks
+ person = Person.new
+ person.extend(ModuleWithCallbacks)
+ assert !person.module_callback_called?
+ person.save
+ assert person.module_callback_called?
+ end
+end
+
class ConditionalCallbackTest < Test::Unit::TestCase
def test_save_conditional_person
person = ConditionalPerson.new

6 comments on commit e0846c8

@technoweenie

Wow, this is breaking one of my apps. I’d suggest leaving this out of 2-1-stable.


SystemStackError in 'Whatevs blah blah client code'
stack level too deep
(eval):9:in `before_validation_callback_chain'
(eval):10:in `before_validation_callback_chain'
spec/models/whatevs_spec.rb:75:
spec/models/whatevs_spec.rb:3:
@josh
Collaborator

Really!?! I’ll revert it from stable in the meantime. Can you whip up a test case for that and ping me.

@methodmissing

Doesn’t play well with Marshal.

cache_fu enabled apps break with :

TypeError: singleton can’t be dumped

/test/cases/associations/extension_test.rb:36:in `test_marshalling_extensions’

@Roman2K

I think you should drop this change because from what I understand, each time a callback is run, #metaclass is called which creates a singleton class for the object if it doesn’t already exist, and from my benchmarks, this is very inefficient both CPU- and memory-wise. Please correct me if I’m wrong.

@josh
Collaborator

Causing too many problems, will revert.

@josh
Collaborator

@methodmissing Can whip up a marshal test for activerecord. Alarms should of sounded if that broke.

Please sign in to comment.
Something went wrong with that request. Please try again.