Rails 5.1/5.0 compatibility: Use constant since string/symbol was removed #18077
Conversation
Rails 5.1 removed support for specifying a string/symbol for the middleware class. You must now use a constant. For fun, autoload doesn't work in application.rb, so you have to either do it in an config/initializers or require the file. rails/rails@83b767ce Rails issue: #25525 Extracted from ManageIQ#18076
There was a problem hiding this comment.
So, I have no problem with this change, but I am confused how it was working previously. The issue sited was on the 5.0.0.rc2 version, so it was incredibly early in the 5.0 branch, so why haven't we seen this ourselves previously?
Since I think this is the middleware you just added, do we need to backport something to make sure that works as expected?
Anyway, I think this change is still fine as is, just some thoughts.
| @@ -109,7 +109,8 @@ class Application < Rails::Application | |||
|
|
|||
| config.autoload_once_paths << Rails.root.join("lib", "vmdb", "console_methods.rb").to_s | |||
|
|
|||
| config.middleware.use 'RequestStartedOnMiddleware' | |||
| require_relative '../lib/request_started_on_middleware' | |||
| config.middleware.use RequestStartedOnMiddleware | |||
There was a problem hiding this comment.
Could we also consider putting this change in the initializer as well, ala something like:
rails/rails@83b767ce#commitcomment-19818100
I have no preference if you do, it would just avoid a require when it will probably get autoloaded as part of app initialization anyway.
There was a problem hiding this comment.
I don't have a strong preference. I've always thought middlewares are important enough to be in the application.rb provided there aren't many of them. I guess we can put it in an initializer but thought this was ok as is since we can't rely upon autoload or reloading here.
It was deprecated in rails 5.0 but removed in rails 5.1 here: I guess because I just added it and didn't see the deprecation messages, it went unnoticed.
I don't think we'll upgrade hammer to rails 5.1, but if we do, we'll need to backport this. |
|
Oh, I see, rails/rails#25525 was saying that this works: But does cause the deprecation warning:
Thought it was just blowing up entirely regardless. Sorry for the confusion and miss reading the linked issue. I think what you have is fine. Don't need to do any tricks for this. |
Rails 5.1 removed support for specifying a string/symbol for the
middleware class. You must now use a constant. For fun, autoload
doesn't work in application.rb, so you have to either
do it in an config/initializers or require the file.
rails/rails@83b767ce
Rails issue: #25525
Extracted from #18076