search

pat / combustion

Simple, elegant testing for Rails Engines
MIT License
707 stars 51 forks source link

legacy_connection_handling= no longer supported in Rails main/edge #123

Closed jrochkind closed 2 years ago

jrochkind commented 2 years ago

Hi! I use combustion in one of my engine gems for testing, where I also test against multiple Rails versions. I include Rails edge (HEAD of master branch in git), because I want to find out as soon as possible if my gem has stopped working against future Rails!

Combustion as of recently does not work against Rails main branch, because Rails has removed legacy_connection_handling configuration, which Combustion tries to set in Rails >= 7:

https://github.com/pat/combustion/blob/792a6890b5f27bde33f813550ea39d7c781260e2/lib/combustion/configurations/active_record.rb#L7-L9

My build fails in a kind of odd way there, failing with: NoMethodError: undefined methodlegacy_connection_handling=' for ActiveRecord::Base:Class`

How might one fix this? I experimented with respond_to?, but that doesn't seem to work. Perhaps just rescuing and ignoring the NoMethodError, I don't know. Or actually you can check ActiveRecord::Base.respond_to(:legacy_connection_handling=) to get proper answers in Rails 7.0 release vs edge.

It is a bit odd, and it's true might settle out as Rails edge continues to be developed. But for my use of combustion, I'd like to keep it working on Rails edge.

However, in the past when I've brought up failures on Rails edge, I think you've said you weren't interested in supporting Rails edge, you'd release a new version of combustion to fix problems only with actual Rails releases.

If that is still your opinion (which may be reasonable!), I may migrate my app away from combustion, as I really like including Rails edge in my build matrix! Let me know what you think? If you are interested in fixing this, I can maybe supply a PR.

pat commented 2 years ago

Hi Jonathan - I'm not against PRs for Rails edge :) though I won't be prioritising new version releases of combustion with those changes, so hopefully it's fine for you to refer to combustion via a git reference?

As for how to change the code - it looks like the edge version number is now 7.1.0.alpha, so perhaps changing if ActiveRecord::VERSION::MAJOR >= 7 to instead be if ActiveRecord::VERSION::STRING.to_f = 7.0 might do the trick?

jrochkind commented 2 years ago

If you'd prefer to stick to a version checking approach rather than a feature-detection approach, my inclination would be to go ahead and use Gem::Version for it's existing correct semantics for version comparisons, in this case I guess something like:

ar_release_version = Gem::Version.new(ActiveRecord::VERSION::String).release 
ar_release_version >= Gem::Version.new("7.0") && ar_release_version < Gem::Version.new("7.1")

Or maybe that's too confusing, cause of the release necessary to drop the .alpha so the comparison works. I dunno.

jrochkind commented 2 years ago

Interstingly, combustion itself also seems to test off rails-edge in it's CI! https://github.com/pat/combustion/blob/main/Appraisals#L82

I'm surprised combustion is not failing it's own CI test suite right now due to this issue. Perhaps it is, but the test suite hasn't run in a while, if there hasn't been a PR in a while, and you don't have automatic regular CI runs set up?