-
Notifications
You must be signed in to change notification settings - Fork 21.9k
Fix 55215 test failure #55227
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
Fix 55215 test failure #55227
Conversation
👋 hey @fabricerenard12, thanks for taking a stab at this! It looks like there's a real failure in CI, could you take a look? |
5f94ea6
to
d3cbc72
Compare
@@ -1,5 +1,7 @@ | |||
# frozen_string_literal: true | |||
|
|||
require "action_controller/railtie" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the right fix is to require "action_controller` only so that all autoloads' are setup.
Requiring the railtie should be opt-in. In a normal Rails application this would not matter much, but in standalone scripts like Rails bug report templates (e.g. https://github.com/rails/rails/blob/05f0b1614d82424a7507caaec5cf701d3a8c2b21/guides/bug_report_templates/active_job.rb) we want to be able to choose which railtie to load.
d3cbc72
to
b678354
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We we'll also need to modify the Rails::ApplicationController
with the same fix https://github.com/rails/rails/blob/05f0b1614d82424a7507caaec5cf701d3a8c2b21/railties/lib/rails/application_controller.rb
It's the parent class of the PwaController which gets also eagerloaded. (it works right now without issue but if we end up swapping these two lines in the future it would fail the same way)
Lines 34 to 37 in 05f0b16
eager_autoload do | |
autoload :HealthController | |
autoload :PwaController | |
end |
Thanks !
b678354
to
e16893b
Compare
e16893b
to
7b680f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 ! Someone from the team with commit privilege will review and merge when they get the chance.
Thanks for your contribution !
Motivation / Background
Fixes #55215
Detail
This Pull Request adds
require "action_controller"
to railties/lib/rails/health_controller.rb, guaranteeing ActionController::Base is loaded before Rails::HealthController is defined and eliminating the NameError seen in #55215.Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]