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

Settings are loaded before setup block executed #244

Open
supremebeing7 opened this issue Jul 25, 2019 · 19 comments
Open

Settings are loaded before setup block executed #244

supremebeing7 opened this issue Jul 25, 2019 · 19 comments
Assignees
Milestone

Comments

@supremebeing7
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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 Settings const name is not available right after Config.setup #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.

@pkuczynski
Copy link
Member

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

@supremebeing7
Copy link
Contributor Author

supremebeing7 commented Aug 7, 2019

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.

@pkuczynski
Copy link
Member

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

@rdubya
Copy link
Contributor

rdubya commented Aug 8, 2019

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.

@pkuczynski
Copy link
Member

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

@pkuczynski pkuczynski modified the milestones: 2.1.0, 2.2.0, 2.2.1, 2.3.0 Jan 3, 2020
@pkuczynski
Copy link
Member

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

@supremebeing7
Copy link
Contributor Author

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

@pkuczynski
Copy link
Member

@supremebeing7 are you still interested in fixing this?

@supremebeing7
Copy link
Contributor Author

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

@pkuczynski
Copy link
Member

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

@pkuczynski
Copy link
Member

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

@supremebeing7
Copy link
Contributor Author

@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 supremebeing7 removed their assignment Dec 8, 2020
@pkuczynski
Copy link
Member

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

@cjlarose
Copy link
Member

cjlarose commented Dec 8, 2020

I can take a look.

@cjlarose cjlarose self-assigned this Dec 8, 2020
@Wildanar06
Copy link

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

good job sir

@pkuczynski pkuczynski removed this from the 4.0.0 milestone Jun 1, 2022
@cjlarose
Copy link
Member

cjlarose commented Mar 6, 2024

Taking another look at this recently since #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 Settings const name is not available right after Config.setup #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.

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

No branches or pull requests

5 participants