From 6c395db5a5b1a15406407eccc9200846524455a6 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Wed, 12 Feb 2020 10:56:23 +0100 Subject: [PATCH 1/4] Removed legacy way of handling alternative templates.. Today we use `ContentFinderByUrlAndTemplate` --- .../Constants-Conventions.cs | 13 +--- src/Umbraco.Web/Routing/PublishedRouter.cs | 73 ++----------------- src/Umbraco.Web/Templates/TemplateRenderer.cs | 11 +-- 3 files changed, 10 insertions(+), 87 deletions(-) diff --git a/src/Umbraco.Abstractions/Constants-Conventions.cs b/src/Umbraco.Abstractions/Constants-Conventions.cs index 37275b156a..6672f22239 100644 --- a/src/Umbraco.Abstractions/Constants-Conventions.cs +++ b/src/Umbraco.Abstractions/Constants-Conventions.cs @@ -137,7 +137,7 @@ namespace Umbraco.Core public static readonly string UmbracoMemberProviderName = "UmbracoMembershipProvider"; public static readonly string UmbracoRoleProviderName = "UmbracoRoleProvider"; - + /// /// Property alias for the Comments on a Member /// @@ -208,17 +208,6 @@ namespace Umbraco.Core public const string AllMembersListId = "all-members"; } - /// - /// Constants for Umbraco URLs/Querystrings. - /// - public static class Url - { - /// - /// Querystring parameter name used for Umbraco's alternative template functionality. - /// - public const string AltTemplate = "altTemplate"; - } - /// /// Defines the alias identifiers for built-in Umbraco relation types. /// diff --git a/src/Umbraco.Web/Routing/PublishedRouter.cs b/src/Umbraco.Web/Routing/PublishedRouter.cs index 9148ce2e31..74fe9f93e6 100644 --- a/src/Umbraco.Web/Routing/PublishedRouter.cs +++ b/src/Umbraco.Web/Routing/PublishedRouter.cs @@ -20,7 +20,6 @@ namespace Umbraco.Web.Routing /// public class PublishedRouter : IPublishedRouter { - private readonly IWebRoutingSection _webRoutingSection; private readonly ContentFinderCollection _contentFinders; private readonly IContentLastChanceFinder _contentLastChanceFinder; private readonly ServiceContext _services; @@ -34,7 +33,6 @@ namespace Umbraco.Web.Routing /// Initializes a new instance of the class. /// public PublishedRouter( - IWebRoutingSection webRoutingSection, ContentFinderCollection contentFinders, IContentLastChanceFinder contentLastChanceFinder, IVariationContextAccessor variationContextAccessor, @@ -43,7 +41,6 @@ namespace Umbraco.Web.Routing IUmbracoSettingsSection umbracoSettingsSection, IUserService userService) { - _webRoutingSection = webRoutingSection ?? throw new ArgumentNullException(nameof(webRoutingSection)); _contentFinders = contentFinders ?? throw new ArgumentNullException(nameof(contentFinders)); _contentLastChanceFinder = contentLastChanceFinder ?? throw new ArgumentNullException(nameof(contentLastChanceFinder)); _services = services ?? throw new ArgumentNullException(nameof(services)); @@ -644,74 +641,14 @@ namespace Umbraco.Web.Routing return; } - // read the alternate template alias, from querystring, form, cookie or server vars, - // only if the published content is the initial once, else the alternate template - // does not apply - // + optionally, apply the alternate template on internal redirects - var useAltTemplate = request.IsInitialPublishedContent - || (_webRoutingSection.InternalRedirectPreservesTemplate && request.IsInternalRedirectPublishedContent); - var altTemplate = useAltTemplate - ? request.UmbracoContext.HttpContext.Request[Constants.Conventions.Url.AltTemplate] - : null; - - if (string.IsNullOrWhiteSpace(altTemplate)) + if (request.HasTemplate) { - // we don't have an alternate template specified. use the current one if there's one already, - // which can happen if a content lookup also set the template (LookupByNiceUrlAndTemplate...), - // else lookup the template id on the document then lookup the template with that id. - - if (request.HasTemplate) - { - _logger.Debug("FindTemplate: Has a template already, and no alternate template."); - return; - } - - // TODO: When we remove the need for a database for templates, then this id should be irrelevant, - // not sure how were going to do this nicely. - - // TODO: We need to limit altTemplate to only allow templates that are assigned to the current document type! - // if the template isn't assigned to the document type we should log a warning and return 404 - - var templateId = request.PublishedContent.TemplateId; - request.TemplateModel = GetTemplateModel(templateId); + _logger.Debug("FindTemplate: Has a template already, and no alternate template."); + return; } - else - { - // we have an alternate template specified. lookup the template with that alias - // this means the we override any template that a content lookup might have set - // so /path/to/page/template1?altTemplate=template2 will use template2 - // ignore if the alias does not match - just trace - - if (request.HasTemplate) - _logger.Debug("FindTemplate: Has a template already, but also an alternative template."); - _logger.Debug("FindTemplate: Look for alternative template alias={AltTemplate}", altTemplate); - - // IsAllowedTemplate deals both with DisableAlternativeTemplates and ValidateAlternativeTemplates settings - if (request.PublishedContent.IsAllowedTemplate(altTemplate)) - { - // allowed, use - var template = _services.FileService.GetTemplate(altTemplate); - - if (template != null) - { - request.TemplateModel = template; - _logger.Debug("FindTemplate: Got alternative template id={TemplateId} alias={TemplateAlias}", template.Id, template.Alias); - } - else - { - _logger.Debug("FindTemplate: The alternative template with alias={AltTemplate} does not exist, ignoring.", altTemplate); - } - } - else - { - _logger.Warn("FindTemplate: Alternative template {TemplateAlias} is not allowed on node {NodeId}, ignoring.", altTemplate, request.PublishedContent.Id); - - // no allowed, back to default - var templateId = request.PublishedContent.TemplateId; - request.TemplateModel = GetTemplateModel(templateId); - } - } + var templateId = request.PublishedContent.TemplateId; + request.TemplateModel = GetTemplateModel(templateId); if (request.HasTemplate == false) { diff --git a/src/Umbraco.Web/Templates/TemplateRenderer.cs b/src/Umbraco.Web/Templates/TemplateRenderer.cs index b13719f6e9..8e8d1b5068 100644 --- a/src/Umbraco.Web/Templates/TemplateRenderer.cs +++ b/src/Umbraco.Web/Templates/TemplateRenderer.cs @@ -96,7 +96,7 @@ namespace Umbraco.Web.Templates //First, save all of the items locally that we know are used in the chain of execution, we'll need to restore these //after this page has rendered. - SaveExistingItems(out var oldPublishedRequest, out var oldAltTemplate); + SaveExistingItems(out var oldPublishedRequest); try { @@ -109,7 +109,7 @@ namespace Umbraco.Web.Templates finally { //restore items on context objects to continuing rendering the parent template - RestoreItems(oldPublishedRequest, oldAltTemplate); + RestoreItems(oldPublishedRequest); } } @@ -172,28 +172,25 @@ namespace Umbraco.Web.Templates private void SetNewItemsOnContextObjects(PublishedRequest request) { //now, set the new ones for this page execution - _umbracoContextAccessor.UmbracoContext.HttpContext.Items[Core.Constants.Conventions.Url.AltTemplate] = null; _umbracoContextAccessor.UmbracoContext.PublishedRequest = request; } /// /// Save all items that we know are used for rendering execution to variables so we can restore after rendering /// - private void SaveExistingItems(out PublishedRequest oldPublishedRequest, out object oldAltTemplate) + private void SaveExistingItems(out PublishedRequest oldPublishedRequest) { //Many objects require that these legacy items are in the http context items... before we render this template we need to first //save the values in them so that we can re-set them after we render so the rest of the execution works as per normal oldPublishedRequest = _umbracoContextAccessor.UmbracoContext.PublishedRequest; - oldAltTemplate = _umbracoContextAccessor.UmbracoContext.HttpContext.Items[Core.Constants.Conventions.Url.AltTemplate]; } /// /// Restores all items back to their context's to continue normal page rendering execution /// - private void RestoreItems(PublishedRequest oldPublishedRequest, object oldAltTemplate) + private void RestoreItems(PublishedRequest oldPublishedRequest) { _umbracoContextAccessor.UmbracoContext.PublishedRequest = oldPublishedRequest; - _umbracoContextAccessor.UmbracoContext.HttpContext.Items[Core.Constants.Conventions.Url.AltTemplate] = oldAltTemplate; } } } From 0f6c945fe0e3338e32e2d9bc44fddfbc565241c8 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Wed, 12 Feb 2020 11:12:09 +0100 Subject: [PATCH 2/4] Fix tests --- src/Umbraco.Tests/TestHelpers/BaseWebTest.cs | 12 +----------- src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs | 2 +- src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs | 2 +- 3 files changed, 3 insertions(+), 13 deletions(-) diff --git a/src/Umbraco.Tests/TestHelpers/BaseWebTest.cs b/src/Umbraco.Tests/TestHelpers/BaseWebTest.cs index 176664b467..01e2c66e3c 100644 --- a/src/Umbraco.Tests/TestHelpers/BaseWebTest.cs +++ b/src/Umbraco.Tests/TestHelpers/BaseWebTest.cs @@ -40,10 +40,6 @@ namespace Umbraco.Tests.TestHelpers // need to specify a custom callback for unit tests // AutoPublishedContentTypes generates properties automatically - var dataTypeService = new TestObjects.TestDataTypeService( - new DataType(new VoidEditor(Mock.Of(), Mock.Of(), Mock.Of(),Mock.Of(), Mock.Of())) { Id = 1 }); - - var factory = new PublishedContentTypeFactory(Mock.Of(), new PropertyValueConverterCollection(Array.Empty()), dataTypeService); var type = new AutoPublishedContentType(0, "anything", new PublishedPropertyType[] { }); ContentTypesCache.GetPublishedContentTypeByAlias = alias => GetPublishedContentTypeByAlias(alias) ?? type; } @@ -87,15 +83,9 @@ namespace Umbraco.Tests.TestHelpers "; } - internal PublishedRouter CreatePublishedRouter(IFactory container = null, ContentFinderCollection contentFinders = null) - { - return CreatePublishedRouter(TestObjects.GetUmbracoSettings().WebRouting, container ?? Factory, contentFinders); - } - - internal static PublishedRouter CreatePublishedRouter(IWebRoutingSection webRoutingSection, IFactory container = null, ContentFinderCollection contentFinders = null) + internal static PublishedRouter CreatePublishedRouter(IFactory container = null, ContentFinderCollection contentFinders = null) { return new PublishedRouter( - webRoutingSection, contentFinders ?? new ContentFinderCollection(Enumerable.Empty()), new TestLastChanceFinder(), new TestVariationContextAccessor(), diff --git a/src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs b/src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs index c54192f869..76a1d0e23d 100644 --- a/src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs +++ b/src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs @@ -160,7 +160,7 @@ namespace Umbraco.Tests.Web.Mvc var content = Mock.Of(publishedContent => publishedContent.Id == 12345); var contextBase = umbracoContext.HttpContext; - var publishedRouter = BaseWebTest.CreatePublishedRouter(TestObjects.GetUmbracoSettings().WebRouting); + var publishedRouter = BaseWebTest.CreatePublishedRouter(); var frequest = publishedRouter.CreateRequest(umbracoContext, new Uri("http://localhost/test")); frequest.PublishedContent = content; diff --git a/src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs b/src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs index 942666c7b0..638189b963 100644 --- a/src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs +++ b/src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs @@ -391,7 +391,7 @@ namespace Umbraco.Tests.Web.Mvc logger, settings, "/dang", 0); - var publishedRouter = BaseWebTest.CreatePublishedRouter(TestObjects.GetUmbracoSettings().WebRouting); + var publishedRouter = BaseWebTest.CreatePublishedRouter(); var frequest = publishedRouter.CreateRequest(umbracoContext, new Uri("http://localhost/dang")); frequest.Culture = CultureInfo.InvariantCulture; From 99045eaaa48ea06411fcce0928737965d04f7668 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 13 Feb 2020 09:17:57 +0100 Subject: [PATCH 3/4] Reintroduced some code that could not be removed --- .../Constants-Conventions.cs | 12 +++ src/Umbraco.Web/Routing/PublishedRouter.cs | 77 +++++++++++++++++-- 2 files changed, 82 insertions(+), 7 deletions(-) diff --git a/src/Umbraco.Abstractions/Constants-Conventions.cs b/src/Umbraco.Abstractions/Constants-Conventions.cs index 6672f22239..0bfb890abd 100644 --- a/src/Umbraco.Abstractions/Constants-Conventions.cs +++ b/src/Umbraco.Abstractions/Constants-Conventions.cs @@ -208,6 +208,17 @@ namespace Umbraco.Core public const string AllMembersListId = "all-members"; } + /// + /// Constants for Umbraco URLs/Querystrings. + /// + public static class Url + { + /// + /// Querystring parameter name used for Umbraco's alternative template functionality. + /// + public const string AltTemplate = "altTemplate"; + } + /// /// Defines the alias identifiers for built-in Umbraco relation types. /// @@ -274,6 +285,7 @@ namespace Umbraco.Core //TODO: return a list of built in types so we can use that to prevent deletion in the uI } + } } } diff --git a/src/Umbraco.Web/Routing/PublishedRouter.cs b/src/Umbraco.Web/Routing/PublishedRouter.cs index 74fe9f93e6..9148ce2e31 100644 --- a/src/Umbraco.Web/Routing/PublishedRouter.cs +++ b/src/Umbraco.Web/Routing/PublishedRouter.cs @@ -20,6 +20,7 @@ namespace Umbraco.Web.Routing /// public class PublishedRouter : IPublishedRouter { + private readonly IWebRoutingSection _webRoutingSection; private readonly ContentFinderCollection _contentFinders; private readonly IContentLastChanceFinder _contentLastChanceFinder; private readonly ServiceContext _services; @@ -33,6 +34,7 @@ namespace Umbraco.Web.Routing /// Initializes a new instance of the class. /// public PublishedRouter( + IWebRoutingSection webRoutingSection, ContentFinderCollection contentFinders, IContentLastChanceFinder contentLastChanceFinder, IVariationContextAccessor variationContextAccessor, @@ -41,6 +43,7 @@ namespace Umbraco.Web.Routing IUmbracoSettingsSection umbracoSettingsSection, IUserService userService) { + _webRoutingSection = webRoutingSection ?? throw new ArgumentNullException(nameof(webRoutingSection)); _contentFinders = contentFinders ?? throw new ArgumentNullException(nameof(contentFinders)); _contentLastChanceFinder = contentLastChanceFinder ?? throw new ArgumentNullException(nameof(contentLastChanceFinder)); _services = services ?? throw new ArgumentNullException(nameof(services)); @@ -641,14 +644,74 @@ namespace Umbraco.Web.Routing return; } - if (request.HasTemplate) - { - _logger.Debug("FindTemplate: Has a template already, and no alternate template."); - return; - } + // read the alternate template alias, from querystring, form, cookie or server vars, + // only if the published content is the initial once, else the alternate template + // does not apply + // + optionally, apply the alternate template on internal redirects + var useAltTemplate = request.IsInitialPublishedContent + || (_webRoutingSection.InternalRedirectPreservesTemplate && request.IsInternalRedirectPublishedContent); + var altTemplate = useAltTemplate + ? request.UmbracoContext.HttpContext.Request[Constants.Conventions.Url.AltTemplate] + : null; - var templateId = request.PublishedContent.TemplateId; - request.TemplateModel = GetTemplateModel(templateId); + if (string.IsNullOrWhiteSpace(altTemplate)) + { + // we don't have an alternate template specified. use the current one if there's one already, + // which can happen if a content lookup also set the template (LookupByNiceUrlAndTemplate...), + // else lookup the template id on the document then lookup the template with that id. + + if (request.HasTemplate) + { + _logger.Debug("FindTemplate: Has a template already, and no alternate template."); + return; + } + + // TODO: When we remove the need for a database for templates, then this id should be irrelevant, + // not sure how were going to do this nicely. + + // TODO: We need to limit altTemplate to only allow templates that are assigned to the current document type! + // if the template isn't assigned to the document type we should log a warning and return 404 + + var templateId = request.PublishedContent.TemplateId; + request.TemplateModel = GetTemplateModel(templateId); + } + else + { + // we have an alternate template specified. lookup the template with that alias + // this means the we override any template that a content lookup might have set + // so /path/to/page/template1?altTemplate=template2 will use template2 + + // ignore if the alias does not match - just trace + + if (request.HasTemplate) + _logger.Debug("FindTemplate: Has a template already, but also an alternative template."); + _logger.Debug("FindTemplate: Look for alternative template alias={AltTemplate}", altTemplate); + + // IsAllowedTemplate deals both with DisableAlternativeTemplates and ValidateAlternativeTemplates settings + if (request.PublishedContent.IsAllowedTemplate(altTemplate)) + { + // allowed, use + var template = _services.FileService.GetTemplate(altTemplate); + + if (template != null) + { + request.TemplateModel = template; + _logger.Debug("FindTemplate: Got alternative template id={TemplateId} alias={TemplateAlias}", template.Id, template.Alias); + } + else + { + _logger.Debug("FindTemplate: The alternative template with alias={AltTemplate} does not exist, ignoring.", altTemplate); + } + } + else + { + _logger.Warn("FindTemplate: Alternative template {TemplateAlias} is not allowed on node {NodeId}, ignoring.", altTemplate, request.PublishedContent.Id); + + // no allowed, back to default + var templateId = request.PublishedContent.TemplateId; + request.TemplateModel = GetTemplateModel(templateId); + } + } if (request.HasTemplate == false) { From 04b1e3734814337decd01e17454b709875741110 Mon Sep 17 00:00:00 2001 From: Bjarke Berg Date: Thu, 13 Feb 2020 09:38:16 +0100 Subject: [PATCH 4/4] Reintroduced some code that could not be removed --- src/Umbraco.Tests/TestHelpers/BaseWebTest.cs | 8 +++++++- src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs | 2 +- src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/Umbraco.Tests/TestHelpers/BaseWebTest.cs b/src/Umbraco.Tests/TestHelpers/BaseWebTest.cs index 01e2c66e3c..3fa61e8961 100644 --- a/src/Umbraco.Tests/TestHelpers/BaseWebTest.cs +++ b/src/Umbraco.Tests/TestHelpers/BaseWebTest.cs @@ -83,9 +83,15 @@ namespace Umbraco.Tests.TestHelpers "; } - internal static PublishedRouter CreatePublishedRouter(IFactory container = null, ContentFinderCollection contentFinders = null) + internal PublishedRouter CreatePublishedRouter(IFactory container = null, ContentFinderCollection contentFinders = null) + { + return CreatePublishedRouter(TestObjects.GetUmbracoSettings().WebRouting, container ?? Factory, contentFinders); + } + + internal static PublishedRouter CreatePublishedRouter(IWebRoutingSection webRoutingSection, IFactory container = null, ContentFinderCollection contentFinders = null) { return new PublishedRouter( + webRoutingSection, contentFinders ?? new ContentFinderCollection(Enumerable.Empty()), new TestLastChanceFinder(), new TestVariationContextAccessor(), diff --git a/src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs b/src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs index 76a1d0e23d..c54192f869 100644 --- a/src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs +++ b/src/Umbraco.Tests/Web/Mvc/SurfaceControllerTests.cs @@ -160,7 +160,7 @@ namespace Umbraco.Tests.Web.Mvc var content = Mock.Of(publishedContent => publishedContent.Id == 12345); var contextBase = umbracoContext.HttpContext; - var publishedRouter = BaseWebTest.CreatePublishedRouter(); + var publishedRouter = BaseWebTest.CreatePublishedRouter(TestObjects.GetUmbracoSettings().WebRouting); var frequest = publishedRouter.CreateRequest(umbracoContext, new Uri("http://localhost/test")); frequest.PublishedContent = content; diff --git a/src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs b/src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs index 638189b963..942666c7b0 100644 --- a/src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs +++ b/src/Umbraco.Tests/Web/Mvc/UmbracoViewPageTests.cs @@ -391,7 +391,7 @@ namespace Umbraco.Tests.Web.Mvc logger, settings, "/dang", 0); - var publishedRouter = BaseWebTest.CreatePublishedRouter(); + var publishedRouter = BaseWebTest.CreatePublishedRouter(TestObjects.GetUmbracoSettings().WebRouting); var frequest = publishedRouter.CreateRequest(umbracoContext, new Uri("http://localhost/dang")); frequest.Culture = CultureInfo.InvariantCulture;