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 activerecord 7.1 and 7.2 deprecations #423

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

tomgi
Copy link
Contributor

@tomgi tomgi commented Mar 12, 2024

Similar to #407, but fixes 2 related deprecation warnings and adds backward-compatibility with older version of rails.

On rails 7.1 these are deprecation warnings.

On rails 7.2 the second one is not a warning anymore but a crash with undefined method clear_active_connections!'


Fixes the following:

DEPRECATION WARNING: Calling ActiveRecord::Base.clear_active_connections! is deprecated. Please call the method directly on the connection handler; for example: ActiveRecord::Base.connection_handler.clear_active_connections!`. (called from call at /app/.bundle/ruby/3.2.0/bundler/gems/shoryuken-869fb0c77f8e/lib/shoryuken/middleware/server/active_record.rb:8)

DEPRECATION WARNING: clear_active_connections! currently only applies to connection pools in the current role (writing). In Rails 7.2, this method will apply to all known pools, regardless of role. To affect only those connections belonging to a specific role, pass the role name as an argument. To switch to the new behavior, pass :all as the role name. (called from call at /app/.bundle/ruby/3.2.0/bundler/gems/shoryuken-869fb0c77f8e/lib/shoryuken/middleware/server/active_record.rb:8)

@nfedyashev
Copy link

This one would be nice to get merged

@peterdavidhamilton
Copy link

💯

@owst
Copy link
Contributor

owst commented Jul 10, 2024

@ZimbiX can we get this one in too please?

@reneeckstein
Copy link

Do you have any updates on this? It currently blocks us from upgrading to Rails 7.2.x

@jdowd
Copy link

jdowd commented Aug 27, 2024

FYI rails 7.2 seems to be working for us with this change to application.rb:

  config.load_defaults 7.1

@erojasb
Copy link

erojasb commented Oct 17, 2024

👍🏽 💯

@Tabby
Copy link

Tabby commented Oct 18, 2024

Would it help to add Rails 7.2 to the test matrix to give more confidence that this does work correctly with Rails 7.2?
Would also need to add a Gemfile-rails-7.2 file in spec/gemfiles

@tomgi tomgi mentioned this pull request Oct 28, 2024
@tomgi
Copy link
Contributor Author

tomgi commented Oct 28, 2024

I've added Rails 7.2 to the test matrix.

The tests fail as expected against current master code #432, but succeed with the changes introduced in this branch.

@ZimbiX can we please get this PR merged? Without it, que doesn't work with Rails 7.2.

@oeoeaio
Copy link
Contributor

oeoeaio commented Oct 28, 2024

@tomgi can you clean up the commits for this PR? There is a WIP and two merge commits which shouldn't be there.

@tomgi
Copy link
Contributor Author

tomgi commented Oct 28, 2024

@tomgi can you clean up the commits for this PR? There is a WIP and two merge commits which shouldn't be there.

@oeoeaio I want these commits to be here 🙂
They are coming from merging #432 to show that the new tests are passing on this branch, but failing on master.

They can be all squashed into a single commit when merging this PR to master to keep the master branch linear and clean, but I think it makes sense to have them as-is in this PR.

@oeoeaio
Copy link
Contributor

oeoeaio commented Oct 28, 2024

@tomgi I'm not sure I follow your logic. As a reviewer, I want to be able to follow your development of a change to fix an issue or add a new feature. That becomes very difficult if I can't understand what each commit is doing. You've got six commits here, that really should just be one or maybe two.

I can see from #432 that there is an issue, you don't need to prove that here as well.

@tomgi tomgi force-pushed the activerecord_deprecation_warning branch from 6b21a6c to 328843a Compare October 28, 2024 03:35
@tomgi
Copy link
Contributor Author

tomgi commented Oct 28, 2024

@oeoeaio I see your point, I cleaned the WIP commit.

But I also think it's important to show that the Rails 7.2 tests are failing on master and passing on this branch.
#432 proves that there is an issue, merging it into this PR proves that this PR fixes that issue.
That's why I would prefer keeping merged #432 here.

@oeoeaio
Copy link
Contributor

oeoeaio commented Oct 28, 2024

@tomgi I don't need to you show that, I can clearly see that there is an issue. If you really desperately want to show that for posterity, then please make ce869a7 the first commit to demonstrate the issue, and then add a second commit (5b1ff2c and dcdbe94 squashed together) to fix the issue.

@tomgi tomgi force-pushed the activerecord_deprecation_warning branch from 328843a to a59fe7f Compare October 28, 2024 03:48
@tomgi tomgi changed the title Fix activerecord 7.1 deprecation warnings Fix activerecord 7.1 deprecation warnings and 7.2 crash Oct 28, 2024
@tomgi
Copy link
Contributor Author

tomgi commented Oct 28, 2024

@oeoeaio thanks, done

@tomgi tomgi changed the title Fix activerecord 7.1 deprecation warnings and 7.2 crash Fix activerecord 7.1 and 7.2 deprecations Oct 28, 2024
@tomgi tomgi force-pushed the activerecord_deprecation_warning branch from a59fe7f to c4dead8 Compare October 28, 2024 03:54
@oeoeaio oeoeaio merged commit f87b462 into que-rb:master Oct 28, 2024
23 checks passed
@ZimbiX
Copy link
Member

ZimbiX commented Oct 28, 2024

Thanks, @tomgi! And thanks for reviewing, @oeoeaio =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants