DEV: Upgrade Rails to 7.1 #27273
DEV: Upgrade Rails to 7.1 #27273
Conversation
github-actions
bot
added
the
i18n
Flink
force-pushed
the
loic-update-to-rails-7.1
branch
2 times, most recently
from
4d7e593
to
c6d2bfe
Compare
Flink
force-pushed
the
loic-update-to-rails-7.1
branch
from
94b55be
to
d7018ed
Compare
Flink
force-pushed
the
loic-update-to-rails-7.1
branch
2 times, most recently
from
07d2cfa
to
d1bd019
Compare
Flink
force-pushed
the
loic-update-to-rails-7.1
branch
from
04e8c92
to
0c86cd0
Compare
Flink
force-pushed
the
loic-update-to-rails-7.1
branch
from
0c86cd0
to
ea6318f
Compare
| @@ -10,12 +10,12 @@ | |||
| if Rails.env.development? && !Sidekiq.server? && ENV["RAILS_LOGS_STDOUT"] == "1" | |||
| Rails.application.config.after_initialize do | |||
| console = ActiveSupport::Logger.new(STDOUT) | |||
| original_logger = Rails.logger.chained.first | |||
| original_logger = Rails.logger.broadcasts.first | |||
There was a problem hiding this comment.
chained is a Logster::Logger attribute, are we changing some behaviour here?
There was a problem hiding this comment.
Yes, because Rails.logger is not a Logster::Logger anymore with the changes introduced in Rails 7.1. The Logster logger is added to the broadcast logger, like any other logger.
You can see the change I made some time ago to Logster for Rails 7.1 compatibility: discourse/logster@a023d77
| config.autoload_lib(ignore: %w[common_passwords emoji generators javascripts tasks]) | ||
| Rails.autoloaders.main.do_not_eager_load(config.root.join("lib")) |
There was a problem hiding this comment.
I am curious about the change in Rails 7.1 which requires us to do this now. Will you be able to share more? Thank you 🙇
There was a problem hiding this comment.
It’s what’s recommended here when upgrading: https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#config-autoload-lib-and-config-autoload-lib-once. They say it should still work, but it’s probably better to follow the new convention 🙂
| # load up schema cache for all multisite assuming all dbs have | ||
| # an identical schema | ||
| dup_cache = schema_cache.dup | ||
| # this line is not really needed, but just in case the | ||
| # underlying implementation changes lets give it a shot | ||
| dup_cache.connection = nil | ||
| ActiveRecord::Base.connection.schema_cache = dup_cache |
There was a problem hiding this comment.
I am curious about the reason for making this change. Is it because the Rails API is no longer supported? How does it affect preloading if this change is removed?
There was a problem hiding this comment.
Indeed, you can’t set the schema cache on the connection anymore. There’s a place where it might be feasible, but it’s buried relatively deep (on the pool config object) and that method has been removed from the current Rails code. From what I understand, though, the schema cache is automatically created per connection once. So I guess the worst-case scenario is that it will be fetched when the first connection is established (for each connection) instead of sharing the same one here like we were doing. I would tend to say it’s probably not that impactful, but I can’t really give you a definitive answer here.
Flink
force-pushed
the
loic-update-to-rails-7.1
branch
6 times, most recently
from
26a3d3d
to
22d4dea
Compare
| response = ActionDispatch::Response.new.tap { _1.request = request_copy } | ||
| instance.set_response!(response) | ||
| instance.set_request!(request_copy) |
There was a problem hiding this comment.
Aren't _1.request = request_copy and instance.set_request!(request_copy) doing the same thing?
There was a problem hiding this comment.
That’s what I was thinking at first, and instance.set_request!(request_copy) wasn’t there initially. But some of the code was failing. I don’t remember exactly why, but IIRC, sometimes things are delegated to the response object, and sometimes it’s directly in the request object set on the controller.
Flink
force-pushed
the
loic-update-to-rails-7.1
branch
from
22d4dea
to
52c37bc
Compare
Flink
force-pushed
the
loic-update-to-rails-7.1
branch
from
52c37bc
to
cf4789e
Compare
No description provided.