Fix lazy load rails

[ericproulx]

Hello,

When running bumbler on our app, I found that paper_trail wasn't lazy loading ActiveRecord properly because it's requiring active_record.

So, I've created a railtie to lazy load ActionController and ActiveRecord. I also rearranged some file require to make sure it wont force ActiveRecord to load.

[tlynam]

Hi @jaredbeck, when it's convenient can you please help review?

[jaredbeck]

Thanks for taking this on, Eric.

This will require careful review. I've always found The Rails Initialization Process to be a difficult topic, and I'm not sure how well our test suite covers it.

I also need to review PT's "railtie history". For example, paper_trail/frameworks/rails/engine.rb defines an initializer, but it's deprecated, and I don't remember why. There have also been a few rails-initialization-related bugs over the years, and I'll want to read about them.

Sorry for thinking out loud here, but those are some of the things that I'd like to understand better before reviewing this.

[jaredbeck]

OK, I've spent a little time playing with this.

1. I'm in favor of lazy loading, in principle. Specifically, I agree that the early require "active_record" in lib/paper_trail.rb seems wrong.
2. This PR introduces PaperTrail::Railtie, but we already have PaperTrail::Rails::Engine. An Engine is a Railtie, so: is it OK to have both, and if so, why would we want both? If we decide to merge the two, which do we keep?
3. Most people have some PaperTrail configuration in an initializer of their own, config/initializers/paper_trail.rb. The documentation encourages this. I assume we want the end-user's initializer to happen last? If so, is there a way to make that explicit in code?
4. I'd like to have a lot more comments in the code. I work with these concepts (engines, railties) so rarely. It would be helpful to have comments explaining the order of operations, and linking to rails docs/guides/source. I can write some of these comments as I review this PR and learn more, but if anyone else wants to add some it'd be appreciated.
5. What is the best way to test these changes?

[ericproulx]

I found a little issue within our stack since AR is loaded properly. It's about abstract_class

We encountered this error :

superclass mismatch for class Version
/app/models/paper_trail/version.rb:8:in `<module:PaperTrail>'
/app/models/paper_trail/version.rb:7:in `<top (required)>'

Basically, since Rails 5.0, all models inherits from ApplicationRecord instead of ActiveRecord::Base. So we did the same for PaperTrail custom class. I guess we should just replace ApplicationRecord by ActiveRecord::Base. But, what if we replace the overriding by a single config attribute ?

module PaperTrail
  # This is the default ActiveRecord model provided by PaperTrail. Most simple
  # applications will use this model as-is, but it is possible to sub-class,
  # extend, or even do without this model entirely. See documentation section
  # 6.a. Custom Version Classes.
  #
  # The paper_trail-association_tracking gem provides a related model,
  # `VersionAssociation`.
  class Version < ::ActiveRecord::Base
    include PaperTrail::VersionConcern
    self.abstract_class = true if PaperTrail.config.version_is_abstract
  end
end

I guess it would be simpler than overriding.

[jaredbeck]

We can change paper_trail/frameworks/active_record/models/paper_trail/version.rb from < ::ActiveRecord::Base to < ::ApplicationRecord, if that's what you're suggesting. Makes sense to me.

Before raising any new topics, please let's address my questions above. Thanks.

[ericproulx]

OK, I've spent a little time playing with this.

1. I'm in favor of lazy loading, in principle. Specifically, I agree that the early require "active_record" in lib/paper_trail.rb seems wrong.
2. This PR introduces PaperTrail::Railtie, but we already have PaperTrail::Rails::Engine. An Engine is a Railtie, so: is it OK to have both, and if so, why would we want both? If we decide to merge the two, which do we keep?
3. Most people have some PaperTrail configuration in an initializer of their own, config/initializers/paper_trail.rb. The documentation encourages this. I assume we want the end-user's initializer to happen last? If so, is there a way to make that explicit in code?
4. I'd like to have a lot more comments in the code. I work with these concepts (engines, railties) so rarely. It would be helpful to have comments explaining the order of operations, and linking to rails docs/guides/source. I can write some of these comments as I review this PR and learn more, but if anyone else wants to add some it'd be appreciated.
5. What is the best way to test these changes?

1. 👍
2. I think you should keep the railtie
3. We could add a hook when PaperTrail has finished loading :

ActiveSupport.run_load_hooks(:paper_trail, self)

Then people could hook in their app like this :

ActiveSupport.on_load(:paper_trail) do
# user's code
end

4 : Sure adding comments is great idea.
5 : I think your test suite is already enough. We're not changing the core, just the order its dependencies are loaded.

About my last_comment, I would suggest the config way instead of the ApplicationRecord way. It's Rails tainted but config isn't.

[jaredbeck]

I think you should keep the railtie

I think so too. I'm looking at our engine and I don't think we need any of it. Please let me know when you've made this change and I'll review again.

I assume we want the end-user's initializer to happen last? If so, is there a way to make that explicit in code?

We could add a hook when PaperTrail has finished loading ..

I'd like to find a solution that doesn't require people to change their app code.

I think your test suite is already enough.

I'm not comfortable merging this without testing. Please research some other railtie-based gems and find out how they test their railtie. Thanks.

[jaredbeck]

Hi Eric, Still working on this? Want me to keep it open? Any guidance you need from me? Thanks!

[jaredbeck]

Superseded by #1281, preserving your authorship, Eric.