Fixed incorrect node url being returned when no suitable domain exist…

[vsilvar]

Prerequisites

• I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #10827

Description

This changes the code to have a behavior close to what is expected while trying not to change to much.
Instead of always falling back to the first registered domain, we simply don't match one and use the default route.

This will more closely follow the default routing behavior and properly show it in the CMS as well.

[umbrabot]

Hi there @vsilvar, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[nikolajlauridsen]

First of all, thanks a lot for another PR, love to see it 🎉

But having done some testing now, I'm not really sure this is what we want to do, while it does make it better in some ways we end up breaking other things, and it still doesn't seem quite right, although it is better.

This is gonna be a bit lengthy, but it's just as much to get some notes down for my self as well 😄

The test setup

Like in the PR I have three languages, with English as the default:

I have some document types that vary by culture.

Only one domain is assigned:

I add this to the template

<br/>
<span>EN is published: @Model.IsPublished("en-us")</span>
<br/>
<span>DK is published: @Model.IsPublished("da")</span>
<br/>
<span>NO is published: @Model.IsPublished("nb")</span>
<br/>


<hr/>
<br/>
<span>EN URL: @Model.Url("en-us")</span>
<br/>
<span>DK URL: @Model.Url("da")</span>
<br/>
<span>NO: @Model.Url("nb")</span>
<br/>

The results

Now before this PR I would see this in the backoffice when I publish:

• EN - /dk/ - but it's accessible through /
• DA - https://localhost:44331/dk/ - Which seems as expected.
• NB - This item is not published - Even though it is published, it's just not routable

and in the rendered templates all cultures return true for .IsPublished, and .Url returns /dk/ for all, which seems weird, but makes sense from the comment:

// no match means that either current does not belong to a site, or the site it belongs to
// does not contain any of domainAndUris. Yet we have to return something. here, it becomes
// a bit arbitrary.

So as I understand it if we can't find anything we more or less just return anything we can, because we must return something.

The problem changing this is that suddenly we don't return anything, so after the changes, if you try to access the en variation through / which you still can, you receive a server error because the filter returns null, when trying to get a URL for @Model.Url("<someCulture>"): InvalidOperationException: The filter returned null.

Another thing that is still weird is that, is that it now in the backoffice say:

• EN - Could not get the URL - but the content is published and accessible through /, but this is still okay, because the domain is not set, which it should be.
• DA - /dk/ and https://localhost:44331/dk/ - As expected, all good
• NB - This item is not published - Even though it is published, it's just not routable, so it should really say "The content is published, but cannot be routed".

So it seems to still be inconsistent but in a different way. I do think this is a step on the ways and kudos for investigating it, but I think we need to take a closer looks at how we can do this in a way where it becomes consistent and that .Url doesn't break, and what it requires.

[vsilvar]

@nikolajlauridsen Thanks for taking the time to look at the PR.
When cleaning up the PR I forgot to include an important part of the fix, causing the erroneous behavior. Sorry about that.

With my latest commit the behavior should now be:
EN - /
DA - /dk/
NB - This document is published but its URL cannot be routed (.Url still returns / though)

I think this improves the current behavior but it's still not perfect due to the last case, which while completely correct in the CMS returns an incorrect Url with .Url

This last case could also be fixed by adding the following in GetUrlFromRoute:

...
var domainUri = ...
if (domainUri == null && !umbracoContext.PublishedSnapshot.Domains.DefaultCulture.InvariantEquals(culture))
{
    return null;
}
...

The downside being that the CMS message will default to the generic:
This document is published but is not in the cache

Please let me know if the behavior makes sense now, and whether the additional fix mentioned above is preferred or not.

[nikolajlauridsen]

Just did some more testing, and the behaviour after your changes is a significant improvement compared to what we currently have. In regards to the behaviour of .Url, it's of course not optimal, however, I think it's fine as it is now since the backofffice message is correct, and this way nothing should spectacularly explode if you don't expect the null, so it's not all bad, and at the end of the day, you really shouldn't have unmapped domains 😄.

I've tried to do as much testing as I can, and all seem to still work out fine, but just with more correct messages 👍 , of course, I can't possibly test every single edge case, so there might be some gremlins, however, I think that if we're going to change this behaviour, now is the time to do it since it's a new major, so I'll go ahead and merge this in.

Amazing work, thanks a lot for your contribution 🎉

[vsilvar]

@nikolajlauridsen
Thank you! 🎉

Also, just created #11032 as this PR was already merged and I noticed I was doing a case sensitive comparison of the cultures.
It still worked, but it's better to keep it consistent to prevent unforeseen issues.

[Shazwazza]

I'm a little worried about the changes here because the functionality is now different from v8. I'm porting over some tests for v8, specifically with nested domains and these changes now make that fail so I fear this has introduced new issues.

Preferably, we need to know if this issue existed in v8 and if so how to fix it there.

@bergmania I think this may cause other underlying issues. The test specifically in v8 that I'm porting over is called UrlsWithNestedDomains.DoNotPolluteCache. This now no longer works in v9. Specifically this logic:

// Set a current URL
const string url = "http://domain1.com/1001-1/1001-1-1";

// ... we then create an Umbraco context with this URL

// Then get the absolute URL for a node that exists under a node that has a domain assigned that is not the current domain
string absUrl = publishedUrlProvider.GetUrl(100111, UrlMode.Absolute);

// This will NOT pass because it's no longer respecting the domain.
Assert.AreEqual("http://domain2.com/1001-1-1/", absUrl);

[Shazwazza]

Actually, this might be an issue with my branch not being merged with the very latest. I'll report back if I still have the issue.