323: upgrade to rails 5.1 #324
Conversation
| sign_in resource_name, resource, bypass: true | ||
| bypass_sign_in(resource) |
There was a problem hiding this comment.
This was because of devise deprecation warning
| <%= devise_error_messages! %> | ||
| <%= render "devise/shared/error_messages", resource: resource %> |
There was a problem hiding this comment.
Resolve devise deprecation warning
| # monkey patch for the json gem to fix ruby keyword arg deprecation | ||
| # https://github.com/flori/json/issues/399 | ||
| module JSON | ||
| module_function | ||
|
|
||
| def parse(source, opts = {}) | ||
| Parser.new(source, **opts).parse | ||
| end | ||
| end |
There was a problem hiding this comment.
I added this to fix ruby deprecation warning. The code is in the gem. See ruby/json#399
| prepend_before_action :require_no_authentication, only: [:new, :create, :cancel] | ||
| prepend_before_action :authenticate_scope!, only: [:edit, :update, :destroy] |
There was a problem hiding this comment.
I didn't know what the difference between a before_action and a prepend_before_action was, so I looked it up.
A prepend_before_action runs before a before_action. There are no before_actions in this controller, so it seems like these prepends could just be regular before_actions. 🤔
Since we're dealing with Devise stuff, and I am not sure of the order of loading or any including that may be happening between controllers, I feel hesitant to make that change. Of course, if the Devise docs say this is the way, then this is the way. I'll do a deeper dive on that in a bit. I've got 80 more files to investigate...
There was a problem hiding this comment.
Yeah, I looked and that's the default Devise code: https://github.com/heartcombo/devise/blob/main/app/controllers/devise/registrations_controller.rb
| @@ -17,6 +17,8 @@ | |||
|
|
|||
| module TmdbMoviequeue | |||
| class Application < Rails::Application | |||
| # Initialize configuration defaults for originally generated Rails version. | |||
| config.load_defaults 5.1 | |||
| # Randomize the order test cases are executed. | ||
| config.active_support.test_order = :random |
There was a problem hiding this comment.
To clarify what we just talked about, this setting is being removed here because this is the new default behavior. There will be no change in behavior by removing this line.
| # Rails.application.config.assets.paths << Emoji.images_path | ||
| # Add Yarn node_modules folder to the asset load path. | ||
| Rails.application.config.assets.paths << Rails.root.join('node_modules') |
| # Read the Guide for Upgrading Ruby on Rails for more info on each option. | ||
|
|
||
| # Make `form_with` generate non-remote forms. | ||
| Rails.application.config.action_view.form_with_generates_remote_forms = false |
There was a problem hiding this comment.
If you plan to keep this setting, we need to move it into a config file. This config/initializers/new_framework_defaults_5_1.rb is a temporary file for use to help us turn on/off new settings for a slow upgrade rollout. We should plan to delete this file soon. This post has a little bit of insight https://lilyreile.medium.com/rails-5-1-new-framework-defaults-what-they-do-and-how-to-safely-uncomment-them-93921a2a5ec2
There was a problem hiding this comment.
Thanks for insights. I had no idea! After reading up on it, it looks like it only impacts form_with (which we're not currently using) and not form_for.
If we get to a point where we add a new form, surely we'll check to see what it's doing and can address it then?
So I think this can just be skipped.
The only other line in this file is commented out, so I think I'll just remove this whole file?
| @@ -11,7 +11,7 @@ def request(endpoint, params) | |||
| return { results: [] } if params[:query].present? && searchable_query(params[:query]).empty? | |||
|
|
|||
| api_path = build_url(endpoint, params) | |||
| response = open("#{BASE_URL}#{api_path}").read | |||
| response = URI.open("#{BASE_URL}#{api_path}").read | |||
There was a problem hiding this comment.
How in the heck did this even work before?
There was a problem hiding this comment.
🤷 Based on the deprecation warning, I think it was still calling open on URI
Related Issues & PRs
Closes #323
Problems Solved
Screenshots
Things Learned
rails app:updatecommand. Run it and hold on to your butts