From e959605b39b3e9d2eb67ea526df91dab4b4bd27f Mon Sep 17 00:00:00 2001 From: Stephan Date: Thu, 20 Dec 2018 14:31:46 +0100 Subject: [PATCH] Cleanup and deal with FIXMEs --- src/Umbraco.Web/Routing/DomainHelper.cs | 30 ++++++------------- src/Umbraco.Web/Routing/UrlProvider.cs | 5 +--- .../Routing/UrlProviderExtensions.cs | 16 +++++----- 3 files changed, 18 insertions(+), 33 deletions(-) diff --git a/src/Umbraco.Web/Routing/DomainHelper.cs b/src/Umbraco.Web/Routing/DomainHelper.cs index 9b300009d0..edfcb33aa5 100644 --- a/src/Umbraco.Web/Routing/DomainHelper.cs +++ b/src/Umbraco.Web/Routing/DomainHelper.cs @@ -30,10 +30,10 @@ namespace Umbraco.Web.Routing /// The culture, or null. /// The domain and its uri, if any, that best matches the specified uri and culture, else null. /// - /// f at least a domain is set on the node then the method returns the domain that + /// If at least a domain is set on the node then the method returns the domain that /// best matches the specified uri and culture, else it returns null. - /// If culture is null, uses the default culture for the installation instead. - /// fixme not exactly - if culture is !null, we MUST have a value for THAT culture, else we use default as hint + /// If culture is null, uses the default culture for the installation instead. Otherwise, + /// will try with the specified culture, else return null. /// internal DomainAndUri DomainForNode(int nodeId, Uri current, string culture = null) { @@ -49,13 +49,9 @@ namespace Umbraco.Web.Routing return null; // else filter - var domainAndUri = SelectDomain(domains, current, culture, _domainCache.DefaultCulture, + // it could be that none apply (due to culture) + return SelectDomain(domains, current, culture, _domainCache.DefaultCulture, (cdomainAndUris, ccurrent, cculture, cdefaultCulture) => _siteDomainHelper.MapDomain(cdomainAndUris, ccurrent, cculture, cdefaultCulture)); - - if (domainAndUri == null) - throw new Exception("DomainForUri returned null."); - - return domainAndUri; } /// @@ -202,19 +198,12 @@ namespace Umbraco.Web.Routing private static IReadOnlyCollection SelectByCulture(IReadOnlyCollection domainsAndUris, string culture, string defaultCulture) { + // we try our best to match cultures, but may end with a bogus domain + if (culture != null) // try the supplied culture { 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}\"."); - //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 @@ -230,13 +219,12 @@ namespace Umbraco.Web.Routing { DomainAndUri domainAndUri; + // we try our best to match cultures, but may end with a bogus domain + if (culture != null) // try the supplied culture { domainAndUri = domainsAndUris.FirstOrDefault(x => x.Culture.Name.InvariantEquals(culture)); if (domainAndUri != null) return domainAndUri; - - // 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}\"."); } if (defaultCulture != null) // try the defaultCulture culture diff --git a/src/Umbraco.Web/Routing/UrlProvider.cs b/src/Umbraco.Web/Routing/UrlProvider.cs index 491deee9e3..7ed530093c 100644 --- a/src/Umbraco.Web/Routing/UrlProvider.cs +++ b/src/Umbraco.Web/Routing/UrlProvider.cs @@ -245,10 +245,7 @@ namespace Umbraco.Web.Routing /// public IEnumerable GetOtherUrls(int id, Uri current) { - // providers can return null or an empty list or a non-empty list, be prepared - var urls = _urlProviders.SelectMany(provider => provider.GetOtherUrls(_umbracoContext, id, current)); - - return urls; + return _urlProviders.SelectMany(provider => provider.GetOtherUrls(_umbracoContext, id, current)); } #endregion diff --git a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs index de9314704b..8db657ed30 100644 --- a/src/Umbraco.Web/Routing/UrlProviderExtensions.cs +++ b/src/Umbraco.Web/Routing/UrlProviderExtensions.cs @@ -5,7 +5,6 @@ using Umbraco.Core.Models; using Umbraco.Core.Services; using Umbraco.Core; using Umbraco.Core.Logging; -using LightInject; namespace Umbraco.Web.Routing { @@ -38,24 +37,25 @@ namespace Umbraco.Web.Routing { yield return UrlInfo.Message(textService.Localize("content/itemNotPublished")); yield break; - } - - var urls = new HashSet(); + } // build a list of urls, for the back-office // which will contain // - the 'main' urls, which is what .Url would return, for each culture // - the 'other' urls we know (based upon domains, etc) // - // 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 + // need to work through each installed culture: + // 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. + // and, not only for those assigned to domains in the branch, because we want + // to show what GetUrl() would return, for every culture. + var urls = new HashSet(); var cultures = localizationService.GetAllLanguages().Select(x => x.IsoCode).ToList(); //get all URLs for all cultures + //in a HashSet, so de-duplicates too foreach (var cultureUrl in GetContentUrlsByCulture(content, cultures, publishedRouter, umbracoContext, contentService, textService, logger)) { urls.Add(cultureUrl);