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

More cleanup of new framework defaults #25235

Merged
merged 5 commits into from Jun 7, 2016

Conversation

Projects
None yet
4 participants
@prathamesh-sonpatki
Member

prathamesh-sonpatki commented Jun 1, 2016

  • Move real new default options to the top of the file.
  • After that club together all the options which were added to keep
    backward compatibility. So all of them will get only one header.
  • Based on #25231 (comment).

r? @kaspth

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 1, 2016

Now we get output like this -

$ edgerails new devrails --dev --force --skip-active-record --api

$ cat devrails/config/initializers/new_framework_defaults.rb
# Be sure to restart your server when you modify this file.
# This file contains all the new default configuration options from
# Rails 5.0.

# Following config options are introduced to ensure that apps
# made on earlier versions of Rails are not affected when upgrading.

# Do not halt callback chains when a callback returns false.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }

# Preserve the timezone of the receiver when calling to `to_time`.
# Ruby 2.4 will change the behavior of `to_time` to preserve the timezone
# when converting to an instance of `Time` instead of the previous behavior
# of converting to the local system timezone.
ActiveSupport.to_time_preserves_timezone = true
$ edgerails new devrails --dev --force --skip-active-record
$ cat devrails/config/initializers/new_framework_defaults.rb
# Be sure to restart your server when you modify this file.
# This file contains all the new default configuration options from
# Rails 5.0.

# Enable per-form CSRF tokens.
Rails.application.config.action_controller.per_form_csrf_tokens = true

# Enable origin-checking CSRF mitigation.
Rails.application.config.action_controller.forgery_protection_origin_check = true

# Following config options are introduced to ensure that apps
# made on earlier versions of Rails are not affected when upgrading.

# Do not halt callback chains when a callback returns false.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }

# Preserve the timezone of the receiver when calling to `to_time`.
# Ruby 2.4 will change the behavior of `to_time` to preserve the timezone
# when converting to an instance of `Time` instead of the previous behavior
# of converting to the local system timezone.
ActiveSupport.to_time_preserves_timezone = true
$ edgerails new devrails --dev --force --api
$ cat devrails/config/initializers/new_framework_defaults.rb
# Be sure to restart your server when you modify this file.
# This file contains all the new default configuration options from
# Rails 5.0.

# Following config options are introduced to ensure that apps
# made on earlier versions of Rails are not affected when upgrading.

# Require `belongs_to` associations by default.
Rails.application.config.active_record.belongs_to_required_by_default = true

# Do not halt callback chains when a callback returns false.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }

# Preserve the timezone of the receiver when calling to `to_time`.
# Ruby 2.4 will change the behavior of `to_time` to preserve the timezone
# when converting to an instance of `Time` instead of the previous behavior
# of converting to the local system timezone.
ActiveSupport.to_time_preserves_timezone = true
$ edgerails new devrails --dev --force
$ cat devrails/config/initializers/new_framework_defaults.rb 
# Be sure to restart your server when you modify this file.
# This file contains all the new default configuration options from
# Rails 5.0.

# Enable per-form CSRF tokens.
Rails.application.config.action_controller.per_form_csrf_tokens = true

# Enable origin-checking CSRF mitigation.
Rails.application.config.action_controller.forgery_protection_origin_check = true

# Following config options are introduced to ensure that apps
# made on earlier versions of Rails are not affected when upgrading.

# Require `belongs_to` associations by default.
Rails.application.config.active_record.belongs_to_required_by_default = true

# Do not halt callback chains when a callback returns false.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }

# Preserve the timezone of the receiver when calling to `to_time`.
# Ruby 2.4 will change the behavior of `to_time` to preserve the timezone
# when converting to an instance of `Time` instead of the previous behavior
# of converting to the local system timezone.
ActiveSupport.to_time_preserves_timezone = true
@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 1, 2016

Build failures are unrelated.

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 1, 2016

Here's my take on what the file should say:

# Be sure to restart your server when you modify this file.
#
# This file contains migration options to ease your Rails 5.0 upgrade.
# They will be removed in the next Rails version.
#
# Flip the defaults to the value of past versions then once upgraded
# begin migrating to the new default one by one.
#
# Read the Rails 5.0 release notes for more info on each option.

# Enable per-form CSRF tokens. Previous versions had false.
Rails.application.config.action_controller.per_form_csrf_tokens = true

# Enable origin-checking CSRF mitigation. Previous versions had false.
Rails.application.config.action_controller.forgery_protection_origin_check = true

# Require `belongs_to` associations by default. Previous versions had false.
Rails.application.config.active_record.belongs_to_required_by_default = true

# Halt callback chains when a callback returns `false`. Previous versions had true.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains. Previous versions had false.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }

# Make Ruby 2.4 preserve the timezone of the receiver when calling `to_time`.
# Previous versions had false.
ActiveSupport.to_time_preserves_timezone = true

I think it's a good idea to print out past version defaults so people have an easier time seeing how far they are in their migration.

However I wonder how much we should put in the top of the file intro. @dhh do you have any opinions on this? Overthinking or a worthy UX improvement?

@dhh

This comment has been minimized.

Member

dhh commented Jun 3, 2016

"They will be removed in the next Rails version" should probably be "They will be removed in the next major Rails version".

Otherwise good 👍

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 3, 2016

Great. I will update it soon :)

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 3, 2016

Sweet! Yeah, let's add "major" in there. I didn't use it at first because I thought people might conflate it with Rails 6.0 when they could be removed in Rails 5.1.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:cleanup branch Jun 3, 2016

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 3, 2016

@kaspth Regarding

Flip the defaults to the value of past versions

I added e72b2d0 to generate proper values for older apps getting upgraded to Rails 5. So we will by default generate flipped values for older apps. Let me know your thoughts on it.

prathamesh-sonpatki added some commits Jun 1, 2016

More cleanup of new framework defaults
- Move real new default options to the top of the file.
- After that club together all the options which were added to keep
  backward compatibility. So all of them will get only one header.
- Based on #25231 (comment).
Fix minor regression about old apps not getting per_form_csrf and req…
…uest_forgery_protection configs

- Earlier per_form_csrf_tokens and request_forgery_protection config
  files were generated for old apps upgraded to Rails 5.
- But when we collapsed all initializers into one file, the entire file
  does not get created for old apps.
- This commit fixes it and also changes values for all new defaults for
  old apps so that they will not break.
- Also added a test for `rails app:update`.
Update the documentation of new_framework_defaults file with more det…
…ails about old apps and how you can upgrade to new defaults.

[Kasper Timm Hansen, Prathamesh Sonpatki]

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:cleanup branch to bb19fb3 Jun 4, 2016

@@ -48,7 +48,7 @@ namespace :app do
require 'rails/generators'
require 'rails/generators/rails/app/app_generator'
gen = Rails::Generators::AppGenerator.new ["rails"],
{ api: !!Rails.application.config.api_only },
{ api: !!Rails.application.config.api_only, update: true, force: ENV['FORCE'] },

This comment has been minimized.

@kaspth

kaspth Jun 4, 2016

Member

Was force just added for tests? What does it mean for end users?

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 7, 2016

Member

Yes. This is only for tests. The other option was manually create the generator class and pass update: true but I chose to call bin/rails app:update directly from tests.

In the long term, I would definitely like to move the update task to new command line infrastructure :)

This comment has been minimized.

@kaspth

kaspth Jun 7, 2016

Member

I really don't like the idea of adding this option since end users won't need it.

The tests already created the AppGenerator manually, what's the harm in continuing that? 😁

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 7, 2016

Member

Sure. I updated it to

generator = Rails::Generators::AppGenerator.new ["rails"], { update: true }, destination_root: app_root, shell: @shell

Notice the update: true part.

Rails.application.config.action_controller.per_form_csrf_tokens = true
# Enable origin-checking CSRF mitigation.
# Enable origin-checking CSRF mitigation. This is new feature of Rails 5.

This comment has been minimized.

@kaspth

kaspth Jun 4, 2016

Member

True these are new features but enabling them when upgrading has ramifications on apps. Let's wrap in update and set to false on upgrading apps; new apps get these features enabled.

Then for consistency, let's add Previous versions had false.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 7, 2016

Member

@kaspth Not sure if this will cause issues with older apps. I haven't dug into the implications. I will check once more closely.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 7, 2016

Member

Checked the documentation

  • config.action_controller.forgery_protection_origin_check configures whether the HTTP Origin header should be checked against the site's origin as an additional CSRF defense.

I think it is safe to not enable for older apps. I will update as per your suggestion.

@kaspth

This comment has been minimized.

Member

kaspth commented Jun 4, 2016

@prathamesh-sonpatki I ❤️❤️❤️ the upgrade switch. Making sure upgrading apps keep the previous default should quench a lot of upgrading woes.

@@ -102,10 +101,6 @@ def config_when_updating
gsub_file 'config/initializers/cookies_serializer.rb', /json(?!,)/, 'marshal'
end
unless new_framework_defaults_config_exist
remove_file 'config/initializers/new_framework_defaults.rb'
end

This comment has been minimized.

@kaspth

kaspth Jun 4, 2016

Member

Wait did this code say remove the file unless it exists? Well... 🤓

ActiveSupport.to_time_preserves_timezone = true
# Following config options are introduced to ensure that apps
# made on earlier versions of Rails are not affected when upgrading.

This comment has been minimized.

@kaspth

kaspth Jun 4, 2016

Member

Think this isn't true as the defaults introduced with new features in Rails 5 have ramifications on upgrading apps. Let's just remove this because the whole file has become about ensuring this 😁

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 7, 2016

Member

Yeah. No longer needed now.

# Do not halt callback chains when a callback returns false. Previous versions had true.
ActiveSupport.halt_callback_chains_on_return_false = <%= options[:update] ? true : false %>
<%- unless options[:update] -%>

This comment has been minimized.

@kaspth

kaspth Jun 4, 2016

Member

Defaulting to subdomains true will still be a default in 5.1 right? Then I think we should mirror the other configs; output this but flip the value.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 7, 2016

Member

Yes. In 5.1 we will default to subdomains: true. The problem with this is older apps might have Rails.application.config.ssl_options set to something else. That was the reason for not adding this change for older apps.

They will get a deprecation warning and hopefully set the config to some value before 5.1 --> https://github.com/rails/rails/blob/b24d44edbb4726d06df782dc5b4b0e52db467d97/actionpack/lib/action_dispatch/middleware/ssl.rb#L-69-L-73

# This file contains migration options to ease your Rails 5.0 upgrade.
# They will be removed in the next major Rails version.
#
# Flip the defaults to the value of past versions then once upgraded

This comment has been minimized.

@kaspth

kaspth Jun 4, 2016

Member

Now handled automatically. Let's say: Once upgraded flip defaults one by one to migrate to the new default.

This comment has been minimized.

@prathamesh-sonpatki
# made on earlier versions of Rails are not affected when upgrading.
<%- unless options[:skip_active_record] -%>
# Require `belongs_to` associations by default. Previous versions had false.

This comment has been minimized.

@kaspth

kaspth Jun 4, 2016

Member

Since we helpfully output the past version for upgraders, this is less helpful. Let's say Next major release defaults to true. here and in the others. That ought to give people a sense a sense of where they ought to go next, and people can rest easy seeing the already have the right default.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 7, 2016

Member

@kaspth We can generate this only for older apps now. New apps can have Previous versions had false..

This comment has been minimized.

@kaspth

kaspth Jun 7, 2016

Member

We can generate this only for older apps now

I'm not sure I understand?

@prathamesh-sonpatki

This comment has been minimized.

Member

prathamesh-sonpatki commented Jun 7, 2016

Updated as per @kaspth's suggestions.

Older app upgraded to Rails 5 get this now -

$ cat config/initializers/new_framework_defaults.rb 
# Be sure to restart your server when you modify this file.
#
# This file contains migration options to ease your Rails 5.0 upgrade.
# They will be removed in the next major Rails version.
#
# Once upgraded flip defaults one by one to migrate to the new default.
#
# Read the Rails 5.0 release notes for more info on each option.

# Enable per-form CSRF tokens. Next major version defaults to true.
Rails.application.config.action_controller.per_form_csrf_tokens = false

# Enable origin-checking CSRF mitigation. Next major version defaults to true.
Rails.application.config.action_controller.forgery_protection_origin_check = false

# Make Ruby 2.4 preserve the timezone of the receiver when calling `to_time`.
ActiveSupport.to_time_preserves_timezone = true

# Require `belongs_to` associations by default. Next major version defaults to true.
Rails.application.config.active_record.belongs_to_required_by_default = false

# Do not halt callback chains when a callback returns false. Next major version defaults to false.
ActiveSupport.halt_callback_chains_on_return_false = true

New apps -

$ edgerails new devrails --dev --force
$ cat devrails/config/initializers/new_framework_defaults.rb 
# Be sure to restart your server when you modify this file.
#
# This file contains migration options to ease your Rails 5.0 upgrade.
# They will be removed in the next major Rails version.
#
# Read the Rails 5.0 release notes for more info on each option.

# Enable per-form CSRF tokens. Previous versions had false.
Rails.application.config.action_controller.per_form_csrf_tokens = true

# Enable origin-checking CSRF mitigation. Previous versions had false.
Rails.application.config.action_controller.forgery_protection_origin_check = true

# Make Ruby 2.4 preserve the timezone of the receiver when calling `to_time`.
ActiveSupport.to_time_preserves_timezone = true


# Require `belongs_to` associations by default. Previous versions had false.
Rails.application.config.active_record.belongs_to_required_by_default = true

# Do not halt callback chains when a callback returns false. Previous versions had true.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains. Previous versions had false.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }
$ edgerails new devrails --dev --force --skip-active-record
$ cat devrails/config/initializers/new_framework_defaults.rb 
# Be sure to restart your server when you modify this file.
#
# This file contains migration options to ease your Rails 5.0 upgrade.
# They will be removed in the next major Rails version.
#
# Read the Rails 5.0 release notes for more info on each option.

# Enable per-form CSRF tokens. Previous versions had false.
Rails.application.config.action_controller.per_form_csrf_tokens = true

# Enable origin-checking CSRF mitigation. Previous versions had false.
Rails.application.config.action_controller.forgery_protection_origin_check = true

# Make Ruby 2.4 preserve the timezone of the receiver when calling `to_time`.
ActiveSupport.to_time_preserves_timezone = true

# Do not halt callback chains when a callback returns false. Previous versions had true.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains. Previous versions had false.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }
$ edgerails new devrails --dev --api --force

$ cat devrails/config/initializers/new_framework_defaults.rb 
# Be sure to restart your server when you modify this file.
#
# This file contains migration options to ease your Rails 5.0 upgrade.
# They will be removed in the next major Rails version.
#
# Read the Rails 5.0 release notes for more info on each option.

# Make Ruby 2.4 preserve the timezone of the receiver when calling `to_time`.
ActiveSupport.to_time_preserves_timezone = true

# Require `belongs_to` associations by default. Previous versions had false.
Rails.application.config.active_record.belongs_to_required_by_default = true

# Do not halt callback chains when a callback returns false. Previous versions had true.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains. Previous versions had false.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }
$ edgerails new devrails --dev --api --skip-active-record --force
$ cat devrails/config/initializers/new_framework_defaults.rb 
# Be sure to restart your server when you modify this file.
#
# This file contains migration options to ease your Rails 5.0 upgrade.
# They will be removed in the next major Rails version.
#
# Read the Rails 5.0 release notes for more info on each option.

# Make Ruby 2.4 preserve the timezone of the receiver when calling `to_time`.
ActiveSupport.to_time_preserves_timezone = true

# Do not halt callback chains when a callback returns false. Previous versions had true.
ActiveSupport.halt_callback_chains_on_return_false = false

# Configure SSL options to enable HSTS with subdomains. Previous versions had false.
Rails.application.config.ssl_options = { hsts: { subdomains: true } }

I tested the config for CodeTriage.com locally which is upgraded from Rails 4 to Rails 5. The flipped defaults seem to working fine. Though I had to do lot of changs related to gems to accommodate Rails 5.1.0.alpha. But that is not related to this PR.

@kaspth Please review.

@sgrif

View changes

...es/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults.rb.tt Outdated
Rails.application.config.active_record.belongs_to_required_by_default = true
#
# This file contains migration options to ease your Rails 5.0 upgrade.
# They will be removed in the next major Rails version.

This comment has been minimized.

@sgrif

sgrif Jun 7, 2016

Member

This isn't necessarily true. We don't need to mention this here, each of the setters will have a deprecation warning individually before they are removed.

This comment has been minimized.

@prathamesh-sonpatki

prathamesh-sonpatki Jun 7, 2016

Member

@sgrif 👍 Removed it.

@prathamesh-sonpatki prathamesh-sonpatki force-pushed the prathamesh-sonpatki:cleanup branch to b8bb600 Jun 7, 2016

@sgrif sgrif merged commit 1ffa9aa into rails:master Jun 7, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@prathamesh-sonpatki prathamesh-sonpatki deleted the prathamesh-sonpatki:cleanup branch Jun 7, 2016

kaspth added a commit that referenced this pull request Jun 7, 2016

Merge pull request #25235 from prathamesh-sonpatki/cleanup
More cleanup of new framework defaults
@kaspth

This comment has been minimized.

Member

kaspth commented Jun 7, 2016

Backported to 5-0-stable @ 5687ee0

@kaspth kaspth removed the needs backport label Jun 7, 2016

sgrif added a commit that referenced this pull request Jun 7, 2016

Merge pull request #25235 from prathamesh-sonpatki/cleanup
More cleanup of new framework defaults
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment