rails 7.2+ support #132
rails 7.2+ support #132
Conversation
| @@ -19,7 +19,7 @@ Gem::Specification.new do |spec| | |||
|
|
|||
| spec.files = Dir.glob('{lib/**/*,*.{md,txt,gemspec}}') | |||
|
|
|||
| spec.add_dependency 'activerecord', '>= 6.1.5', '< 7.2' | |||
| spec.add_dependency 'activerecord', '>= 6.1.5', '< 7.3' | |||
There was a problem hiding this comment.
can you please add Rails 7.1 and 7.2 to the testmatrix .github/workflows/build.yml
There was a problem hiding this comment.
kind of a noob on this one - would it need to be activerecord: ["~> 6.1", "~> 7.2"] in build.yml?
There was a problem hiding this comment.
I think it should be activerecord: ["~> 6.1", "~> 7.0" "~> 7.1" "~> 7.2"] so everything is tested. But I'm not an expert here as well.
| @@ -10,6 +10,11 @@ class VersionHelper | |||
| # ::ActiveRecord::VERSION::MAJOR > 7 || | |||
| # ::ActiveRecord::VERSION::MAJOR == 7 && ::ActiveRecord::VERSION::MINOR >= 1 | |||
| # end | |||
|
|
|||
| def self.at_least_7_2? | |||
| ::ActiveRecord::VERSION::MAJOR == 7 && ::ActiveRecord::VERSION::MINOR >= 2 | |||
There was a problem hiding this comment.
You need to add ::ActiveRecord::VERSION::MAJOR > 7 || if the changes also apply for Rails 8.0
There was a problem hiding this comment.
unclear if this will work on rails 8 at this time
There was a problem hiding this comment.
The name of the method suggests that it will apply to Rails 8.0.
I believe that the Associations::AliasTracker is not reverting to a simple connection.
Even if there are issues in Version 8, it would be simpler to start with the Rails 7.2 changes rather than the Rails 7.1 changes.
ActiveRecord 8.0.0.rc2 is green with the changes you made. 👍
| @@ -119,7 +119,7 @@ def build(relation, buckets) | |||
|
|
|||
| join_list = join_nodes + joins | |||
|
|
|||
| alias_tracker = Associations::AliasTracker.create(relation.klass.connection, relation.table.name, join_list) | |||
| alias_tracker = Associations::AliasTracker.create(BabySqueel::ActiveRecord::VersionHelper.at_least_7_2? ? relation.klass.connection.pool : relation.klass.connection, relation.table.name, join_list) | |||
There was a problem hiding this comment.
Can you please point to the Documentation why this change is needed.
There was a problem hiding this comment.
the first param for the init method for Associations::AliasTracker was changed to accept a pool now: https://github.com/rails/rails/blob/v7.2.1.2/activerecord/lib/active_record/associations/alias_tracker.rb#L9
lib/baby_squeel/nodes/proxy.rb
Outdated
| module BabySqueel | ||
| module Nodes | ||
| # This proxy class allows us to quack like any arel object. When a | ||
| # method missing is hit, we'll instantiate a new proxy object. | ||
| class Proxy < ActiveSupport::ProxyObject | ||
| class Proxy < (BabySqueel::ActiveRecord::VersionHelper.at_least_7_2? ? BasicObject : ActiveSupport::ProxyObject) |
There was a problem hiding this comment.
Do we need a version check here or does the BasicObject also work in older versions?
ActiveSupport::ProxyObject will be removed: https://github.com/rails/rails/blob/v7.2.1.1/activesupport/lib/active_support/proxy_object.rb
MR: rails/rails@ea7d3c5
Was that part importent?
undef_method :==
undef_method :equal?
There was a problem hiding this comment.
good catch here - the gist of this was they deprecated ActiveSupport:Proxy in favor of BaseObject - I tested BaseObject against 6.x and 7.x and it does seem to work just fine.
| @@ -19,7 +19,7 @@ Gem::Specification.new do |spec| | |||
|
|
|||
| spec.files = Dir.glob('{lib/**/*,*.{md,txt,gemspec}}') | |||
|
|
|||
| spec.add_dependency 'activerecord', '>= 6.1.5', '< 7.2' | |||
| spec.add_dependency 'activerecord', '>= 6.1.5', '< 7.3' | |||
There was a problem hiding this comment.
I think it should be activerecord: ["~> 6.1", "~> 7.0" "~> 7.1" "~> 7.2"] so everything is tested. But I'm not an expert here as well.
| @@ -10,6 +10,11 @@ class VersionHelper | |||
| # ::ActiveRecord::VERSION::MAJOR > 7 || | |||
| # ::ActiveRecord::VERSION::MAJOR == 7 && ::ActiveRecord::VERSION::MINOR >= 1 | |||
| # end | |||
|
|
|||
| def self.at_least_7_2? | |||
| ::ActiveRecord::VERSION::MAJOR == 7 && ::ActiveRecord::VERSION::MINOR >= 2 | |||
There was a problem hiding this comment.
The name of the method suggests that it will apply to Rails 8.0.
I believe that the Associations::AliasTracker is not reverting to a simple connection.
Even if there are issues in Version 8, it would be simpler to start with the Rails 7.2 changes rather than the Rails 7.1 changes.
ActiveRecord 8.0.0.rc2 is green with the changes you made. 👍
|
Hi, is there any update on this? We're looking forward to upgrade to rails 7.2 and waiting for this compatibility support 🙂 |
a few minor changes - left it backwards compatible with 6.1.5. All tests passing 👍