diff --git a/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs b/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs index 2f1bef3b9a..c7ce42e4bc 100644 --- a/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs +++ b/src/Umbraco.Tests/Routing/UrlsProviderWithDomainsTests.cs @@ -418,8 +418,8 @@ namespace Umbraco.Tests.Routing foreach (var x in result) Console.WriteLine(x); Assert.AreEqual(2, result.Length); - Assert.AreEqual(result[0].Text, "http://domain1a.com/en/1001-1-1/"); - Assert.AreEqual(result[1].Text, "http://domain1b.com/en/1001-1-1/"); + Assert.AreEqual(result[0].Text, "http://domain1b.com/en/1001-1-1/"); + Assert.AreEqual(result[1].Text, "http://domain1a.com/en/1001-1-1/"); } } } diff --git a/src/Umbraco.Web/Routing/DomainHelper.cs b/src/Umbraco.Web/Routing/DomainHelper.cs index b6d79e788a..9b300009d0 100644 --- a/src/Umbraco.Web/Routing/DomainHelper.cs +++ b/src/Umbraco.Web/Routing/DomainHelper.cs @@ -207,8 +207,14 @@ namespace Umbraco.Web.Routing var cultureDomains = domainsAndUris.Where(x => x.Culture.Name.InvariantEquals(culture)).ToList(); if (cultureDomains.Count > 0) return cultureDomains; + // if a culture is supplied, we *want* a url for that culture, else fail - throw new InvalidOperationException($"Could not find a domain for culture \"{culture}\"."); + //throw new InvalidOperationException($"Could not find a domain for culture \"{culture}\"."); + //fixme: Review - throwing here causes a problem because the UrlProviderExtensions.GetContentUrls iterates through + // ALL cultures even if those cultures are not assigned for use within a branch. Returning null + // here fixes that problem and the URLs resolve correctly, however i don't know if this is causing other + // residual problems. It would also suggest that below in GetByCulture we don't throw either but instead return null?? + return null; } if (defaultCulture != null) // try the defaultCulture culture diff --git a/src/Umbraco.Web/Routing/UrlInfo.cs b/src/Umbraco.Web/Routing/UrlInfo.cs index a795f1577b..ae5c4b412c 100644 --- a/src/Umbraco.Web/Routing/UrlInfo.cs +++ b/src/Umbraco.Web/Routing/UrlInfo.cs @@ -23,7 +23,7 @@ namespace Umbraco.Web.Routing /// /// Initializes a new instance of the class. /// - private UrlInfo(string text, bool isUrl, string culture) + public UrlInfo(string text, bool isUrl, string culture) { if (string.IsNullOrWhiteSpace(text)) throw new ArgumentException("Value cannot be null or whitespace.", nameof(text)); IsUrl = isUrl; diff --git a/src/Umbraco.Web/Routing/UrlInfoComparer.cs b/src/Umbraco.Web/Routing/UrlInfoComparer.cs new file mode 100644 index 0000000000..ff41dc6bc9 --- /dev/null +++ b/src/Umbraco.Web/Routing/UrlInfoComparer.cs @@ -0,0 +1,66 @@ +using System; +using System.Collections.Generic; + +namespace Umbraco.Web.Routing +{ + /// + /// Compares + /// + public class UrlInfoComparer : IEqualityComparer + { + private readonly bool _variesByCulture; + + public UrlInfoComparer(bool variesByCulture) + { + _variesByCulture = variesByCulture; + } + + /// + /// Determines equality between + /// + /// + /// + /// + /// + /// If variesByCulture is true, then culture is compared, otherwise culture is not compared. + /// Both culture and url are compared without case sensitivity. + /// + public bool Equals(UrlInfo x, UrlInfo y) + { + if (ReferenceEquals(null, y)) return false; + if (ReferenceEquals(null, x)) return false; + if (ReferenceEquals(x, y)) return true; + + if (_variesByCulture) + { + return string.Equals(x.Culture, y.Culture, StringComparison.InvariantCultureIgnoreCase) + && x.IsUrl == y.IsUrl + && string.Equals(x.Text, y.Text, StringComparison.InvariantCultureIgnoreCase); + } + + return x.IsUrl == y.IsUrl + && string.Equals(x.Text, y.Text, StringComparison.InvariantCultureIgnoreCase); + } + + /// + /// Calculates a hash code + /// + /// + /// + /// + /// If variesByCulture is true then culture is used in the calculation, otherwise it's not + /// + public int GetHashCode(UrlInfo obj) + { + unchecked + { + var hashCode = (obj.Text != null ? StringComparer.InvariantCultureIgnoreCase.GetHashCode(obj.Text) : 0); + hashCode = (hashCode * 397) ^ obj.IsUrl.GetHashCode(); + if (_variesByCulture) + hashCode = (hashCode * 397) ^ (obj.Culture != null ? StringComparer.InvariantCultureIgnoreCase.GetHashCode(obj.Culture) : 0); + return hashCode; + } + + } + } +} \ No newline at end of file diff --git a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs index 3a0caeec78..71a0c294ab 100644 --- a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs +++ b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs @@ -18,7 +18,7 @@ namespace Umbraco.Web.Routing /// Use when displaying Urls. If errors occur when generating the Urls, they will show in the list. /// Contains all the Urls that we can figure out (based upon domains, etc). /// - public static IEnumerable GetContentUrls(this IContent content, + public static IEnumerable GetContentUrls(this IContent content, PublishedRouter publishedRouter, UmbracoContext umbracoContext, ILocalizationService localizationService, @@ -44,12 +44,57 @@ namespace Umbraco.Web.Routing // - the 'main' urls, which is what .Url would return, for each culture // - the 'other' urls we know (based upon domains, etc) // - // need to work on each culture. - // on invariant trees, each culture return the same thing - // but, we don't know if the tree to this content is invariant + // need to work through each installed culture. + // fixme: Why not just work with each culture assigned to domains in the branch? + // on invariant nodes, each culture returns the same url segment + // but, we don't know if the branch to this content is invariant so we need to ask + // for URLs for all cultures. var cultures = localizationService.GetAllLanguages().Select(x => x.IsoCode).ToList(); + //get all URLs for all cultures + foreach (var cultureUrl in GetContentUrlsByCulture(content, cultures, publishedRouter, umbracoContext, contentService, textService, logger)) + { + urls.Add(cultureUrl); + } + + //return the real urls first, then the messages + foreach (var urlGroup in urls.GroupBy(x => x.IsUrl).OrderByDescending(x => x.Key)) + { + //in some cases there will be the same URL for multiple cultures: + // * The entire branch is invariant + // * If there are less domain/cultures assigned to the branch than the number of cultures/languages installed + + foreach (var dUrl in urlGroup.DistinctBy(x => x.Text.ToUpperInvariant())) + yield return dUrl; + } + + // get the 'other' urls - ie not what you'd get with GetUrl() but urls that would route to the document, nevertheless. + // for these 'other' urls, we don't check whether they are routable, collide, anything - we just report them. + foreach (var otherUrl in umbracoContext.UrlProvider.GetOtherUrls(content.Id)) + if (urls.Add(otherUrl)) //avoid duplicates + yield return otherUrl; + } + + /// + /// Tries to return a for each culture for the content while detecting collisions/errors + /// + /// + /// + /// + /// + /// + /// + /// + /// + private static IEnumerable GetContentUrlsByCulture(IContent content, + IEnumerable cultures, + PublishedRouter publishedRouter, + UmbracoContext umbracoContext, + IContentService contentService, + ILocalizedTextService textService, + ILogger logger) + { foreach (var culture in cultures) { // if content is variant, and culture is not published, skip @@ -73,34 +118,26 @@ namespace Umbraco.Web.Routing { // deal with 'could not get the url' case "#": - HandleCouldNotGetUrl(content, culture, urls, contentService, textService); + yield return HandleCouldNotGetUrl(content, culture, contentService, textService); break; // deal with exceptions case "#ex": - urls.Add(UrlInfo.Message(textService.Localize("content/getUrlException"), culture)); + yield return UrlInfo.Message(textService.Localize("content/getUrlException"), culture); break; // got a url, deal with collisions, add url default: - if (!DetectCollision(content, url, urls, culture, umbracoContext, publishedRouter, textService)) // detect collisions, etc - urls.Add(UrlInfo.Url(url, culture)); + if (DetectCollision(content, url, culture, umbracoContext, publishedRouter, textService, out var urlInfo)) // detect collisions, etc + yield return urlInfo; + else + yield return UrlInfo.Url(url, culture); break; } } - - //return the real urls first, then the messages - foreach (var urlGroup in urls.GroupBy(x => x.IsUrl).OrderByDescending(x => x.Key)) - foreach (var url in urlGroup) - yield return url; - - // get the 'other' urls - ie not what you'd get with GetUrl() but urls that would route to the document, nevertheless. - // for these 'other' urls, we don't check whether they are routable, collide, anything - we just report them. - foreach (var otherUrl in umbracoContext.UrlProvider.GetOtherUrls(content.Id)) - yield return otherUrl; } - private static void HandleCouldNotGetUrl(IContent content, string culture, ICollection urls, IContentService contentService, ILocalizedTextService textService) + private static UrlInfo HandleCouldNotGetUrl(IContent content, string culture, IContentService contentService, ILocalizedTextService textService) { // document has a published version yet its url is "#" => a parent must be // unpublished, walk up the tree until we find it, and report. @@ -112,16 +149,16 @@ namespace Umbraco.Web.Routing while (parent != null && parent.Published && (!parent.ContentType.VariesByCulture() || parent.IsCulturePublished(culture))); if (parent == null) // oops, internal error - urls.Add(UrlInfo.Message(textService.Localize("content/parentNotPublishedAnomaly"), culture)); + return UrlInfo.Message(textService.Localize("content/parentNotPublishedAnomaly"), culture); else if (!parent.Published) // totally not published - urls.Add(UrlInfo.Message(textService.Localize("content/parentNotPublished", new[] { parent.Name }), culture)); + return UrlInfo.Message(textService.Localize("content/parentNotPublished", new[] {parent.Name}), culture); else // culture not published - urls.Add(UrlInfo.Message(textService.Localize("content/parentCultureNotPublished", new[] { parent.Name }), culture)); + return UrlInfo.Message(textService.Localize("content/parentCultureNotPublished", new[] {parent.Name}), culture); } - private static bool DetectCollision(IContent content, string url, ICollection urls, string culture, UmbracoContext umbracoContext, PublishedRouter publishedRouter, ILocalizedTextService textService) + private static bool DetectCollision(IContent content, string url, string culture, UmbracoContext umbracoContext, PublishedRouter publishedRouter, ILocalizedTextService textService, out UrlInfo urlInfo) { // test for collisions on the 'main' url var uri = new Uri(url.TrimEnd('/'), UriKind.RelativeOrAbsolute); @@ -130,9 +167,11 @@ namespace Umbraco.Web.Routing var pcr = publishedRouter.CreateRequest(umbracoContext, uri); publishedRouter.TryRouteRequest(pcr); + urlInfo = null; + if (pcr.HasPublishedContent == false) { - urls.Add(UrlInfo.Message(textService.Localize("content/routeErrorCannotRoute"), culture)); + urlInfo = UrlInfo.Message(textService.Localize("content/routeErrorCannotRoute"), culture); return true; } @@ -151,7 +190,7 @@ namespace Umbraco.Web.Routing l.Reverse(); var s = "/" + string.Join("/", l) + " (id=" + pcr.PublishedContent.Id + ")"; - urls.Add(UrlInfo.Message(textService.Localize("content/routeError", new[] { s }), culture)); + urlInfo = UrlInfo.Message(textService.Localize("content/routeError", new[] { s }), culture); return true; } diff --git a/src/Umbraco.Web/Umbraco.Web.csproj b/src/Umbraco.Web/Umbraco.Web.csproj index 699c985af3..6700c0b182 100755 --- a/src/Umbraco.Web/Umbraco.Web.csproj +++ b/src/Umbraco.Web/Umbraco.Web.csproj @@ -167,6 +167,7 @@ +