Skip to content
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

Fix specs on Rails 6 RC2 #5109

Merged
merged 5 commits into from
Aug 7, 2019
Merged

Fix specs on Rails 6 RC2 #5109

merged 5 commits into from
Aug 7, 2019

Conversation

tegon
Copy link
Member

@tegon tegon commented Aug 6, 2019

The ActiveRecord::MigrationContext class now has a schema_migration attribute.
Reference: https://github.com/rails/rails/pull/36439/files#diff-8d3c44120f7b67ff79e2fbe6a40d0ad6R1018

This pull request also includes some changes to avoid deprecation warnings: 3273f9e, 77cb394 and 69534c6

Before Rails 6 RC2, the `ActionDispatch::Response#content_type` method
would return only the media part of the `Content-Type` header, without any
other parts. Now the `#content_type` method returns the entire header -
as it is - and `#media_type` should be used instead to get the previous
behavior.

Ref:
- rails/rails#36034
- rails/rails#36854
Render file will need the full path in order to avoid security breaches.
In this particular case, there's no need to use render file, it's ok to
use render template.

Ref: rails/rails#35688
@tegon tegon self-assigned this Aug 6, 2019
Copy link

@lucasnar lucasnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with Devise. Not sure if my comments make sense. 😅

@@ -46,7 +46,7 @@ class Application < Rails::Application
end

# Remove this check once Rails 5.0 support is removed.
if Devise::Test.rails52_and_up?
if Devise::Test.rails52_and_up? && !Devise::Test.rails6?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably something like # Remove the first check once Rails 5.0 support is removed. now? Do you know if Rails 6 is always going to require this? (It might make sense to update the docs to consider it too.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll still need the rails6? check once we remove support for Rails 5 because here we need to set this config for Rails 5.2. When Rails 6 is the only supported version, then we can remove this code altogether.

@@ -8,6 +8,10 @@ module Devise
module Test
# Detection for minor differences between Rails 4 and 5, 5.1, and 5.2 in tests.

def self.rails6?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we update this module's docs as well?

@tegon
Copy link
Member Author

tegon commented Aug 7, 2019

@lucasnar Thanks!

@tegon tegon merged commit ad58923 into master Aug 7, 2019
@tegon tegon deleted the let-rails6-rc2 branch August 7, 2019 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants