Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move ISlugService to OrchardCore.Autoroute.Core #11491

Merged
merged 24 commits into from Oct 12, 2022
Merged

Move ISlugService to OrchardCore.Autoroute.Core #11491

merged 24 commits into from Oct 12, 2022

Conversation

Skrypt
Copy link
Contributor

@Skrypt Skrypt commented Apr 4, 2022

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
Copy link
Contributor Author

Skrypt commented Apr 4, 2022

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

@hishamco
Copy link
Member

hishamco commented Apr 4, 2022

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

@Skrypt
Copy link
Contributor Author

Skrypt commented Apr 4, 2022

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 Skrypt added the breaking change 💥 Issues or pull requests that introduces breaking change(s) label Apr 5, 2022
@Skrypt Skrypt changed the title Move ISlugService to OrchardCore.Autoroute.Core Move ISlugService to OrchardCore.Autoroute.Abstractions Apr 5, 2022
@jtkech
Copy link
Member

jtkech commented Apr 8, 2022

@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.

@Skrypt
Copy link
Contributor Author

Skrypt commented Apr 9, 2022

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

@Skrypt
Copy link
Contributor Author

Skrypt commented Apr 9, 2022

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

@Skrypt
Copy link
Contributor Author

Skrypt commented Apr 9, 2022

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 Skrypt removed the breaking change 💥 Issues or pull requests that introduces breaking change(s) label Apr 9, 2022
@jtkech
Copy link
Member

jtkech commented Apr 9, 2022

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.***.

@Skrypt
Copy link
Contributor Author

Skrypt commented Apr 9, 2022

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
Copy link
Contributor Author

Skrypt commented Apr 11, 2022

Okay waiting on @sebastienros approval on this one.

@Skrypt Skrypt changed the title Move ISlugService to OrchardCore.Autoroute.Abstractions Move ISlugService to OrchardCore.Autoroute.Core Apr 12, 2022
@sebastienros
Copy link
Member

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.

@agriffard
Copy link
Member

Conflicts to solve.

@hishamco
Copy link
Member

Resolve the conflict

# Conflicts:
#	OrchardCore.sln
#	src/OrchardCore/OrchardCore.Queries.Abstractions/LuceneQueryResults.cs
@Skrypt
Copy link
Contributor Author

Skrypt commented Sep 22, 2022

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

@Skrypt
Copy link
Contributor Author

Skrypt commented Sep 22, 2022

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

@Skrypt
Copy link
Contributor Author

Skrypt commented Sep 22, 2022

Done.

@Skrypt
Copy link
Contributor Author

Skrypt commented Oct 8, 2022

Fixed build again.

@Skrypt Skrypt merged commit 7376e41 into main Oct 12, 2022
@Skrypt Skrypt deleted the skrypt/#11489 branch October 12, 2022 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants