-
-
Notifications
You must be signed in to change notification settings - Fork 804
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
Fix rails 7.2 loading #1534
base: main
Are you sure you want to change the base?
Fix rails 7.2 loading #1534
Conversation
@@ -1,3 +1,4 @@ | |||
require 'active_support/dependencies/autoload' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just requiring this explicitly since on_load
does get used further down in this file (and I suppose should be considered in any other files that get required from here).
@@ -6,13 +6,12 @@ module ActionView::Helpers::Tags | |||
# https://github.com/rails/rails/commit/c1a118a | |||
class Base | |||
private | |||
if defined? ::ActiveRecord |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that this was ever truly dependent on ActiveRecord. At one point, this was a version check. I agree with the comment that it would be nice to avoid this patch to ActionView, but I also think that the patch is necessary regardless of whether ActiveRecord is defined or not. Moreover, the code within doesn't depend on ActiveRecord as we are just calling send
on an object 🤔 .
cc: @scottbarrow |
require 'action_controller' | ||
require 'ransack/helpers' | ||
require 'pry' | ||
require 'simplecov' | ||
require 'byebug' | ||
require 'machinist/active_record' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
machinist/active_record
requires active_record
, which made all of the code wrapped in defined?(::ActiveRecord)
execute. Requiring machinist/active_record
after required ransack
, I think, defines a load order that better imitates a Rails app.
I am not sure whether going down this path is the best approach. I feel that perhaps ActiveSupport could require everything within active_support/core_ext to ensure that active_support/core_ext can load itself. Still, there's no changing old versions of Rails, so I suppose this solution works? I am also not sure what the Rails team would recommend when it comes to requiring specific pieces of ActiveSupport (to leverage things like mattr_accessor). In any case, I was able to reproduce the CI failures locally and these changes make everything pass for me locally.
Fixes #1518
Problem
In a typical Rails app, the Ransack gem gets loaded before ActiveRecord gets loaded. Rails recommends that gems avoid loading Rails frameworks when they themselves are loaded. Ransack accomplishes that (as far as I can tell); however, Ransack initializes certain parts of its code within blocks like
If Ransack is loaded before
ActiveRecord
is loaded, those blocks of code are never reached. I think these blocks of code should be wrapped withActiveSupport.on_load(:active_record)
instead ofif defined?(::ActiveRecord)
. The blocks of code should run afterActiveRecord
is loaded, which is whaton_load(:active_record)
is meant to accomplish.Test coverage
I wasn't quite sure how to cover this in the specs, so I adjusted the load order in
spec/spec_helper.rb
torequire "ransack"
first. That did expose the load order issue(s) within the gem, so maybe that is good enough. It might be ideal to set up a test that spins up a Rails app and runs a basic query just to check that the basic functionality of the gem works within Rails 🤷♂️ . For local development, I used a single file Rails app like this: