config icon indicating copy to clipboard operation
config copied to clipboard

Settings are loaded before `setup` block executed

Open supremebeing7 opened this issue 5 years ago • 19 comments

Version 2.0.0

I'm trying to use the use_env setting, but was noticing the override wasn't working. On closer inspection, it appears as if all of my settings are loaded prior to the Config.setup block being executed. I verified this by checking it in the initializer:

Config.setup do |config|
  config.const_name = 'Settings'
  config.use_env = true
  config.env_prefix = 'SETTINGS'
  config.env_separator = '__'
  config.env_converter = :downcase
  config.env_parse_values = true
end

puts ENV['SETTINGS__FOO']
# => baz

puts Settings.foo
# => bar

Settings.reload!

puts Settings.foo
# => baz

Has anyone else experienced this? I don't think it's due to any special setup for my particular app, but will try to spin up an example app to test.

supremebeing7 avatar Jul 25 '19 18:07 supremebeing7

Reproduced this in an example app https://github.com/supremebeing7/example_config That one is on rails 6.0.0.rc1, the other app I experienced this is on rails 5.2.3.

supremebeing7 avatar Jul 25 '19 19:07 supremebeing7

Worth mentioning also that the const_name override doesn't appear to work either, presumably for the same reason. The problem there is that it still won't work after Settings.reload! since that only reloads the settings and does not redefine any constants.

supremebeing7 avatar Jul 25 '19 19:07 supremebeing7

OK, I think there's two issues here:

  1. I renamed the initializer to 01_config.rb, but in the railtie it's loading the specific config.rb file"
          initializer = ::Rails.root.join('config', 'initializers', 'config.rb')
          require initializer if File.exist?(initializer)
  1. I did the above rename because when I tried to access the Settings const in the config initializer, it complained of uninitialized constant Settings (NameError). (Related to #187)

Once I noticed the railtie from [1] above, then [2] makes sense, since the Config.setup method gets called as part of Config.load_and_set_settings after the initializer is already required.

I'm not sure the best solution here...

I do think some additional documentation around this would be helpful.

However, I'm also wondering if maybe the config/initializers/config.rb should live just in config/config.rb? It seems very unintuitive to me to load/require an initializer in a railtie like that, though I admit I'm not super familiar with how similar projects load their settings. But I also think it is surprising not to allow using the settings constant defined in an initializer, as I assume once the initializer runs that Settings constant is defined. If the Config.setup block was not in an initializer but in config/config.rb, and we required it from there in the railtie, I think that would make more sense, and I could then add an initializer to do any manipulation to the settings (like adding sources at runtime, in the case of #187).

@pkuczynski What do you think? I'm happy to work on this, just unsure what solution makes the most sense. Also open to other ideas than the ones I laid out.

supremebeing7 avatar Jul 25 '19 20:07 supremebeing7

Yeah, the whole point of the initializer is to define configuration options for the Config before the instance is created. The reason it's done this way was, that you don't need an initializer at all if you follow default setup, as Config is plugging into the rails automatically here: https://github.com/railsconfig/config/blob/master/lib/config.rb#L81

Also see my comment in #187

Question is: why would you like to access Settings in initializer? Is there a real need for it?

Maybe we should update documentation. PR welcome :)

pkuczynski avatar Aug 06 '19 18:08 pkuczynski

Thanks for the clarification @pkuczynski.

you don't need an initializer at all if you follow default setup, as Config is plugging into the rails automatically

Makes sense! I think the default setup is great and wouldn't want to change how this works.

Question is: why would you like to access Settings in initializer? Is there a real need for it?

In my specific case, it was due to migrating from a different configuration gem where we had assigned config values to a constant APP_CONFIG. So, to avoid a messy migration path, in my initializer I just did APP_CONFIG = Settings.

I'll be the first to admit that's probably not a common use case. A perhaps more common use case might be the add_source approach taken in #187. However, I still think it's odd to have an initializer get loaded like that. In my mind, initializers are run in the order they're placed in the initializers directory, so to have the Config.setup block in config/initializers/config.rb loaded before any of the initializers was very surprising.

With the caveat that I'm not too familiar with how other projects like this are structured, to me it would make more sense to move the setup block to config/config.rb and load that in the railtie. Then there is no question of initializer load order.

Obviously this would have to wait for the next minor version at least since it's a breaking change, though we could implement both approaches now and show a deprecation warning for the initializer approach to help ease the transition.

I'm happy to work on this if that all seems reasonable. However if we think that's excessive or unnecessary and we just want to add more documentation around this, I can certainly do that instead.

supremebeing7 avatar Aug 07 '19 04:08 supremebeing7

As I mentioned the other day, I am mainly working on Node.js stack these days, so can't really say how modern app layout is done nowadays.

Adding some more explanation to the Rails section of the documentation makes perfect sense if you could do that? Should be enough for now, as nobody else complained.

@rdubya @pyromaniac what do you think about changing the behavior? I am personally not sure if that's worth the hassle...

pkuczynski avatar Aug 07 '19 17:08 pkuczynski

It does seem like it would make sense to move it out of the initializers directory. We reference Settings in other initializers but not in the config initializer.

rdubya avatar Aug 08 '19 01:08 rdubya

Then sure, we can work on this and release as 2.1. @supremebeing7 if you wanna take it over, go ahead.

pkuczynski avatar Aug 12 '19 08:08 pkuczynski

@supremebeing7 are u still interested in firing up a PR?

pkuczynski avatar Jan 08 '20 18:01 pkuczynski

@pkuczynski I'm interested but unfortunately don't have time currently. So if someone else wants to take a stab at it, they can. Otherwise we can push it to a later milestone and I can look into it then.

supremebeing7 avatar Jan 08 '20 18:01 supremebeing7

@supremebeing7 are you still interested in fixing this?

pkuczynski avatar Mar 25 '20 20:03 pkuczynski

@pkuczynski Interested, but I don't have time to do it. And I don't see that situation changing anytime soon.

supremebeing7 avatar Mar 26 '20 00:03 supremebeing7

That's a shame, but I totally understand. Will keep this open and ping you in some time :)

pkuczynski avatar Mar 26 '20 12:03 pkuczynski

@supremebeing7 how do you look with time now? :)

pkuczynski avatar Dec 08 '20 14:12 pkuczynski

@pkuczynski Still unavailable to work on this. FWIW: This is very low priority for me - after the initial setup, this issue has not come up again. So, unless someone else wants to pick this up, I'd be fine with closing it.

supremebeing7 avatar Dec 08 '20 16:12 supremebeing7

Got it! @cjlarose do you wanna have a look or shall we close it?

pkuczynski avatar Dec 08 '20 22:12 pkuczynski

I can take a look.

cjlarose avatar Dec 08 '20 23:12 cjlarose

Itu memalukan, tapi aku benar-benar mengerti. Akan membuat ini tetap terbuka dan melakukan ping kepada Anda dalam beberapa waktu :)

good job sir

Wildanar06 avatar Dec 10 '21 01:12 Wildanar06

Taking another look at this recently since https://github.com/rubyconfig/config/issues/352 is related. This issue brings up a couple of distinct, though related, points

  • In the default configuration for a Rails project, config/initializers/config.rb is executed twice on boot (once by the Railtie and once again when Rails loads all initializers).
  • In the default configuration for a Rails project, the Settings constant is not available in config/initializers/config.rb, though it is available in other initializers https://github.com/rubyconfig/config/issues/187
  • If the user goes with a different name for the initializer in a Rails project (such as 01_config.rb), things get especially weird, since while Settings is available in that initializer, it is populated by the call to load_and_set_settings as performed by the Railtie (and does not account for the user's own settings assuming they're calling Config.setup in the initializer).

I think the solution is still the same: let's move configuration for the config gem to config/config.rb (really, anywhere outsides of initializers). This is a breaking change, but I think it's probably worth it and the migration should be pretty easy for most users.

cjlarose avatar Mar 06 '24 07:03 cjlarose