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

Deprecate legacy_connection_handling #41859

Merged

Conversation

eileencodes
Copy link
Member

This deprecates legacy_connection_handling via the
connection_handlers setter. This is called from the ActiveRecord
Railtie on boot and since most applications don't set this themselves
this will prevent the deprecation from being raised multiple times for a
test run or in development.

I've also updated the guides to include a migration path for
applications using the deprecated methods. The majority of applications
won't need to make any changes.

@@ -379,7 +381,9 @@ def test_retrieve_connection_pool_with_invalid_id
end

def test_connection_handlers_are_per_thread_and_not_per_fiber
ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler, reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new }
assert_deprecated do
ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler, reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler, reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new }
ActiveRecord::Base.connection_handlers = {
writing: ActiveRecord::Base.default_connection_handler,
reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new
}

@@ -392,7 +396,9 @@ def test_connection_handlers_are_per_thread_and_not_per_fiber
end

def test_connection_handlers_swapping_connections_in_fiber
ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler, reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new }
assert_deprecated do
ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler, reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new }
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ActiveRecord::Base.connection_handlers = { writing: ActiveRecord::Base.default_connection_handler, reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new }
ActiveRecord::Base.connection_handlers = {
writing: ActiveRecord::Base.default_connection_handler,
reading: ActiveRecord::ConnectionAdapters::ConnectionHandler.new
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestions. I'm didn't merge in the suggested changes because it's deprecated code, and they're just tests. 😄

This deprecates `legacy_connection_handling` via the
`connection_handlers` setter. This is called from the ActiveRecord
Railtie on boot and since most applications don't set this themselves
this will prevent the deprecation from being raised multiple times for a
test run or in development.

I've also updated the guides to include a migration path for
applications using the deprecated methods. The majority of applications
won't need to make any changes.
@eileencodes eileencodes force-pushed the deprecate-legacy-connection-handling branch from 4196be9 to 634bf89 Compare April 6, 2021 22:57
@eileencodes eileencodes merged commit 564a26d into rails:main Apr 6, 2021
@eileencodes eileencodes deleted the deprecate-legacy-connection-handling branch April 6, 2021 23:38
The new connection handling does not support `connection_handlers`
getter and setter.

Read more about how to migrate at:
Copy link
Contributor

Choose a reason for hiding this comment

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

Read more link is missing. Is it intentionally left blank until guide changes are released?

We can use https://guides.rubyonrails.org/active_record_multiple_databases.html#migrate-to-the-new-connection-handling considering after guide release the link would work fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, thanks for catching that. I pushed a fix in b8b1c9e

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.

None yet

3 participants