Create extension methods to easily append to CollectionBuilders

[erikjanwestendorp]

Description

Add an extension method to register content apps to make it possible to register content apps in the startup.cs class.
No idea if this is desirable. But to me it seems like a nice addition to easily register content apps. Maybe also an idea to do the same for Components?

Method can be used like:

public void ConfigureServices(IServiceCollection services)
{

    services.AddUmbraco(_env, _config)
        .AddBackOffice()
        .AddWebsite()
        .AddComposers()
        .AddContentApp<WordCounterApp>()
        .Build();

}

[umbrabot]

Hi there @erikjanwestendorp, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[erikjanwestendorp]

After I made this PR, I thought maybe this could be done in a nicer way. Is the below an option that has a chance?

Rename the file
UmbracoBuilder.ContentApps to UmbracoBuilder.Collections

Add extensions methods for other collections, like:

public static partial class UmbracoBuilderExtensions
{
    public static IUmbracoBuilder AddContentApp<T>(this IUmbracoBuilder builder)
        where T : class, IContentAppFactory
    {
        builder.ContentApps().Append<T>();
        return builder;
    }

    public static IUmbracoBuilder AddDashboard<T>(this IUmbracoBuilder builder)
        where T : class, IDashboard
    {
        builder.Dashboards().Add<T>();
        return builder;
    }

    public static IUmbracoBuilder AddComponents<T>(this IUmbracoBuilder builder)
        where T : class, IComponent
    {
        builder.Components().Append<T>();
        return builder;
    }
}

[mikecp]

Hi @erikjanwestendorp,

Thanks for this PR! 👍

I think it's a great idea because it not only makes things more ".Net Core like", it also encapsulates the actual implementation, so if for some reason we change it later on, we can just adapt the extensions methods and people that used them won't need to change anything in their code 😁🎉

I don't see any problems in following your second suggestion and having the extensions for the different collections.

Cheers!

[erikjanwestendorp]

Hi @mikecp

Thanks for your review! 😄 I just updated the PR and added the methods below:

• AddComponent
• AddContentApp
• AddContentFinder
• AddDashboard
• AddMediaUrlProvider
• AddOEmbedProvider
• AddSection
• AddUrlProvider

Would you like to review this?

[mikecp]

Hey @erikjanwestendorp ,

I just reviewed it and tested locally, it all works fine 👍
Just a small remark/question, the method to add an embed provider is named AddOEmbedProvider, I guess it's a typo and it should be AddEmbedProvider (without the capital O) instead ?😁

If you could make that small change, then it will be ready to be merged!

[erikjanwestendorp]

Hi @mikecp

Thanks for your review again! 👍

It wasn't actually a typo. I have followed the naming convention of, for example:

Umbraco-CMS/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.Collections.cs

Line 105 in 004d341

builder.OEmbedProviders()

I totally agree that it would be nicer to use EmbedProvider instead of OEmbedProvider. 😄 In my last commit I changed it 👍

[mikecp]

Hi @erikjanwestendorp ,

Woopsie, I totally missed the fact that the Umbraco collection itself was also called OEmbedProviders 🤦‍♂️

Then I guess it's best to follow the current naming convention the whole way, the O probably stands for "Object" or something.

Would you mind getting back to your previous naming? Sorry about that😬

[erikjanwestendorp]

@mikecp Follow the current naming convention sounds good to me 😄! No problem of course! 👍 I renamed it again 😄

[mikecp]

Thanks @erikjanwestendorp 👍😀

[ronaldbarendse]

the O probably stands for "Object" or something

Most (if not all) of these providers use OEmbed to retrieve the HTML to embed, so it's actually using an implementation specific name. But then the actual implementation base class EmbedProviderBase uses a generic name again 😞

I would recommend changing the following (in a new PR, which needs to be merged before releasing 9.2.0):

• Rename the extension method to AddEmbedProvider() again;
• Obsolete OEmbedProviders() and proxy it to a new EmbedProviders() method with the existing body:

  Umbraco-CMS/src/Umbraco.Core/DependencyInjection/UmbracoBuilder.Collections.cs

  Lines 268 to 269 in 004d341

  	public static EmbedProvidersCollectionBuilder OEmbedProviders(this IUmbracoBuilder builder)
  	=> builder.WithCollectionBuilder<EmbedProvidersCollectionBuilder>();

• Optionally do something similar (but in 'reverse') for EmbedProviderBase:
  • Rename the existing class to OEmbedProviderBase;
  • Add an obsoleted class public abstract class EmbedProviderBase : OEmbedProviderBase;
  • Add // TODO (V10): comments to all implementations to change their base class to OEmbedProviderBase in the next major version (changing this will break the inheritance chain, making it a breaking change).

[mikecp]

Thanks @ronaldbarendse for the feedback, I think @erikjanwestendorp is gonna have nightmares about this renaming 😂

@erikjanwestendorp feeling like taking this into a new PR 😁?

[erikjanwestendorp]

@ronaldbarendse Thanks for your feedback! 😄. @mikecp nightmares indeed about renaiming this 😂.

@ronaldbarendse @mikecp working on it #11703

[abjerner]

What was the conclusion for the naming?

I think the O stands for Open as OEmbed is described as an open standard. I think some of the purpose is lost when switching to EmbedProviders instead of OEmbedProviders as done in #11703.

It's specifically providers for and following the OEmbed standard, so I think it would be wrong to rename the method to just EmbedProviders.

[ronaldbarendse]

@erikjanwestendorp Awesome work and thanks for getting it done this fast!

@abjerner True, the current IEmbedProvider still includes OEmbed specific details... OEmbed isn't the only way to get the embed markup though, as some platforms provide other endpoints, use OpenGraph, etc. Some are now even depreciating their OEmbed endpoints, so I think it makes sense not to tightly couple Umbraco's implementation to this specific standard...