Open marcemarc opened 9 months ago
Hi there @marcemarc!
Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.
We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.
We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.
Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:
Hi @marcemarc ๐
Thank you for the investigation and the information sharing ๐ I managed to reproduce this in the latest Umbraco 10 as well. From "But now in V12 latest this blows up!" it seems like you had this working in another version of Umbraco. Can you clarify which one, so we can figure out if we are dealing with a regression bug?
For now, I will bring this with the team to discuss ๐ and get back to you either this or next week
Notes:
I managed to reproduce this in v10-v13 with the following setup and snippets:
using Microsoft.Extensions.Options;
using Umbraco.Cms.Core.Configuration.Models;
using Umbraco.Cms.Core.Models.PublishedContent;
using Umbraco.Cms.Core.Routing;
using Umbraco.Cms.Core.Services;
using Umbraco.Cms.Core.Web;
namespace Umbraco.Cms.Web.UI;
public class CustomUrlProvider : DefaultUrlProvider
{
public CustomUrlProvider(
IOptionsMonitor<RequestHandlerSettings> requestSettings,
ILogger<DefaultUrlProvider> logger,
ISiteDomainMapper siteDomainMapper,
IUmbracoContextAccessor umbracoContextAccessor,
UriUtility uriUtility,
ILocalizationService localizationService)
: base(requestSettings,
logger,
siteDomainMapper,
umbracoContextAccessor,
uriUtility,
localizationService)
{
}
public override UrlInfo? GetUrl(IPublishedContent content, UrlMode mode, string? culture, Uri current)
{
if (content is null)
{
return null;
}
// Only apply this to hidden folder pages
if (content.ContentType.Alias == "hiddenFolder")
{
// Get the original base url that the DefaultUrlProvider would have returned,
// it's important to call this via the base, rather than .Url, or UrlProvider.GetUrl to avoid cyclically calling this same provider in an infinite loop!!)
UrlInfo? defaultUrlInfo = base.GetUrl(content, mode, culture, current);
if (defaultUrlInfo is null)
{
return null;
}
if (!defaultUrlInfo.IsUrl)
{
// This is a message (eg published but not visible because the parent is unpublished or similar)
return defaultUrlInfo;
}
// Manipulate the url somehow in a custom fashion:
return UrlInfo.Message("This is not a url", culture);
}
// Otherwise return null
return null;
}
}
using Umbraco.Cms.Core.Composing;
namespace Umbraco.Cms.Web.UI;
public class RegisterCustomUrlProviderComposer : IComposer
{
public void Compose(IUmbracoBuilder builder)
{
builder.UrlProviders().Insert<CustomUrlProvider>();
}
}
HI @elit0451 Thanks for taking the time to reproduce. Looking through git blame, when collision detection was introduced: https://github.com/umbraco/Umbraco-CMS/blob/5f8747b4d3770ea2585a86f080a582f1826c3160/src/Umbraco.Web/Routing/UrlProviderExtensions.cs#L20 This would have worked, so around V7 time! There is a commit Here: github.com/umbraco/Umbraco-CMS/commit/8561d85f7af4b17681c6d84b5950a787fddabf7e#diff-cd06d5e77b193e5227cafc506b674f6620ba39c5c4f36acd55439db48ae573a0R75 Where the new URI was introduced in a commit that pulls 7.6 into 8....
... so maybe this is broken for a long time! I guess for this length of time it hasn't affected too many people!
But I think it is meant to be possible to return a UrlInfo Message in a UrlProvider! But if it's no longer meant to be we could stop the use of the UrlInfo.Message extension method etc
I think my suggested fix might be good though, because then the UrlInfo classes that are returned from the UrlProviders aren't discarded in favour of 'new' constructed UrlInfos!! memory allocation and all that! :-P
regards
Marc
Hi again ๐
So, I talked to the team and we agreed that this bug should be fixed. We would like some help with it so I will mark it as up for grabs. We need to make sure that if we create UrlInfo(string text, bool isUrl, string? culture)
and isUrl = false
we are not trying to make this a routable content
Hi @marcemarc,
We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.
For more information about issues and states, have a look at this blog post.
Thanks muchly, from your friendly Umbraco GitHub bot :-)
I've encountered a similar issue to what has been previously reported, and it appears that the CacheContent
functionality in Umbraco is causing problems. Specifically, the issue arises in the implementation of cache logic for GetUrl
, as seen in the GetRouteById
method.
Relevant Code:
The problematic code can be found in the Umbraco GitHub repository, within the ContentCache.cs
file:
Umbraco-CMS/ContentCache.cs at e04c4a89adcbe73d35290a993713a5faf4dc55ff
Issue Details:
The main issue is that the cache implements its own logic for GetUrl
in the GetRouteById
method. This implementation does not take into account custom URL providers. As a result, it seems to bypass or ignore any customizations provided by CustomUrlProvider
, leading to unexpected behavior or incorrect URL generation after saving a node.
e.g. CustomUrlProvider removes an urlsegement
/firstsegment/anotherone -> /anotherone
Initial run works, just after saving and publishing "/firstsegment" node causing that url is the old one /firstsegment/anotherone
Expected Behavior:
Ideally, the GetRouteById
method should respect and integrate with any custom logic provided by CustomUrlProvider
. This would ensure consistent and expected URL generation, aligning with the customizations intended by the developer.
Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)
12.3.3
Bug summary
When you are building an Umbraco site, you are not limited to having Urls generated based on their position in the Umbraco Content Tree, it's possible to manipulate the generation of the Urls by using a custom UrlProvider in combination with an IContentFinder.
In some scenarios with large Umbraco sites you might want to 'hide' a particular node from the Url.
eg if your site had lots of campaign landing pages, all in the root of the site for super SEO ranking points you might want to introduce nodes in the backoffice content tree, to look like folders but which aren't part of the Url, but help editors organise their campaign mess. eg /biscuit-offers/new-custard-cream-sale would become /new-custard-cream-sale and you could achieve this with a custom UrlProvider + IContentFinder to route the page at the root when it's not at the root.
In this scenario or similar, if the editor visited the /biscuit-offers/ node in the backoffice, and switch to info panel they would get the url /biscuit-offers/ but this isn't an actual page! or you would get the message 'this page is published but it is not in the cache'
Anyway you could always tidy this scenario up wtih Umbraco's super flexibility by having something like this in your UrlProvider
With the expectation that this message is shown to the editor in the links section instead of a Url!
But now in V12 latest this blows up!
Stack Trace
''' at System.Uri.CreateThis(String uri, Boolean dontEscape, UriKind uriKind, UriCreationOptions& creationOptions) at System.Uri..ctor(String uriString) at Umbraco.Extensions.UriExtensions.MakeAbsolute(Uri uri, Uri baseUri) at Umbraco.Extensions.UrlProviderExtensions.DetectCollisionAsync(ILogger logger, IContent content, String url, String culture, IUmbracoContext umbracoContext, IPublishedRouter publishedRouter, ILocalizedTextService textService, IVariationContextAccessor variationContextAccessor, UriUtility uriUtility) at Umbraco.Extensions.UrlProviderExtensions.GetContentUrlsByCultureAsync(IContent content, IEnumerableb1(Object source, Object target, MapperContext context)
at Umbraco.Cms.Core.Mapping.UmbracoMapper.Map[TTarget](Object source, Type sourceType, MapperContext context)
at Umbraco.Cms.Core.Mapping.UmbracoMapper.Map[TTarget](Object source, MapperContext context)
at Umbraco.Cms.Core.Mapping.UmbracoMapper.Map[TTarget](Object source, Action`1 f)
at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.MapToDisplayWithSchedule(IContent content)
at Umbraco.Cms.Web.BackOffice.Controllers.ContentController.GetById(Int32 id)
at lambda_method899(Closure, Object, Object[])
at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.SyncObjectResultExecutor.Execute(ActionContext actionContext, IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g__Logged|12_1(ControllerActionInvoker invoker)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.g Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.InvokeInnerFilterAsync()
--- End of stack trace from previous location ---
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|26_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
1 cultures, IPublishedRouter publishedRouter, IUmbracoContext umbracoContext, IContentService contentService, ILocalizedTextService textService, IVariationContextAccessor variationContextAccessor, ILogger logger, UriUtility uriUtility, IPublishedUrlProvider publishedUrlProvider) at Umbraco.Extensions.UrlProviderExtensions.GetContentUrlsAsync(IContent content, IPublishedRouter publishedRouter, IUmbracoContext umbracoContext, ILocalizationService localizationService, ILocalizedTextService textService, IContentService contentService, IVariationContextAccessor variationContextAccessor, ILogger
1 logger, UriUtility uriUtility, IPublishedUrlProvider publishedUrlProvider) at Umbraco.Cms.Web.BackOffice.Mapping.ContentMapDefinition.GetUrls(IContent source) at Umbraco.Cms.Web.BackOffice.Mapping.ContentMapDefinition.Map[TVariant](IContent source, ContentItemDisplay1 target, MapperContext context) at Umbraco.Cms.Core.Mapping.UmbracoMapper.<>c__DisplayClass11_0
2.'''
Specifics
Digging into this further...
What UrlInfo.Message does (it's an extension method on UrlInfo - https://github.com/umbraco/Umbraco-CMS/blob/contrib/src/Umbraco.Core/Routing/UrlInfo.cs#L80) is create a UrlInfo response from the provider with the text set to be the custom message and with the all-important isUrl flag set to false - so nobody mistakes this as an actual Url and tries to do something 'Urlish' with it...
.... but ....
In the GetContentUrlsByCultureAsync method, for each culture it tries to examine each possible Url and detect any collisions.
https://github.com/umbraco/Umbraco-CMS/blob/c6d01178c06a519423b3c37fea7bbdd0e536319f/src/Umbraco.Core/Routing/UrlProviderExtensions.cs#L107
The PublishedUrlProvider calls GetUrl, to find the UrlInfo objects for the node from the underlying registered UrlProviders... but only returns the 'text' property as a string... not the whole UrlInfo object
then this is passed to DetectCollsionAsync - https://github.com/umbraco/Umbraco-CMS/blob/c6d01178c06a519423b3c37fea7bbdd0e536319f/src/Umbraco.Core/Routing/UrlProviderExtensions.cs#L156
Attempt<UrlInfo?> hasCollision = await DetectCollisionAsync(logger, content, url, culture, umbracoContext, publishedRouter, textService, variationContextAccessor, uriUtility);
(which ironically returns a list of UrlInfos!)
These 'url strings' are used to see if there is a collision.
If a collision occurs, the collision message is added to the list of returned UrlInfos... if no collision - a brand new UrlInfo class is constructed from the string url
But the problemtherefore is triggered inside the DetectCollisionAsync method, the first line of code converts the provided Url into a Uri
var uri = new Uri(url.TrimEnd(Constants.CharArrays.ForwardSlash), UriKind.RelativeOrAbsolute);
https://github.com/umbraco/Umbraco-CMS/blob/c6d01178c06a519423b3c37fea7bbdd0e536319f/src/Umbraco.Core/Routing/UrlProviderExtensions.cs#L215C8-L215C103
and when the Text returned from UrlProvider is a custom message 'This is a hidden folder and has no Url' rather than an actual valid routable url, this blows up!
Ways to fix?
The PublishedUrlProvider could have a GetUrlInfos method that returns UrlInfo objects from the underlying UrlProviders, and then the IsUrl property could be used to determine whether or not to detect collisions, eg no need to if isUrl is false, it can just be added to the filtered list of UrlInfos... there won't be a clash from a message!
or
we could Try and parse the Url in DetectCollsionsAsync to see if it's a valid URI, or check for starting with / or something, and again allow anything that isn't a Url to be added as a message...
or have I totally misunderstood the situation!
Happy to have a go at fixing it if it needs to be fixed and if it's not a super breaking change that can't be made til Umbraco 25 etc...
Bizarre workaround
If you set your message to be /a-hidden-folder-has-no-url/ then this gets parsed successfully as a Uri! so you avoid the error, and if you add a further custom IContentFinder, that then routes this specific Url to page not found page... then the message appears as a Url! But you can tidy this up in the EditorSendingContentNotification
Steps to reproduce
Create a Custom UrlProvider for your site (have it inherit DefaultUrlProvider), replace the DefaultUrlProvider with your custom UrlProvider with UmbracoBuilder.
In your custom UrlProvider, check for a particular doctype, and return
UrlInfo.Message("this is not a url", culture);
Then visit that DocType in the backoffice and you'll get the error.
Expected result / actual result
Expected result is to see the Custom Message appear in the Urls section for a page based on the Doc Type you are sending the custom message to.