search

westonganger / protected_attributes_continued

The community continued version of protected_attributes for Rails 5+
MIT License
45 stars 33 forks source link

Gem interferes with detection of ActiveRecord in other gems #4

Closed noahsettersten closed 7 years ago

noahsettersten commented 7 years ago

Setup:

Hello,

In defining a module on ActiveRecord in /lib/protected_attributes_continued.rb, any other gems that check for ActiveRecord seem to falsely assume it has been loaded.

I ran into this issue on upgrading a Rails 4 application that makes heavy use of MongoDB via the mongoid gem. Since protected_attributes_continued extends ActiveRecord by default whether it has been included or not, a number of other gems believed that ActiveRecord was in use even though it hadn't been loaded in the Rails startup.

I understand this gem is a continuation of the now-unsupported Rails feature and may only be designed to serve the use-case of ActiveRecord models. If that's the case I fully understand, but this was an unexpected consequence of switching from the previous protected_attributes gem.

noahsettersten commented 7 years ago

Here's the relevant portion of config/application.rb:

require "rails"
# Pick the frameworks you want:
require "active_model/railtie"
require "active_job/railtie"
# require "active_record/railtie"
require "action_controller/railtie"
require "action_mailer/railtie"
require "action_view/railtie"
require "action_cable/railtie"
require "sprockets/railtie"

For now, I was able to work around this by creating a local vendor copy of the previous protected_attributes code with a few minor modifications to play nice with Mongoid and Rails 5.

westonganger commented 7 years ago

Maybe we can use a class_eval or module_eval instead of defining the module.

I am not familiar with non-activerecord especially mongoid. Can you provide the relevant upgrades for your local copy in a gist or something?

noahsettersten commented 7 years ago

I can share those changes, but I'm not sure they'll be very helpful here. I'm not too familiar with the inner workings of ActiveModel and really the only changes I made to the original code was to swap some requires to use require_relative for locating the correct code.

As for the differences with non-activerecord / mongoid, it seems that only the modules within the /lib/active_model folder are applied and in use. Perhaps if the initial ActiveModel::Core.initialize was injected like so instead of defined in the main /lib/protected_attributes_continued.rb:

require 'active_record/custom_initialize' if defined? ActiveRecord

Otherwise, our use is a pretty edge case overall (we've actually planned to re-architect some of this in the future). I wouldn't focus too much of your time if it isn't a simple change.

westonganger commented 7 years ago

I just made a branch non_active_record please try that and report your results for both with active_record and non-active_record

noahsettersten commented 7 years ago

Thanks for the quick work!

westonganger commented 7 years ago

Sorry I meant test new new branch in a regular Rails app as well. I dont expect master to work with your app yet.

noahsettersten commented 7 years ago

I'm following you now, I had assumed you were looking for a comparison of the two branches on my app.

Running a fresh app via rails new on rails@5.1.0 with the new non_active_record branch doesn't result in any errors during startup and the 'Yay! You're on Rails' message successfully appears on the main page. Did you have any specific tests or checks in mind?

westonganger commented 7 years ago

The reason this fix was added was for migrations. I recommend testing this. Thanks for your help by the way.

noahsettersten commented 7 years ago

In the new Rails app, I created a Users model and generated a few migrations to add/remove fields. Migrations ran successfully and I didn't see any errors in the server logs when starting up. Anything beyond this is probably best tested by another user who is using ActiveRecord in a live app.

westonganger commented 7 years ago

Ya sure no worries. This seems good, lets merge this baby and I'll try to get a new version out today.

noahsettersten commented 7 years ago

That will be great, thank you for investigating right away.

westonganger commented 7 years ago

Still working on that new version. Can you test the master branch and see if its still working for your non-activerecord use case. If that passes I can release a new version.

noahsettersten commented 7 years ago

It's looking great in my specific use-case. Ship it!

westonganger commented 7 years ago

v1.3.0 is now released.