OrchardCore icon indicating copy to clipboard operation
OrchardCore copied to clipboard

Move ISlugService to OrchardCore.Autoroute.Core

Open Skrypt opened this issue 2 years ago • 13 comments

Introduces new assembly OrchardCore.Autoroute.Core.

I moved some code into this assembly to clean up the OrchardCore.Autoroute module. SlugService is now registered as an application-level singleton and it is moved to the OrchardCore assembly directly along with the LocalClock service for example.

Skrypt avatar Apr 04 '22 16:04 Skrypt

Ok, this breaks because the ISlugService is not registered before doing AddAntiForgery(). Also, probably need to be registered like the ShellSettings are.

Skrypt avatar Apr 04 '22 17:04 Skrypt

I quote from the title

Move ISlugService to OrchardCore.Autoroute.Core

I think ISlugService is better to be places in the abstractions not the Core

hishamco avatar Apr 04 '22 20:04 hishamco

That's what I did in the next commit. There is still an issue with registering the service at the right place. I tried some places without success. Will take a second look later.

Skrypt avatar Apr 04 '22 20:04 Skrypt

@Skrypt

Ok, this breaks because the ISlugService is not registered before doing AddAntiForgery()

Yes we are in a builder.ConfigureServices() so while building a tenant container, so it would have to be an app level singleton as IHostEnvironment, the tenant singleton ShellSettings is a special case, we eagerly register it in the pre-built container we use to resolve module startups an then run their ConfigureServices().

So would need to be an app level singleton registered for example in our AddDefaultServices, so defined in OrchardCore and the interface in OC.Abstractions, OR don't change anything and just make a static Slug component/helper that does the same and that could be used here, maybe the default ISlugSrvice could use this static component (but not necessarily), as I did at some point for the IdGenerator in OC.Abstractions.

jtkech avatar Apr 08 '22 22:04 jtkech

I tried in the AddDefaultServices() without success. Maybe it was not a singleton though. I will try again.

Skrypt avatar Apr 09 '22 02:04 Skrypt

Thanks @jtkech it works now as a singleton. I tested the Liquid filter on a Liquid page and it works so far.

Skrypt avatar Apr 09 '22 04:04 Skrypt

Removing the "breaking change" label. It will only break modules that were using the SlugService in the OrchardCore.Liquid module. Marked the class as obsolete for now.

Skrypt avatar Apr 09 '22 04:04 Skrypt

I would have just created a new static component helper in OC.Abstractions for what you need, OR if you still want to always inject ISlugService, just move the implementation from OC.Liquid to OrchardCore and the interface definition to OC.Abstractions, not in OC.Autoroute.***.

jtkech avatar Apr 09 '22 05:04 jtkech

Ok, now the SlugService is registered only as an Application level singleton and it works for all OC modules. Removed the OrchardCore.Autoroute.Abstraction project. Moved the ISlugService to OrchardCore.Abstractions.

I think it makes sense that we register this as an Application level singleton. It doesn't need to be a scoped service like it was before.

Skrypt avatar Apr 09 '22 14:04 Skrypt

Okay waiting on @sebastienros approval on this one.

Skrypt avatar Apr 11 '22 16:04 Skrypt

I am ok to look at the PR but not with the intent of fixing the linked issue, it's not the correct solution IMO.

sebastienros avatar Apr 12 '22 19:04 sebastienros

Conflicts to solve.

agriffard avatar Jun 04 '22 12:06 agriffard

Resolve the conflict

hishamco avatar Sep 14 '22 06:09 hishamco

2 approvals. I'm leaving someone else to merge this one. I fixed the remaining conflicts.

Skrypt avatar Sep 22 '22 20:09 Skrypt

Oops, don't merge yet. Unit tests failing, that's a new one.

Skrypt avatar Sep 22 '22 20:09 Skrypt

Done.

Skrypt avatar Sep 22 '22 20:09 Skrypt

Fixed build again.

Skrypt avatar Oct 08 '22 17:10 Skrypt