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

Injecting Geocoder into AR::Base before config is loaded causes settings to be ignored #1493

Open
mvastola opened this issue Mar 8, 2021 · 4 comments

Comments

@mvastola
Copy link

mvastola commented Mar 8, 2021

Expected behavior

If I set Rails.application.config.active_record.belongs_to_required_by_default = false in an initializer, I should be able to run rails c and get the following output:

[2] pry(main)> ActiveRecord::Base.belongs_to_required_by_default
=> false

Actual behavior

true is returned

Steps to reproduce

Would seem to be as simple as adding geocoder to a rails app and adding the setting above in an initializer.

Cause

So this was a weird one to track down, because we had no idea what was causing the setting to get nuked. Per an issue raised in the rails project, it seems that ActiveRecord::Base looks up and sets its configuration when it is first loaded. However, in commit 5d92d0b to this project, ActiveRecord::Base is caused to load earlier than it is supposed to be (namely, before: :load_config_initializers) so the app's configuration gets silently ignored.

Environment info

  • Geocoder version: 1.6.5
  • Rails version: 6.1
@alexreisner
Copy link
Owner

Thanks for reporting this. Just to clarify, and to make sure I understand the issue: in your example, wouldn't you expect ActiveRecord::Base.belongs_to_required_by_default to return false?

@mvastola
Copy link
Author

@alexreisner , oh, I'm sorry, I screwed this up. Actual behavior was it returns true. This is because it's the rails default and my change to it didn't take effect. I do expect it to return false, but it doesn't.

@alexreisner
Copy link
Owner

alexreisner commented Mar 12, 2021

No worries. I just wanted to make sure we're on the same page (thanks for updating the issue). It may take me a couple of days to find time to investigate this, but I wanted to let you know I'm on it, and my initial instinct is that you're right. The solution ideally won't break whatever that commit was trying to fix (which I don't recall at the moment).

@mvastola
Copy link
Author

No rush.

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

No branches or pull requests

2 participants