Limit private callback lookup.

[svoop]

Support for private callbacks introduced in commit 71937c0 is a good thing, however, the way private callbacks are looked up caused a nasty issue in one of my models dealing with credit cards:

class CreditcardPayment < ActiveRecord::Base
  workflow do
    state :pending do
      (...)
      event :fail, transitions_to: :failed
    end
    state :failed
  end
end

Now try to transition with fail! and - kaboom - runtime error. The problem is the lookup in the workflow gem:

def has_callback?(action)
  self.respond_to?(action) or self.class.private_method_defined?(action)
end

Eventhough no private event handler fail is defined in the model, private_method_defined? still returns true as it does for any object due to Kernel#fail.

The problem is quite nasty and since "fail" appears to be a rather common transition in workflows, using another event name is not the best solution. But there's an easy way to limit the method lookup:

def has_callback?(action)
  self.respond_to?(action) or self.private_methods(false).include?(action)
end

Which is exactly what this pull request contains :-)

[dwbutler]

Thanks for catching that! This is a good idea, but this code won't work in Ruby 1.8.7 because private_methods returns an array of strings.

#1.9.3
"hello".private_methods(false)
 => [:initialize, :initialize_copy]

#1.8.7
"hello".private_methods(false)
=> ["initialize_copy", "initialize"]

In a previous commit I ended up mapping the return value to an array of symbols.

Would be good to see some tests included! =)

[svoop]

As you suggested, I've added map for Ruby 1.8.7 compatibility (can't wait for this Ruby generation to be retired next spring) and a test - all collapsed into one commit.

[dwbutler]

@geekq, think we can get a bugfix 0.8.7 release soon? =)

[geekq]

Sorry - was busy with my current project.

• I am going to switch the gem releasing away from jeweller to the modern pure bundler as I did for jetty-rackup
• Will apply all the piled up patches and release then

Going to do this this weekend/monday ETA: 2013-01-14

[geekq]

The change will brake class hierarchies with private callback methods. But it is less harm, than unintentionally calling some private methods, especially in kernel. So going to merge... Would like to test with ruby-1.9.3, ruby-1.8.7 and jruby. Unfortunately my current rbenv for 1.8.7 and 1.8.6 broken. Need to repair that first.

[dwbutler]

Wouldn't defining the methods as protected instead of private allow them to be called in a child class? (I'm not entirely sure - I'd have to do some testing to determine if respond_to? returns true for protected methods.)

[dwbutler]

After some testing I've determined the following about protected methods defined on a parent class: (this is consistent in 1.9.3 and 1.8.7):

• respond_to? returns false.
• protected_instance_methods includes only the protected methods, and no core Ruby methods.
• protected_method_defined? returns true

Based on these results, I say we add protected_method_defined? into the conditions and recommend people to use protected rather than private methods.

[dwbutler]

I made a gist to help me understand and document all the craziness behind private methods, protected methods, and inheritance:

https://gist.github.com/4554269

[geekq]

Hi,

I've prepared my continuous integration environment for testing with various ruby versions. Not yet on travis-ci.org, just on my notebook. Can now review/try out/merge.

@sven Thanks for the test and for the improved 1.8.7 compatibility!

@david Thanks for the 1.8.7 investigation and for the 'protected method' idea!

I think, using protected methods as callbacks is the best way to go:

• callbacks can be hidden (non public) as desired in Support for private callbacks #53
• no unintentional calls on fail! and other Kernel methods
• inheritance hierarchy with workflow is still supported

Best Regards,

Vladimir (geekq)

[dwbutler]

Thanks!

My idea was actually to support both private and protected methods, by using both private_instance_methods(false).map(&:to_sym).include? and protected_method_defined?. Do you think it's feasible to support both? I think a lot of Rubyists are used to using private rather than protected methods.

[geekq]

OK, adding support for private methods again ;-) - but only in the immediate class now.
Also expanding tests.

Big surprise: respond_to? returns true for inherited protected methods (for me). But it can change one more time in Ruby2.

Meta programming is hard and it is even harder with a moving target like Ruby 1.9 ;-)

So I'll keep all the 3 checks even if the second one currently seems to be unneeded on 1.8.7-p371 and 1.9.3-rc1

[dwbutler]

Great! I like how to checks are documented now with some comments! =D Otherwise it would be quite confusing to understand why all those checks are necessary.