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.

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

Move controller to app/controllers/health_check #110

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Crammaman
Copy link

The rails convention for a rails engine is to have the folder structure mimic a rails applications. With rails bringing in Zeitwerk this convention is being enforced. By not having the controller under app/controllers the controller (and so the constant ActionController::Base) is getting required during initialization.

This is currently raising the deprecation warning:
"Initialization autoloaded the constants ActionText::ContentHelper and ActionText::TagHelper. Being able to do this is deprecated. Autoloading during initialization is going to be an error condition in future versions of Rails."

Moving the controller into app/controllers/health_check and removing the require allows rails to autoload the controller correctly and so silences this deprecation warning.

The rails convention for a rails engine is to have the folder structure mimic a rails applications. With rails bringing in Zeitwerk this convention is being enforced. By not having the controller under app/controllers the controller (and so the constant ActionController::Base) is getting required during initialization. This is currently raising the deprecation warning:
"Initialization autoloaded the constants ActionText::ContentHelper and ActionText::TagHelper. Being able to do this is deprecated. Autoloading during initialization is going to be an error condition in future versions of Rails."

Moving the controller into app/controllers/health_check and removing the require allows rails to autoload the controller correctly and so silences this deprecation warning.
@thedarkside
Copy link

Can we get this merged?

@evolve2k
Copy link

Minor change fixes deprecation warning.
Looks good to merge :)

@jmarchello
Copy link
Contributor

I'd like to test this a bit before accepting it. It's sat for a couple years now and Rails 7 introduced some changes to autoloading.

@Crammaman
Copy link
Author

@jmarchello Fair enough, I can only really confirm that it works in the Rails 6 application I work on.

@valscion
Copy link
Contributor

valscion commented Aug 4, 2023

This change is also needed for Rails to use any config.action_controller values set inside an initializer.

Without this change, health_check gem loads ActionController::Base class before any initializers inside config/initializers/ run. When ActionController::Base is loaded, it copies the configuration values set in Rails.application.config.action_controller.

That is why the outcome of bin/rails app:update during a Rails upgrade process, which creates a config/initializers/*new_framework_defaults.rb file, doesn't work when health_check gem is present (without this PR). More information of this topic over at the Rails issue tracker:

@pkordel
Copy link

pkordel commented Aug 10, 2023

Bump... please?

@valscion
Copy link
Contributor

valscion commented Aug 10, 2023

For what it's worth, we've been able to work around this issue by setting gem 'health_check', require: false in Gemfile and loading the health_check gem only after ActionController has been loaded via an initializer like this:

# config/initializers/health_check.rb
ActiveSupport.on_load(:action_controller) do
  require 'health_check'
  HealthCheck.setup do |config|
    # ... configuration here ...
  end
end

@pkordel
Copy link

pkordel commented Aug 10, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants