search

pat / combustion

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

Rails main branch deprecation on cache_format_version #138

Closed jrochkind closed 3 days ago

jrochkind commented 4 months ago

I use combustion to test under multiple rails versions, and usually use rails edge (current tip of main) as one of the versions, to stay ready for the next rails release as soon as it happens. This works out well for me.

It also means I notice any time combustion triggers a deprecation on Rails main.

On post-Rails 7.1, rails main @ c402ec78724a

DEPRECATION WARNING: Support for `config.active_support.cache_format_version = 6.1` has been deprecated and will be removed in Rails 7.2.

Check the Rails upgrade guide at https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-activesupport-cache-serialization-format
for more information on how to upgrade.
 (called from initialize! at /Users/jrochkind/.gem/ruby/3.2.3/bundler/gems/combustion-b3bf79a7bcff/lib/combustion.rb:56)

I wonder if combustion should just be doing a config.load_defaults #{current_rails_version_loaded}, to always be loading whatever framework defaults are for whatever version of rails is being tested? That is probably what I would want to test with! And then you wouldn't have to manually keep defaults up to date/working/non-deprecated for each version of rails?

pat commented 4 months ago

I wonder if combustion should just be doing a config.load_defaults #{current_rails_version_loaded}, to always be loading whatever framework defaults are for whatever version of rails is being tested? That is probably what I would want to test with! And then you wouldn't have to manually keep defaults up to date/working/non-deprecated for each version of rails?

Yes, this sounds great to me! I wonder if this approach didn't exist when I first created combustion, but surely it's covered in every supported version of Rails now? 🤞🏻

If you want to take a go at a PR, that'd be brilliant, but otherwise I'll try to get to it soon.

jrochkind commented 4 months ago

I think you are right that it didn't exist back when Combustion started.

And combustion, at least according to gemspec, still supports Rails back to 3.x! So it might have to be a conditional application.

Not sure what to do with the Rails after config.load_defaults exists but for which combustion currently does not do load_defaults. Perhaps for backwards compatibility, combustion would only do the load_defaults trick going forward for rails > 7.1. (Current main branch rails claims to be 7.2.0.alpha1, although I think it's maybe never going to be released as that but as 8.0 instead, not sure).

jrochkind commented 4 months ago

Not totally sure if I'll have time to work on it; if I do, I'll post here saying I'm working on it, but at the moment, I am not!

Thanks!

pat commented 3 days ago

Closing this issue as I've just merged @ankane's work in making this the new behaviour for 7.2 onwards. I'm open to have it being the default behaviour for all Rails versions that do allow for config.load_defaults - but that feels like a breaking change, so we'll (finally) get this current code out in a release and then ponder that.