umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
Other
4.49k stars 2.69k forks source link

Refactor configuration to take advantage of Options Pattern #8651

Closed JimBobSquarePants closed 4 years ago

JimBobSquarePants commented 4 years ago

Current .NET Core configuration utilizes a non-conventional pattern to load configuration values.

https://github.com/umbraco/Umbraco-CMS/blob/43efee9647ed8f2eaab3ec2b71d3dd3eb46b9017/src/Umbraco.Configuration/AspNetCoreConfigsFactory.cs

ASP.NET Core already has an established pattern to load options. https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/options?view=aspnetcore-3.1

There are several advantages to using this pattern.

  1. It's common convention. No surprises for future contributors.
  2. It allows options monitoring and dynamic reloading when required.
  3. It negates the requirement for each superfluous interface definition and implementation.

Binding a section becomes trivial.

services.Configure<MyOptions>(Configuration.GetSection(nameof(MyOptions)));

Consuming it equally so.

public class MyOptionsConsumer(IOptions<MyOptions> options)
{
    Guard.NotNull(options, nameof(options));

    this.options = options.Value;    
}

Unit testing again is trivial.

var myOptions = new MyOptions();
var myOptionsConsumer = new MyOptionsConsumer(Options.Create(myOptions));
bergmania commented 4 years ago

Hi @JimBobSquarePants..

Overall seems like a good idea. The only thing I don't like is if we have to know the IOptions<T> all over our own codebase.
I'm sure there is a way to get the best of both worlds.
E.g https://www.strathweb.com/2016/09/strongly-typed-configuration-in-asp-net-core-without-ioptionst/

JimBobSquarePants commented 4 years ago

I think that post is a little naïve and doesn't take into account any of the the highlighted benefits of IOptions<T> and co.

Having IOptions<T> passed to your constructors isn't a bad thing. As I noted before.

It's common convention. No surprises for future contributors.

bergmania commented 4 years ago

We decide to allow the MS Config interfaces in Umbraco.Core. @AndyButland already started a draft PR. Good start, and we can continue on work.

I think we should make a netcore/config branch as the base branch for all these smaller config PRs, so we can merge these into netcore/netcore as one big PR, but review it as several smaller PRs.

umbrabot commented 4 years ago

Hi @JimBobSquarePants,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly Umbraco GitHub bot :-)

AndyButland commented 4 years ago

Can we/should we close this now @bergmania? Seems that any further uses of IOptions will come in as and when new features are migrated, rather than being a separate task to add.