From 776df77dfe22a95e2f3c628405b551555d583ede Mon Sep 17 00:00:00 2001 From: Shannon Date: Fri, 11 Dec 2020 15:29:27 +1100 Subject: [PATCH] Notes and cleanup --- src/Umbraco.Core/Constants-Web.cs | 5 ----- src/Umbraco.Core/Web/IUmbracoContext.cs | 3 ++- .../Repositories/Implement/ContentRepositoryBase.cs | 9 ++++----- .../Implement/ContentTypeServiceBaseOfTItemTService.cs | 2 +- .../Services/EntityServiceTests.cs | 4 ---- .../Extensions/ApplicationBuilderExtensions.cs | 2 ++ .../Routing/UmbracoRouteValueTransformer.cs | 1 + 7 files changed, 10 insertions(+), 16 deletions(-) diff --git a/src/Umbraco.Core/Constants-Web.cs b/src/Umbraco.Core/Constants-Web.cs index e29d793909..8199d9fbd0 100644 --- a/src/Umbraco.Core/Constants-Web.cs +++ b/src/Umbraco.Core/Constants-Web.cs @@ -7,11 +7,6 @@ namespace Umbraco.Core /// public static class Web { - // TODO: Need to review these... - //public const string UmbracoContextDataToken = "umbraco-context"; - //public const string UmbracoDataToken = "umbraco"; - //public const string PublishedDocumentRequestDataToken = "umbraco-doc-request"; - //public const string CustomRouteDataToken = "umbraco-custom-route"; public const string UmbracoRouteDefinitionDataToken = "umbraco-route-def"; /// diff --git a/src/Umbraco.Core/Web/IUmbracoContext.cs b/src/Umbraco.Core/Web/IUmbracoContext.cs index 681dedbfd2..7fa02e3b73 100644 --- a/src/Umbraco.Core/Web/IUmbracoContext.cs +++ b/src/Umbraco.Core/Web/IUmbracoContext.cs @@ -49,11 +49,12 @@ namespace Umbraco.Web /// /// Boolean value indicating whether the current request is a front-end umbraco request /// - bool IsFrontEndUmbracoRequest { get; } + bool IsFrontEndUmbracoRequest { get; } // TODO: This could easily be an ext method and mocking just means setting the published request to null /// /// Gets/sets the PublishedRequest object /// + // TODO: Can we refactor this and not expose this mutable object here? Instead just expose IPublishedContent? A bunch of stuff would need to change but would be better IPublishedRequest PublishedRequest { get; set; } /// diff --git a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs index 7ce363e446..a84b34b75a 100644 --- a/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs +++ b/src/Umbraco.Infrastructure/Persistence/Repositories/Implement/ContentRepositoryBase.cs @@ -772,11 +772,10 @@ namespace Umbraco.Core.Persistence.Repositories.Implement * level now since we can fire these events within the transaction... * The reason these events 'need' to fire in the transaction is to ensure data consistency with Nucache (currently * the only thing that uses them). For example, if the transaction succeeds and NuCache listened to ContentService.Saved - * and then NuCache failed at persisting data after the trans completed, then NuCache would be out of sync. This way - * the entire trans is rolled back if NuCache files. That said, I'm unsure this is really required because there - * are other systems that rely on the "ed" (i.e. Saved) events like Examine which would be inconsistent if it failed - * too. I'm just not sure this is totally necessary especially. - * So these events can be moved to the service level. However, see the notes below, it seems the only event we + * and then NuCache failed at persisting data after the trans completed, then NuCache would be out of sync. This way + * the entire trans is rolled back if NuCache files. This is part of the discussion about removing the static events, + * possibly there's 3 levels of eventing, "ing", "scoped" (in trans) and "ed" (after trans). + * These particular events can be moved to the service level. However, see the notes below, it seems the only event we * really need is the ScopedEntityRefresh. The only tricky part with moving that to the service level is that the * handlers of that event will need to deal with the data a little differently because it seems that the * "Published" flag on the content item matters and this event is raised before that flag is switched. Weird. diff --git a/src/Umbraco.Infrastructure/Services/Implement/ContentTypeServiceBaseOfTItemTService.cs b/src/Umbraco.Infrastructure/Services/Implement/ContentTypeServiceBaseOfTItemTService.cs index 1bdd00f576..be541486ff 100644 --- a/src/Umbraco.Infrastructure/Services/Implement/ContentTypeServiceBaseOfTItemTService.cs +++ b/src/Umbraco.Infrastructure/Services/Implement/ContentTypeServiceBaseOfTItemTService.cs @@ -31,7 +31,7 @@ namespace Umbraco.Core.Services.Implement /// The purpose of this event being raised within the transaction is so that listeners can perform database /// operations from within the same transaction and guarantee data consistency so that if anything goes wrong /// the entire transaction can be rolled back. This is used by Nucache. - /// TODO: See remarks in ContentRepositoryBase about these types of events. Not sure we need/want them. + /// TODO: See remarks in ContentRepositoryBase about these types of events. /// public static event TypedEventHandler.EventArgs> ScopedRefreshedEntity; diff --git a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/EntityServiceTests.cs b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/EntityServiceTests.cs index 4f4a852902..6d5cf19b50 100644 --- a/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/EntityServiceTests.cs +++ b/src/Umbraco.Tests.Integration/Umbraco.Infrastructure/Services/EntityServiceTests.cs @@ -42,10 +42,6 @@ namespace Umbraco.Tests.Integration.Umbraco.Infrastructure.Services [SetUp] public void SetupTestData() { - // This is super nasty, but this lets us initialize the cache while it is empty. - // var publishedSnapshotService = GetRequiredService() as PublishedSnapshotService; - // publishedSnapshotService?.OnApplicationInit(null, EventArgs.Empty); - if (_langFr == null && _langEs == null) { var globalSettings = new GlobalSettings(); diff --git a/src/Umbraco.Web.Common/Extensions/ApplicationBuilderExtensions.cs b/src/Umbraco.Web.Common/Extensions/ApplicationBuilderExtensions.cs index 7290aa9b0e..6fdd5c9be7 100644 --- a/src/Umbraco.Web.Common/Extensions/ApplicationBuilderExtensions.cs +++ b/src/Umbraco.Web.Common/Extensions/ApplicationBuilderExtensions.cs @@ -183,6 +183,8 @@ namespace Umbraco.Extensions /// public static IApplicationBuilder UseUmbracoContentCache(this IApplicationBuilder app) { + // TODO: This should install middleware to initialize instead of eagerly doing the initialize here + PublishedSnapshotServiceEventHandler publishedContentEvents = app.ApplicationServices.GetRequiredService(); publishedContentEvents.Start(); return app; diff --git a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs index 2c1debc3ee..be7c9f7409 100644 --- a/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs +++ b/src/Umbraco.Web.Website/Routing/UmbracoRouteValueTransformer.cs @@ -32,6 +32,7 @@ namespace Umbraco.Web.Website.Routing /// It seems as though with the "State" parameter we could more easily assign the IPublishedRequest or IPublishedContent /// or UmbracoContext more easily that way. In the meantime we will rely on assigning the IPublishedRequest to the /// route values along with the IPublishedContent to the umbraco context + /// have created a GH discussion here https://github.com/dotnet/aspnetcore/discussions/28562 we'll see if anyone responds /// public class UmbracoRouteValueTransformer : DynamicRouteValueTransformer {