OrchardCMS / OrchardCore

Orchard Core is an open-source modular and multi-tenant application framework built with ASP.NET Core, and a content management system (CMS) built on top of that framework.
https://orchardcore.net
BSD 3-Clause "New" or "Revised" License
7.43k stars 2.4k forks source link

configuration stored in appsettings.json is not taken into account #15310

Closed lampersky closed 7 months ago

lampersky commented 8 months ago

I've noticed that configuration stored in appsettings.json is not binded in many places:

It looks like somebody created some extensions methods, but they were not used, other than that they were wrong in my opinion.

Steps to reproduces:

rjpowers10 commented 8 months ago

I'm not the original author, but my interpretation is those extension methods are intentionally not called in base Orchard Core code. They are provided as a way for consumers of OC to override the DB settings with configuration settings, if desired.

Here is the docs for email

I'm using this override for email settings myself. And one thing I noticed is the configuration values do override the DB settings (as intended), but the admin UI does not reflect these overrides, so it can be a little confusing. Which is perhaps part of the reason these are provided as optional overrides.

lampersky commented 8 months ago

I'm not the original author, but my interpretation is those extension methods are intentionally not called in base Orchard Core code. They are provided as a way for consumers of OC to override the DB settings with configuration settings, if desired.

It makes sense thank you @rjpowers10, but in my opinion it would be better if those settings from appsettings.json were always taken into account when present in the file (or in env variables).

For example If I will configure Facebook (via portal azure) I still need to deploy new version of the APP with additional call in Startup.cs to one of those extension methods ConfigureFacebookSettings. Which could be avoided.

image

ns8482e commented 8 months ago

@lampersky It's by design - By default in orchard cms setting are configurable in UI.

If you would like to use appsettings instead of UI then it's opt-in feature, you have to call ConfigureFacebookSettings explicitly in your Program.cs/Startup.cs as it's documented here https://docs.orchardcore.net/en/latest/docs/reference/modules/Facebook/#meta-settings-configuration

To opt-in to use appsettings change your program.cs as following eg.

builder.Services
    .AddOrchardCms()
    .ConfigureFacebookSettings()
Piedone commented 8 months ago

Yep, this works like that by design, as others have elaborated.

Piedone commented 8 months ago

That being said, we can change the design.

The settings coming from the configuration providers should already only apply if you have configurations available (do they only apply then?). So, it's opt-in. Currently, we support the scenario of having configuration but not wanting to use it, which seems like at most an edge case.

There can be some performance impact on the app start. However, at that point the DB configurations could be optional as well, since those also have a perhaps even larger impact.

ns8482e commented 8 months ago

Sure we can change the design, as if that adds flexibility and new features.

However, currently when developer can use Orchard CMS target as nuget package reference in the webapp - that allows them to customize Program.cs and opt-in for behavior asked in the issue - is already supported scenario without needing to have change in codebase.

lampersky commented 8 months ago

I have to admit something to you guys, I've never ventured into that part of the documentation :-P

I mistakenly assumed that it should work like for other configurable modules: OrchardCore_Apis_GraphQL, OrchardCore_Media_Azure and many others. The configuration from the appsettings.json file is taken into account automatically, you do not need to activate it additionally by calling any dedicated extension methods.

So, thank you @ns8482e, @Piedone and sorry for getting you involved unnecessarily. Maybe I was too quick into preparing the fix. If you guys will decide to reject the PR, let me know and I will extract this part which solves #14050 into another PR.

sebastienros commented 7 months ago

Should we close the PR and add more documentation about the behavior instead?

Piedone commented 7 months ago

I'm OK with that. @lampersky would you be so kind to improve the docs instead?