Netcore: Remove internal composers

[p-m-j]

Further enhancements to simplify the service registration story continuing on from #8653

We no longer require IComposer implementations in the Umbraco codebase, however the concept must continue to exist for plugin developers such that they can continue to support no-code installations.

Those with the ability to edit (or replace) Startup.cs should prefer Startup.ConfigureServices & extensions on IUmbracoBuilder to register their services over custom composer implementations.

The following tasks have been constructed based on discussion in #8653

• Rename IComposer to IUmbracoComposer to boost search engine results.
• Remove CoreComposer, UserComposer, there should exist only IUmbracoComposer
• All internal implementations of IComposer should be replaced with extensions on IUmbracoBuilder
• Remove Composers.cs, we don't require ComposeAfter or DisableComposer attributes, we will run all IUmbracoComposer instances in the order they are discovered.

Ordering of UmbracoBuilder extensions for adding core service shouldn't be important, why would Umbraco core replace its own registrations.

However, there may be some legacy cruft here so keep an eye out for duplicate registrations of services in the existing composers.

[JimBobSquarePants]

Add collection of Actions on UmbracoBuilder so a composer has a final chance to replace services after other composers run (we do not need to worry about what happens if two composers fight over late registration, this isn't supported in 8 and wont be in 9 either).

I think this is something someone can do from startup if they require directly against the builder there. If we do what I propose re collections there will be no complexity whatsoever.

[p-m-j]

I think this is something someone can do from startup if they require directly against the builder there. If we do what I propose re collections there will be no complexity whatsoever.

This is the workaround for no code late reg, should only be used by plugin devs.

@Shazwazza what was the point of this again?

Why would Our.Umbraco.Bar want to overrule Our.Umbraco.Foo on a registration, and what happens when Our.Umbraco.Foo pushes an update to use the late reg.

Why would we handle the 1st case but not the nth case, perhaps better to not worry about it at all.

[JimBobSquarePants]

This is the workaround for no code late reg, should only be used by plugin devs.

I thought we'd already established that this was an edge case not worth the effort and that if someone explicitly wanted to do that they use startup?

Plugin devs won't be doing stuff like that anyway. Most plugins are property value converters.

[p-m-j]

The last said on the matter was #8653 (comment)

Most concrete examples are package running things after certain core components but if all core services are registered ourselves before any package components are run then this scenario doesn't ever occur. I don't have a concrete example of a package that relies on another package using ComposeAfter but i'm sure it exists someplace, but like I said above, that would be solved in a simpler way by defining a callback during composition like the OnContainerBuilding example.

[JimBobSquarePants]

I really don't think it's worth the effort.

[Shazwazza]

I thought of an example: an Umbraco Forms addon packages. Umbraco Forms itself is a package and there's a possibility that an addon package author would want to replace a service shipped with Umbraco Forms. Another similar example could be a Vendr (https://vendr.net/) addon package that might want to replace a service shipped with that ecom engine.

The OnContainerBuilder example was just an idea to potentially assist with this edge case and I know that this doesn't 'solve' the case at all if 2x addons are trying to replace the same service. Its the same issue there is today with before/after thing. The only real solution is the end user calling a method in their startup.

So...

perhaps better to not worry about it at all.

Agree :)

I really don't think it's worth the effort.

Agree :)

[lars-erik]

Just leaving a few open ended cents again.

I had a chat with @callumbwhyte since I know he's done quite a few things where [ComposeAfter] has been necessary to ensure that ExamineManager has initialized and prepped the indexers. A search after ComposeAfter and ComposeBefore yields ~100 files from substantially less projects that use them pretty much in the same way.

I'm thinking that most, if not all of those cases, are due to legacy framework stuff and basically just not DI-compatible architecture in both Umbraco and the plugins. Why shouldn't ExamineManager already be ready if it's ready to be injected? It could at least lazy initialize instead of having a temporal coupling to its component. Or be initialized in a factory method.

The big question then ends up "do we force everyone to change architecture in order to make Umbraco startup totally non temporal for plugins?"

On a sidenote, interception and decoration would pretty much solve all of this. The only showstopper is if two plugins decorate the same concrete class, then the implementor would be forced to actually modify the composition during startup.

[JimBobSquarePants]

The big question then ends up "do we force everyone to change architecture in order to make Umbraco startup totally non temporal for plugins?"

YES!!

[p-m-j]

Examine is in core, so its Component (assuming they survive) would always init before an IUmbracoComposer added component.

I'm not sure if there's an issue here @lars-erik?

---

Only remaining concern is a package replacing uniques added by another package, e.g Shannons example of someone extending Umbraco.Forms but the current consensus is it's not worth worrying about.

Core stuff should always be ready in time for downstream stuff.

[Shazwazza]

Yes, the only reason people have to hack things with ComposeAfter - especially around things that are shipped in core - is just due to it not being initialized explicitly like it should be and instead treating core things as plugins.

The big question then ends up "do we force everyone to change architecture in order to make Umbraco startup totally non temporal for plugins?"

YES I agree. Though it's still not going to be a huge change and will be less confusing.

Also I've seen a number of people doing ComposerAfter with Examine only because they have copy/pasted things that they shouldn't have. There should be no reason for it whatsoever if people are just using the default IUserComposer.

Core stuff should always be ready in time for downstream stuff.

indeed - essentially it would be like today's IUserComposer.

There are 2 strange things I've just remembered though. This is just something we need to fix/review internally but essentially there is some core code that currently needs to execute after all plugin components and user startup.

• ExamineFinalComposer/ExamineFinalComponent
  • The Examine thing ensures indexes are unlocked before the web app starts but after all Examine configuration takes place. So this would need to execute after normal user's Startup code. But I think that's best handled as part of the UmbracoBuilder and we have a method/callback where the user can add code to customize Examine.
• WebFinalComposer/WebFinalComponent
  • The Web thing is an old webapi thing, i think it can just be deleted.

[p-m-j]

Had a first hack at this, starts to become a problem at ModelsBuilderComposer (Models builder depends on Umbraco.Web.BackOffice, but BackOffice would be registering once composers are gone)

I can yank AddAllBackOfficeComponents and AddUmbracoCore upto Umbraco.Web.UI.NetCore and everything would work, anyone got any better ideas?

[JimBobSquarePants]

ExamineFinalComposer/ExamineFinalComponent
The Examine thing ensures indexes are unlocked before the web app starts but after all Examine configuration takes place. So this would need to execute after normal user's Startup code. But I think that's best handled as part of the UmbracoBuilder and we have a method/callback where the user can add code to customize Examine.

Yep. Examine constructor injects IEventAggregator. Examine invokes it passing an OnExamineStarting type or similar.

Examine should have it's own events.

[p-m-j]

That’s presumably the way you solve it in the other ticket about removing static events.

For now in this issue you would just call builder.Components.append<examine final> after builder.AddComposers.

[JimBobSquarePants]

Are we doing these in the right order? It seems to me the code will be fixed twice?

[p-m-j]

I think that’s a reasonable argument, don’t need to move component registrations when components no longer exist.

However whilst the other issue has a couple of thumbs up, I don’t feel like the strategy is set in stone or approved so this one feels like a quicker win.

[JimBobSquarePants]

Rolls up sleeves 👷‍♂️

[bergmania]

All internal composers are now removed...

We need to be aware when we merge stuff from v8, so new ones are not introduced.