Deprecation warning on new Rails 3.1 install

[mentalbrew]

env.rb is giving me this error:

DEPRECATION WARNING: class_inheritable_attribute is deprecated, please use class_attribute method instead. Notice their behavior are slightly different, so refer to class_attribute documentation first. (called from require at /path/to/rails/features/support/env.rb:7)

[botandrose]

http://martinciu.com/2011/07/difference-between-class_inheritable_attribute-and-class_attribute.html

Offending method is in lib/cucumber/rails/hooks/active_record.rb line 3:

if defined?(ActiveRecord::Base)
  class ActiveRecord::Base
    class_inheritable_accessor :shared_connection

    def self.connection
      self.shared_connection || retrieve_connection
    end
  end

  Before('@javascript') do
     # Forces all threads to share a connection on a per-model basis,
     # as connections may vary per model as per establish_connection. This works
     # on Capybara because it starts the web server in a thread.
     ActiveRecord::Base.shared_connection = ActiveRecord::Base.connection
     ActiveRecord::Base.descendants.each do |model|
       model.shared_connection = model.connection
     end
   end

  Before('~@javascript') do
    # Do not use a shared connection unless we're in a @javascript scenario
    ActiveRecord::Base.shared_connection = nil
    ActiveRecord::Base.descendants.each do |model|
       model.shared_connection = nil
     end
  end
end

I've tried replacing the method with class_attribute, and it doesn't appear to break anything. However, after reading the article above, a cursory glance at the Before('@javascript') and Before('~@javascript') blocks seems to indicate that the difference between the two methods will have an effect. I think that it will simply remove the need for manually setting the descendants' copy of shared_connection via ActiveRecord::Base.descendants.each do |model|, because that's what class_attribute does anyway.

What do you guys think?

[botandrose]

Created a pull request for this: https://github.com/cucumber/cucumber-rails/pull/170

All tests pass before and after. I figured a new test wasn't required for this.

[mattwynne]

How will this fly on older versions of Rails?

[botandrose]

class_attribute showed up in ActiveSupport 3.0, so we should be good on Rails 3.0. As for Rails 2, cucumber-rails already doesn't support it.

[botandrose]

Looking at this again, I think we actually may need to keep the two ActiveRecord::Base.descendants.each do |model| blocks.

The first block I misread the first time through. The values of connection could change on a per-model basis, and class_attribute's new semantics won't give us that for free, clearly, so we should probably keep it. It'd be nice to have a test for this, maybe? I'll see if I can't reduce the need for this block down to a test-case.

The second block would be redundant with the new semantics of class_attribute, except if you set a subclass's attribute, it no longer tracks the parent's value, and that's what we're doing in the first block.

I wish I better understood the purpose of shared_connection. Anyone?

[mattwynne]

On 18 Sep 2011, at 18:28, Micah Geisel wrote:

I wish I better understood the purpose of shared_connection. Anyone?

The purpose is to help transactional database cleaning between scenarios to work. There are a few different ways to do it, and this was based on a hack of @josevalim's that @jnickas suggested. I think the idea is to ensure that ActiveRecord uses the same connection object across threads (since Capybara runs the Rails server in a separate thread). I've tried to find a reference to the discussion on the mailing list about this (there have been many) but you might be better off just looking at the history of the code.

[aslakhellesoy]

@winnipegtransit do you have any views on this? You provided the fix for #152 so maybe you know whether or not to keep iterating over descendants?

[winnipegtransit]

I think iterating over descendants is necessary. Like @botandrose said, each model could potentially be connected to a different database and thus have a different connection, so we do need to copy them over individually; just copying the topmost connection won't work.

I didn't use class_attribute because class_inheritable_attribute was what I was familiar with and it wasn't deprecated at the time. After looking at the differences between the two (and the article above) I think class_attribute should work, though you may want to pass :instance_reader => false, :instance_writer => false as flags, as changing the connection for a particular instance doesn't make sense.

If you do change the code to use class_attribute, we can run our app's (now on Rails 3.1) tests against it to see if anything breaks.

[aslakhellesoy]

@winnipegtransit - thanks for the heads up. Any chance you could run your tests with #170 applied?

[aslakhellesoy]

Fixed by 30e5e36d1de6122c33efa09859a3e7d9cc088c5e and 4e90fe0afdbd356159f59d3d65b4ce289602a342