geocoder icon indicating copy to clipboard operation
geocoder copied to clipboard

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

Open mvastola opened this issue 4 years ago • 4 comments

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 5d92d0b5a423e0a061d15049b7011631914465d1 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

mvastola avatar Mar 08 '21 21:03 mvastola

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?

alexreisner avatar Mar 09 '21 19:03 alexreisner

@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.

mvastola avatar Mar 10 '21 18:03 mvastola

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).

alexreisner avatar Mar 12 '21 17:03 alexreisner

No rush.

mvastola avatar Mar 16 '21 16:03 mvastola