Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for ActiveRecord 5 #129

Closed
md5 opened this issue Jun 8, 2016 · 11 comments
Closed

Support for ActiveRecord 5 #129

md5 opened this issue Jun 8, 2016 · 11 comments

Comments

@md5
Copy link

md5 commented Jun 8, 2016

It looks like Rails 5.0.0 will be released relatively soon, so I wanted to open an issue regarding support for ActiveRecord 5.

Since the version range for activerecord is set to ">= 3.2", "< 5", Bundler will happily allow it to be installed with 5.0.0.rc1, which is what I have in my application (5.0.0.rc1 is considered < 5 by RubyGems). So far, I haven't noticed any incompatibilities, but I haven't done extensive testing beyond basic feature flag changes.

@md5
Copy link
Author

md5 commented Jun 9, 2016

I just ran across an issue that seems to be related to flipper-active_record. I had an initializer that looked like this to set up the memorizer middleware:

# config/initializers/flipper.rb

module MyApplication
  def self.flipper
    @flipper ||= begin
      adapter = Flipper::Adapters::ActiveRecord.new

      Flipper.new(adapter)
    end
  end
end

require 'flipper/middleware/memoizer'
Rails.application.config.middleware.use Flipper::Middleware::Memoizer, -> { MyApplication.flipper }

I also have this initializer generated by rails new:

# config/initializers/active_record_belongs_to_required_by_default.rb 

# Be sure to restart your server when you modify this file.

# Require `belongs_to` associations by default. This is a new Rails 5.0
# default, so it is introduced as a configuration option to ensure that apps
# made on earlier versions of Rails are not affected when upgrading.
Rails.application.config.active_record.belongs_to_required_by_default = true

At some point, I noticed that ActiveRecord::Base.belongs_to_required_by_default was no longer set to true. I did a git bisect and found that it started when I added flipper-active_record to my project.

After I changed my Gemfile to add require: false to my gem 'flipper-active_record' line and added an explicit require 'flipper-active_record' to my initializer above, I once again had ActiveRecord::Base.belongs_to_required_by_default == true as expected.

@jnunemaker
Copy link
Collaborator

Thanks for opening the issue and testing. I don't think there should be anything big that would be incompatible, but we should definitely start testing. Step 1 is probably to get a rails 5 gemfile going that runs in travis ci. It doesn't make sense that flipper would affect initializers like you've mentioned. Seems like something else complex is going on there, but let me know if it is flipper.

@md5
Copy link
Author

md5 commented Jun 11, 2016

@jnunemaker It didn't make sense to me either, but git bisect seemed to point to the commit where I added flipper. As you say, it's quite likely there's more to it.

@jnunemaker
Copy link
Collaborator

I just realized I already have rails 5 in the matrix for ci.
https://github.com/jnunemaker/flipper/blob/ce311e5d4f5d570ffd9dae65572cf5688b07ee4e/script/test#L23-L25

It was on beta3, so I just updated it to rc1. Pretty sure that everything is good as the tests have been passing so I'm going to close this, but let me know if you run into any issues.

As far as the issue you mentioned above, seems really weird. The only thing that I do in the AR adapter is require "active_record" and setup a couple of models, so it doesn't seem related. Please let me know if you find that it is.

@md5
Copy link
Author

md5 commented Jul 6, 2016

@jnunemaker Looks like the version dependency for flipper-active_record is still ">= 3.2", "< 5". Since Rails 5.0 was just released, it's no longer possible to install flipper-active_record with a non-beta or RC version of Rails 5.

@jnunemaker
Copy link
Collaborator

Just waiting on CI:
https://github.com/jnunemaker/flipper/compare/rails5

I'll release after CI is green.

@jnunemaker
Copy link
Collaborator

0.9.1 is out with rails 5 support. Let me know if you hit any issues.

@md5
Copy link
Author

md5 commented Jul 7, 2016

Thanks @jnunemaker!

@md5
Copy link
Author

md5 commented Aug 17, 2016

@jnunemaker We came across the same issue with belongs_to_required_by_default breaking with another gem, in this case delayed_job. It looks like the fix proposed there (and one made for devise) is to use ActiveSupport.on_load to require 'activerecord'.

See rails/rails#23589, collectiveidea/delayed_job_active_record#128, collectiveidea/delayed_job_active_record#106 (PR), and heartcombo/devise@c2c74b0

@md5
Copy link
Author

md5 commented Aug 17, 2016

johnreilly added a commit to johnreilly/flipper that referenced this issue Nov 10, 2016
This prevents overwriting the  setting in Rails 5, as described in flippercloud#129
@johnreilly
Copy link
Contributor

FWIW, we just ran into this as well. Using flipper v0.9.2 and flipper-active_record v0.9.2 with rails v5.0.0.1.

@md5's suggestion below resolved the issue for us for now:

After I changed my Gemfile to add require: false to my gem 'flipper-active_record' line and added an explicit require 'flipper-active_record' to my initializer above, I once again had ActiveRecord::Base.belongs_to_required_by_default == true as expected.

I had a go at implementing the ActiveSupport.on_load fix. I will submit a PR here shortly, if that's useful. (It's been a while since I've submitted patches, so forgive me if I goof it up!)

Thanks for tracking this down, @md5! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants