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 rails clear_active_connections! deprecation warning #208

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

Abdullah-l
Copy link
Contributor

What? Why?

Fixes #205

How was it tested?

Existing specs

@Abdullah-l Abdullah-l force-pushed the fix-deprecation-warning branch 2 times, most recently from a4b7a2e to 2b67781 Compare February 23, 2024 00:28
@jlambert121
Copy link

It'd be great to get this merged and released, anything I can do to help?

@Abdullah-l
Copy link
Contributor Author

@splittingred Can we get this PR reviewed? 🙏

@splittingred
Copy link
Member

@Abdullah-l can you confirm this works on:

  • Rails 6.1
  • Rails 7.0
  • Rails 7.1
  • A non-rails system using ActiveRecord?

This PR has some assumptions right now that connection_handler is going to exist, and do what we want. I think we're going to want proof it's interoperable with those versions of AR in those runtime environments.

@Abdullah-l
Copy link
Contributor Author

@Abdullah-l can you confirm this works on:

  • Rails 6.1
  • Rails 7.0
  • Rails 7.1
  • A non-rails system using ActiveRecord?

This PR has some assumptions right now that connection_handler is going to exist, and do what we want. I think we're going to want proof it's interoperable with those versions of AR in those runtime environments.

The proof is in active_record source code. connection_handler has always been available, so this change is compatible with any active_record version. The deprecation warning comes for this PR.

Copy link
Member

@splittingred splittingred left a comment

Choose a reason for hiding this comment

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

Great, thanks for doing the due diligence here. LGTM. 👍

@splittingred splittingred merged commit 08172e5 into bigcommerce:main Jul 22, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment