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

new_framework_defaults_7_1.rb are not working as expected #46567

Closed
fatkodima opened this issue Nov 24, 2022 · 11 comments
Closed

new_framework_defaults_7_1.rb are not working as expected #46567

fatkodima opened this issue Nov 24, 2022 · 11 comments
Labels

Comments

@fatkodima
Copy link
Member

Steps to reproduce

  1. Create a new (< 7.1) rails app
  2. Use config.load_defaults 7.0 in application.rb
  3. Add https://github.com/rails/rails/blob/main/railties/lib/rails/generators/rails/app/templates/config/initializers/new_framework_defaults_7_1.rb.tt to initializers and uncomment commented lines

Expected behavior

For example, in console

ActiveRecord.allow_deprecated_singular_associations_name # => false

Actual behavior

ActiveRecord.allow_deprecated_singular_associations_name # => true

Even though there is a test case for this -

test "ActiveRecord.allow_deprecated_singular_associations_name can be configured via config.active_record.allow_deprecated_singular_associations_name" do
remove_from_config '.*config\.load_defaults.*\n'
app_file "config/initializers/new_framework_defaults_7_1.rb", <<-RUBY
Rails.application.config.active_record.allow_deprecated_singular_associations_name = false
RUBY
app "development"
assert_equal false, ActiveRecord.allow_deprecated_singular_associations_name
end

The same situation applies for many of the configs in the new_framework_defaults_7_1.rb.tt file.
Am I doing something wrong?

System configuration

Rails version: main

@rafaelfranca
Copy link
Member

This usually happen because something in the application is loading ActiveRecord::Base too early. Did you observe this behavior really in a new generated application or did you added any gem after it?

@fatkodima
Copy link
Member Author

Did you observe this behavior really in a new generated application

Rechecked within a completely new app - works as expected.

I originally checked within the existing app. I was able to identify 2 gems that load ActiveRecord::Base too early. Without them - also works as expected.

I am wondering if reassigning a local variable configs here -

configs = configs.except(
, which is used then here -
configs.each do |k, v|
when ActiveRecord::Base is loaded earlier, is intentional? Because without that mutation everything works as expected with that 2 gems.

@mrhead
Copy link
Contributor

mrhead commented Nov 25, 2022

Unfortunately, this is a common issue caused by different gems. Another example: rollbar/rollbar-gem#906

I wish Rails would automatically identify issues like this, but I'm unsure how to implement it.

@ghiculescu
Copy link
Member

ghiculescu commented Nov 25, 2022

I wish Rails would automatically identify issues like this, but I'm unsure how to implement it.

There's a few ideas floating around. I think my favourite is #45297

@fatkodima are you able to share which gems had issues? Ideally they get patched.

Because without that mutation everything works as expected with that 2 gems.

Actually that reminds me of #46278 - another one caused by a third party issue.

@fatkodima
Copy link
Member Author

One is https://github.com/ledermann/unread, another one is mine - https://github.com/fatkodima/data_checks 😄

Actually that reminds me of #46278 - another one caused by a third party issue.

Ah, yes, haven't seen that. Had exactly the same idea.

@ghiculescu
Copy link
Member

I think this is probably what you want the architecture to look like in your gem. But I agree that it would be better if Rails could handle this in more cases.

@fatkodima
Copy link
Member Author

Yes, thanks for looking into this. I will definitely fix my gem and probably send a PR into unread (if no one will outpace me).
I defer loading of Active Record in my other gem with a similar technique you mentioned 👍.

@fatkodima
Copy link
Member Author

Opened a PR in unread and released a new version of data_checks with the fix.
So feel free to close, if no more actions can be taken.

@ghiculescu
Copy link
Member

@byroot @rafaelfranca do you think we will make any big changes for this in Rails 7.1?

In any case I will close this particular issue as resolved.

benoittgt added a commit to benoittgt/database_cleaner-active_record that referenced this issue Nov 23, 2023
This is well known issue. We should prefer to code that reference active
record only when it's already lazy loaded.
It can break other gems initializing process

Related:
- rails/rails#46567
- paper-trail-gem/paper_trail@fc6c5f6
(inspiration)
- thoughtbot/factory_bot_rails#432

Fix: DatabaseCleaner#89
benoittgt added a commit to benoittgt/database_cleaner-active_record that referenced this issue Nov 23, 2023
This is well known issue. We should prefer to code that reference active
record only when it's already lazy loaded.
It can break other gems initializing process

Related:
- rails/rails#46567
- paper-trail-gem/paper_trail@fc6c5f6
(inspiration)
- thoughtbot/factory_bot_rails#432

Fix: DatabaseCleaner#89
@pbstriker38
Copy link
Contributor

Here's another gem that loads ActiveRecord too early. It seems like they don't want to accept my PR unless I add a way to test it. Any ideas on a good way to test it?

flippercloud/flipper#832

@byroot
Copy link
Member

byroot commented Mar 28, 2024

That's not really testable unless you have something like our Railties test that generate and boot an application. But it's very slow and doubt flipper will want just 20 seconds test.

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

No branches or pull requests

6 participants