Skip to content

Avoid suggesting very unlikely changes in rails app:update - #41083

Merged
rafaelfranca merged 1 commit into
rails:mainfrom
ghiculescu:less-choices-in-app-update
Feb 9, 2021
Merged

Avoid suggesting very unlikely changes in rails app:update#41083
rafaelfranca merged 1 commit into
rails:mainfrom
ghiculescu:less-choices-in-app-update

Conversation

@ghiculescu

@ghiculescu ghiculescu commented Jan 12, 2021

Copy link
Copy Markdown
Member

While upgrading a Rails 6 app to Rails 6.1, I noticed that rails app:update asks you to review some file changes that you'd basically never want to accept. In this PR, I propose we make the update task do a bit less, by not offering to overwrite files where the developer has almost certainly changed them from the default.

Specific changes:

  • Don't replace the following files, as they change very rarely at the framework level, so if the user has changes they almost certainly intend to keep them: config/storage.yml config/cable.yml config/puma.rb config.ru.
  • Don't overwrite the config/locales directory. There's basically no chance you'll want to replace your current config/locales/en.yml with a hello world.
  • Don't replace config/routes.rb. It's so unlikely that the user will want to replace their routes file with an empty file.

With these changes, you will still be prompted to accept/decline changes to these files when doing an update:

  • config/application.rb
  • config/environments/{development|test|production}.rb
  • All the default initializers that you already have a copy of. I go back and forth on also opting some of these out, for example assets.rb, filter_parameter_logging.rb, cors.rb, inflections.rb pretty much have to be changed to be useful.
  • All the default binstubs that you already have a copy of. I decided to still make the user review these, as bugs here are going to be annoying to debug, but it is good to use the latest versions if possible.

While here, I also made some touchups to the upgrade guide:

  • Don't list unsupported (per this) Rails versions in the "Ruby versions" section.
  • Explain new framework defaults a bit better.

@rails-bot rails-bot Bot added the railties label Jan 12, 2021
@ghiculescu
ghiculescu force-pushed the less-choices-in-app-update branch 2 times, most recently from 82fb9a5 to ced58a1 Compare January 12, 2021 01:17
@rails-bot rails-bot Bot added the docs label Jan 12, 2021
@ghiculescu
ghiculescu force-pushed the less-choices-in-app-update branch 2 times, most recently from ec10b1d to 1841d8b Compare January 12, 2021 01:23
@ghiculescu ghiculescu changed the title Avoid footguns in rails app:update Avoid suggesting very unlikely changes in rails app:update Jan 12, 2021
@ghiculescu
ghiculescu marked this pull request as ready for review January 12, 2021 01:35
Base automatically changed from master to main January 14, 2021 17:03
@ghiculescu
ghiculescu force-pushed the less-choices-in-app-update branch 2 times, most recently from dda9507 to 1afb63b Compare February 9, 2021 18:10
Comment thread railties/lib/rails/generators/rails/app/app_generator.rb Outdated
Comment thread railties/lib/rails/generators/rails/app/app_generator.rb Outdated
Comment thread railties/lib/rails/generators/rails/app/app_generator.rb Outdated
Comment thread railties/lib/rails/generators/rails/app/app_generator.rb Outdated
Comment thread railties/lib/rails/generators/rails/app/app_generator.rb Outdated
Comment thread railties/lib/rails/tasks/framework.rake Outdated
While upgrading a Rails 6 app to Rails 6.1, I noticed that `rails app:update` asks you to review some file changes that you'd basically never want to accept. In this PR, I propose we make the update task do a bit less, by not offering to overwrite files where the developer has almost certainly changed them from the default. Specific changes:

Don't replace the following files, as they change very rarely at the framework level, so if the user has changes they almost certainly intend to keep them:
- [config/boot.rb](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/boot.rb.tt)
- [config/environment.rb](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/environment.rb.tt)
- [config/storage.yml](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/storage.yml.tt)
- [config/spring.rb](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/spring.rb.tt)
- [config/cable.yml](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/cable.yml.tt)
- [config/puma.rb](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config/puma.rb.tt)
- [config.ru](https://github.com/rails/rails/commits/master/railties/lib/rails/generators/rails/app/templates/config.ru.tt)

Don't overwrite the config/locales directory. There's basically no chance you'll want to replace your current `config/locales/en.yml` with a [hello world](https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/app/templates/config/locales/en.yml).

Don't replace config/routes.rb. It's so unlikely that the user will want to replace their routes file with [an empty file](https://github.com/rails/rails/blob/master/railties/lib/rails/generators/rails/app/templates/config/routes.rb.tt).

With these changes, you will still be prompted to accept/decline changes to these files when doing an update:

- config/application.rb
- config/environments/{development|test|production}.rb
- All the default [initializers](https://github.com/rails/rails/tree/master/railties/lib/rails/generators/rails/app/templates/config/initializers) that you already have a copy of. I go back and forth on also opting some of these out, specifically `assets.rb`, `filter_parameter_logging.rb`, and `inflections.rb` which seem pretty likely to have been changed.
- All the default [binstubs](https://github.com/rails/rails/tree/master/railties/lib/rails/generators/rails/app/templates/bin) that you already have a copy of. I decided to still make the user review these, as bugs here are going to be annoying to debug, but it is good to use the latest versions if possible.
@ghiculescu
ghiculescu force-pushed the less-choices-in-app-update branch from 1afb63b to 1d1c9f9 Compare February 9, 2021 19:21
@ghiculescu

ghiculescu commented Feb 9, 2021

Copy link
Copy Markdown
Member Author

@rafaelfranca thanks! I reverted my changes for config/environment.rb, config/boot.rb, and config/spring.rb.

@rafaelfranca
rafaelfranca merged commit a57c6ee into rails:main Feb 9, 2021
@ghiculescu
ghiculescu deleted the less-choices-in-app-update branch February 9, 2021 20:05
ghiculescu added a commit to ghiculescu/rails that referenced this pull request Jun 18, 2021
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.
jyoun-godaddy pushed a commit to jyoun-godaddy/activestorage that referenced this pull request Jul 5, 2022
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/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.
y-yagi added a commit to y-yagi/rails that referenced this pull request Jun 11, 2024
This is a partial revert of rails#41083.

Rails has improved `puma.rb` sometimes, for example, 06d614a
and f719787.

To allow users to know those improvements, I think we should update
`puma.rb` by `app:update`.
y-yagi added a commit to y-yagi/rails that referenced this pull request Jun 11, 2024
This is a partial revert of rails#41083.

`puma.rb` may update by users, but  Rails has improved `puma.rb` sometimes.
For example, 06d614a and f719787.

To allow users to know those improvements, I think we should update
`puma.rb` by `app:update`.
Set2005 pushed a commit to Set2005/fix-association-initialize-order that referenced this pull request Jul 8, 2024
This is a partial revert of rails#41083.

`puma.rb` may update by users, but  Rails has improved `puma.rb` sometimes.
For example, 06d614a and f719787.

To allow users to know those improvements, I think we should update
`puma.rb` by `app:update`.
DanielaVelasquez pushed a commit to DanielaVelasquez/rails that referenced this pull request Oct 3, 2024
This is a partial revert of rails#41083.

`puma.rb` may update by users, but  Rails has improved `puma.rb` sometimes.
For example, 06d614a and f719787.

To allow users to know those improvements, I think we should update
`puma.rb` by `app:update`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants