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.

Sign up for GitHub

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

Jump to bottom

Fix rails 7.2 loading #1534

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Fix rails 7.2 loading #1534

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ad129a
Wrap Polyamorous module in on_load(:active_record)
jdlubrano Oct 29, 2024
a26e251
Remove defined? ActiveRecord check from form builder
jdlubrano Oct 29, 2024
e9f20f5
Require ActiveSupport.on_load explicitly
jdlubrano Oct 29, 2024
4ece2c4
Load ransack first in specs to expose load order issues
jdlubrano Oct 29, 2024
046bae7
Requiring more pieces of ActiveSupport
jdlubrano Nov 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/polyamorous/polyamorous.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
if defined?(::ActiveRecord)
ActiveSupport.on_load(:active_record) do
module Polyamorous
InnerJoin = Arel::Nodes::InnerJoin
OuterJoin = Arel::Nodes::OuterJoin
Expand Down
1 change: 1 addition & 0 deletions lib/ransack.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require 'active_support/dependencies/autoload'
Copy link
Author

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

require 'active_support/core_ext'
require 'ransack/configuration'
require 'polyamorous/polyamorous'
Expand Down
13 changes: 6 additions & 7 deletions lib/ransack/helpers/form_builder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,12 @@ module ActionView::Helpers::Tags
# https://github.com/rails/rails/commit/c1a118a
class Base
private
if defined? ::ActiveRecord
Copy link
Author

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 🤔 .

def value
if @allow_method_names_outside_object
object.send @method_name if object && object.respond_to?(@method_name, true)
else
object.send @method_name if object
end

def value
if @allow_method_names_outside_object
object.send @method_name if object && object.respond_to?(@method_name, true)
else
object.send @method_name if object
end
end
end
Expand Down
5 changes: 2 additions & 3 deletions spec/spec_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
require 'machinist/active_record'
require 'polyamorous/polyamorous'
require 'ransack'
require 'sham'
require 'faker'
require 'ransack'
require 'action_controller'
require 'ransack/helpers'
require 'pry'
require 'simplecov'
require 'byebug'
require 'machinist/active_record'

Copy link
Author

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.

SimpleCov.start
I18n.enforce_available_locales = false
Expand Down
Loading