-
Notifications
You must be signed in to change notification settings - Fork 21.7k
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
Generate less initializers in new/upgraded Rails apps #42538
Conversation
Currently when you make a new Rails app, we generate a lot of initializers. For new users, I think we should try and include as few as possible - the less files, the less daunting a new app is. And for upgrades I'd like to [continue to simplify the update process](rails#41083), in this case by not bringing back initializers you have probably already dismissed or modified. In this PR I'm proposing we remove two initializers: `application_controller_renderer.rb` and `cookies_serializer.rb`: **`application_controller_renderer.rb`**. This configures [`ActionController::Renderer`](https://api.rubyonrails.org/classes/ActionController/Renderer.html), for rendering views outside of controller actions. I don't think this is something most Rails apps will need (certainly not on day 1); users can configure this feature when they need it. **`cookies_serializer.rb`**. This was added for [Rails 4.1](https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#cookies-serializer). The behaviour is: - For new apps, the initializer says `:json`. - For upgraded apps that don't have the initializer, it is added with value `:marshal`. - If there's no initializer, the [default value](https://github.com/rails/rails/blob/c9a89a4067834a095a41084030b8c9ccdbce77d5/actionpack/lib/action_dispatch/middleware/cookies.rb#L589) is `:marshal`. Since nobody should be upgrading direct from Rails 4.0 to Rails 7.0, we can simplify this by using new framework defaults. So the behavior will now be: - For new apps, `config.load_defaults("7.0")` sets the value to `:json`. - The `new_framework_defaults_7_0.rb` file explains this, and suggests using `:hybrid` to be upgrade to JSON cookies. - No changes to [the code](https://github.com/rails/rails/blob/c9a89a4067834a095a41084030b8c9ccdbce77d5/actionpack/lib/action_dispatch/middleware/cookies.rb#L589); the default value is `:marshal` if you don't set one. So if you were not setting a `cookies_serializer` previously and you want to keep using `:marshal`, you'll need to explicitly set this before using `config.load_defaults("7.0")`, otherwise it will switch to `:json`. The upside of this is you won't get the `cookies_serializer.rb` file created for you every time you upgrade.
f05dcc2
to
de23812
Compare
@@ -94,6 +94,7 @@ def read_raw_cookie | |||
|
|||
config.action_dispatch.signed_cookie_digest = "SHA512" | |||
config.action_dispatch.signed_cookie_salt = "sha512 salt" | |||
config.action_dispatch.cookies_serializer = :marshal |
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 checked on main
, and Rails.application.config.action_dispatch.cookies_serializer
is nil
when the requests in these tests are made. If you change it to :json
it crashes - with this PR that happens automatically, so I had to switch back to :marshal
to get the tests to pass.
I think it's worth confirming if the tests are buggy or if this indicates a problem with the :json
serializer (I think that's outside the scope of this PR).
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.
@rafaelfranca while this is fresh in mind, do you have any thoughts on this?
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.
@ghiculescu You'd need to pass serializer: JSON
, to the MessageEncryptor
and MessageVerifier
in the test.
Like we do here:
json_value = ActiveSupport::MessageVerifier.new(secret, serializer: JSON).generate(45) |
There is a related ticket, that also has some fixes for the tests you mentioned: #33967 |
It seems the |
# If you're confident all your cookies are JSON formatted, you can switch to the `:json` formatter. | ||
# | ||
# See https://guides.rubyonrails.org/action_controller_overview.html#cookies for more information. | ||
# Rails.application.config.action_dispatch.cookies_serializer = :marshal |
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.
The new default is :json
, not marshal. So in this case this config should not be in the new_framework_defaults file. If anything we should set the config to :hybrid
to help people upgrading.
@@ -199,6 +199,7 @@ def load_defaults(target_version) | |||
|
|||
if respond_to?(:action_dispatch) | |||
action_dispatch.return_only_request_media_type_on_content_type = false | |||
action_dispatch.cookies_serializer = :json |
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 was just working on upgrading an app to Rails 7 and hit this case. Cookie serialization changed when changing to Rails 7 defaults, but this change isn't called out on https://guides.rubyonrails.org/configuring.html#results-of-config-load-defaults so it took me quite some time to find that this is where it changed. Had to review the code itself.
Is this something that should be documented at https://guides.rubyonrails.org/configuring.html#results-of-config-load-defaults? So that it's more clear that this default changed from using marshal
to json
for the Rails 7 defaults?
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.
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.
@ghiculescu I was reading the docs at https://guides.rubyonrails.org/configuring.html#results-of-config-load-defaults to try to find out which setting broke tests for me and I couldn't find any one that applied. I didn't use that process to be honest but only looked at the documentation where it was missing.
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.
You're right, it should be in that guide too. I will make a PR.
Could you please see if using new framework defaults fixes the issue for you?
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.
@ghiculescu Once I knew what the issue was it was really simple to fix, so I think it's more a documentation issue than anything else.
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.
Cool, I added to docs in #44067
Thank you for raising this.
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.
@ghiculescu Thanks for the quick fix! ❤️
As noted in rails#42538 (comment) this is missing from the docs.
This is a follow up to rails#42538 Currently when doing a Rails upgrade, all the [here](https://github.com/rails/rails/tree/main/railties/lib/rails/generators/rails/app/templates/config/initializers) will get recreated for you. Apart from the new framework defaults file (which you always want), the newest initializer in that folder is `permissions_policy.rb` which was added in Rails 6.1. Since you should upgrade Rails major versions one at a time, we can assume that anyone upgrading to Rails 7.1 (which this PR targets) will have already seen all of these initializers at least once and either chosen to use them, move them, or delete them. If you use the initializers then you'll get prompted to merge in any updates, this is fine. If you moved or deleted the initializers, then the update process recreates unwanted, potentially duplicate, config. This is annoying. So this PR proposes that we don't recreate initializers during the app update if they didn't already exist. This way we are not bothering users with initializers they have already made a decision on. But we are still presenting them to new users.
…`:json` GitHub: ref rangubaGH-7 In Rails v7, this setting defaults to `:json`. Using `:marshal` as the default value was for backward compatibility with Rails v4.0. We no longer need this default because we have been using the JSON format by overwriting it in `config/initializers/cookies_serializer.rb`. In Ranguba, this change has no effect since we don't use cookies. ref: rails/rails#42538 ref: https://guides.rubyonrails.org/configuring.html#config-action-dispatch-cookies-serializer
…`:json` GitHub: ref rangubaGH-7 In Rails v7, this setting defaults to `:json`. Using `:marshal` as the default value was for backward compatibility with Rails v4.0. We no longer need this default because we have been using the JSON format by overwriting it in `config/initializers/cookies_serializer.rb`. In Ranguba, this change has no effect since we don't use cookies. ref: rails/rails#42538 ref: https://guides.rubyonrails.org/configuring.html#config-action-dispatch-cookies-serializer
…`:json` (#30) GitHub: ref GH-7 In Rails v7, this setting defaults to `:json`. Using `:marshal` as the default value was for backward compatibility with Rails v4.0. We no longer need this default because we have been using the JSON format by overwriting it in `config/initializers/cookies_serializer.rb`. In Ranguba, this change has no effect since we don't use cookies. ref: rails/rails#42538 ref: https://guides.rubyonrails.org/configuring.html#config-action-dispatch-cookies-serializer
Currently when you make a new Rails app, we generate a lot of initializers. For new users, I think we should try and include as few as possible - the less files, the less daunting a new app is. And for upgrades I'd like to continue to simplify the update process, in this case by not bringing back initializers you have probably already dismissed or modified.
In this PR I'm proposing we remove two initializers:
application_controller_renderer.rb
andcookies_serializer.rb
:application_controller_renderer.rb
: This configuresActionController::Renderer
, for rendering views outside of controller actions. I don't think this is something most Rails apps will need (certainly not on day 1); users can configure this feature when they need it.cookies_serializer.rb
: This was added for Rails 4.1. The behaviour is::json
.:marshal
.:marshal
.Since nobody should be upgrading direct from Rails 4.0 to Rails 7.0, we can simplify this by using new framework defaults, that way we don't need to create an extra file for every new app. The behavior will now be:
config.load_defaults("7.0")
sets the value to:json
.new_framework_defaults_7_0.rb
file explains this, and suggests using:marshal
to keep the behavior you had before Rails 7, or:hybrid
to upgrade to JSON cookies.:marshal
if you don't set one.So if you were not setting a
cookies_serializer
previously and you want to keep using:marshal
, you'll need to explicitly set this before usingconfig.load_defaults("7.0")
, otherwise it will switch to:json
. The upside of this is you won't get thecookies_serializer.rb
file created for you every time you upgrade.You can see all the initializers in a new app here: https://github.com/rails/rails/tree/main/railties/lib/rails/generators/rails/app/templates/config/initializers
I think there's probably a few other files that don't need to be there, but these two seemed like the most clear cut changes to me. But I'm okay with expanding the scope of this!