Deprecation warning on new Rails 3.1 install #169

Closed
mentalbrew opened this Issue Sep 14, 2011 · 10 comments

Comments

Projects
None yet
5 participants

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)
Contributor

botandrose commented Sep 14, 2011

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?

Contributor

botandrose commented Sep 14, 2011

Created a pull request for this: #170

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

Owner

mattwynne commented Sep 18, 2011

How will this fly on older versions of Rails?

Contributor

botandrose commented Sep 18, 2011

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.

Contributor

botandrose commented Sep 18, 2011

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?

Owner

mattwynne commented Sep 19, 2011

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.

Owner

aslakhellesoy commented Sep 20, 2011

@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?

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.

Owner

aslakhellesoy commented Sep 21, 2011

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

Owner

aslakhellesoy commented Sep 25, 2011

Fixed by 30e5e36 and 4e90fe0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment