search

ankane / ahoy_email

First-party email analytics for Rails
MIT License
1.11k stars 137 forks source link

Fix Rails.application.secrets deprecation warning in Rails 7.1 #164

Closed kylerippey closed 1 month ago

kylerippey commented 1 month ago

We receive the following deprecation warning in our Rails 7.1 app because we use an environment variable to set up secret_key_base:

Failure/Error: require File.expand_path('../config/environment', __dir__)

ActiveSupport::DeprecationException:
  DEPRECATION WARNING: `Rails.application.secrets` is deprecated in favor of `Rails.application.credentials` and will be removed in Rails 7.2. (called from <top (required)> at /Users/***/code/recurrent/api/config/environment.rb:5)
# /Users/***/.rvm/gems/ruby-3.1.5/gems/ahoy_email-2.3.0/lib/ahoy_email/engine.rb:11:in `block in <class:Engine>'
# /Users/***/.rvm/gems/ruby-3.1.5/gems/railties-7.1.3.4/lib/rails/initializable.rb:32:in `instance_exec'
# /Users/***/.rvm/gems/ruby-3.1.5/gems/railties-7.1.3.4/lib/rails/initializable.rb:32:in `run'
# /Users/***/.rvm/gems/ruby-3.1.5/gems/railties-7.1.3.4/lib/rails/initializable.rb:61:in `block in run_initializers'
# /Users/***/.rvm/gems/ruby-3.1.5/gems/railties-7.1.3.4/lib/rails/initializable.rb:60:in `run_initializers'
# /Users/***/.rvm/gems/ruby-3.1.5/gems/railties-7.1.3.4/lib/rails/application.rb:426:in `initialize!'
# ...

In this PR I've attempted to address this by first checking if Rails.application responds to secret_key_base and it contains a truthy value before falling back to other mechanisms.

I don't love the big if/else block here, but since all of this is inside of a begin/end it prevented me from doing an early return.

I'd love to hear if you think this is a reasonably safe fix to address these deprecation warnings until 3.0 is released. I also looked into adding some tests, but I didn't see many that were already exercising this behavior so wasn't sure if I should add some or not. I'm happy to do so if you like!

Thanks for your consideration!

ankane commented 1 month ago

Hi @kylerippey, thanks for the PR! Pushed a fix in the commit above that I think preserves backwards compatibility slightly better.

kylerippey commented 1 month ago

Pushed a fix in the commit above that I think preserves backwards compatibility slightly better.

Nice fix! Looks like this fixes our deprecation warning as expected. Thank you!

ankane commented 1 month ago

Great, just pushed a new release.