Querying extensions: Allow ContentAtRoot() to accept culture

[bjarnef]

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 #20117

Description

This PR allow ContentAtRoot() to query specific culture just like GetAtRoot() in published content query.

[github-actions]

Hi there @bjarnef, 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 🤖 🙂

[Copilot]

Pull Request Overview

This PR enhances the ContentAtRoot() method to accept an optional culture parameter, allowing developers to query root content for specific cultures, bringing consistency with the existing GetAtRoot() method in published content query.

Key Changes:

• Added culture parameter support to ContentAtRoot() method across the API surface
• Updated documentation comments to reference the correct method name

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Umbraco.Web.Common/UmbracoHelper.cs	Added overload of ContentAtRoot() with optional culture parameter
src/Umbraco.Infrastructure/IPublishedContentQuery.cs	Added interface method signature for culture-aware ContentAtRoot()
src/Umbraco.Infrastructure/PublishedContentQuery.cs	Implemented culture-aware ContentAtRoot() and updated helper method
src/Umbraco.Web.Common/Extensions/FriendlyPublishedContentExtensions.cs	Updated documentation comments to reference correct method name
src/Umbraco.Core/Extensions/PublishedContentExtensions.cs	Updated documentation comments to reference correct method name

[bjarnef]

@AndyButland any chance this can be included in next minor of v13?
https://our.umbraco.com/download/releases/13110

[emmagarland]

Hi @bjarnef ,

Thanks a lot for your PR to fix #20117, where the ContentAtRoot() and GetAtRoot() return null if the content varies by culture.

One of the Core Collaborators team will review this as soon as possible - I've noticed you've tagged Andy on this, so we'll likely wait to see what comes out of that conversation :)

Best regards

Emma

[Zeegaan]

Its breaking to refactor the method (even with a default parameter), we should probably just make an overload that takes a culture instead 😁

[bjarnef]

@Zeegaan I wondered about that, but it didn't seem to break the existing usage it the template of the "old" method.
Actually Copilot suggested that change 😅 (I know it is not always correct).

I am not sure in which case it will break though as the parameter is optional and it didn't have any before, but otherwise we can add an overload as I originally did.

[bjarnef]

@Zeegaan perhaps it also make sense to include this in v16 as contextReference.UmbracoContext.Content.GetAtRoot() is obsolete.

_publishedContentQuery.ContentAtRoot() is still valid to use, but it doesn't allow to query a specific culture like GetAtRoot() did 🙈

[umbracocommunity]

This pull request has been mentioned on Umbraco community forum. There might be relevant details there:

https://forum.umbraco.com/t/replacement-for-ipublishedcache-getatroot/5874/1

[AndyButland]

Thanks @bjarnef - I had a quick look over. To resolve the breaking change that's being flagged in conversation and in the automated checks, I think you'll need to have the interface like this:

IEnumerable<IPublishedContent> ContentAtRoot();

IEnumerable<IPublishedContent> ContentAtRoot(string culture) => throw new NotSupportedException();

That way the old method is still there and the new method has a default implementation so it won't be binary breaking in the rare chance that someone has a custom implementation of IPublishedContentQuery. By making the culture parameter not nullable, we also avoid an ambiguous method exception.

Then in PublishedContentQuery you could implement like this:

public IEnumerable<IPublishedContent> ContentAtRoot() => ItemsAtRoot(_publishedSnapshot.Content);

public IEnumerable<IPublishedContent> ContentAtRoot(string culture) => ItemsAtRoot(_publishedSnapshot.Content, culture);

You need similar on UmbracoHelper as that's also a public class so changing a method parameter is a binary breaking change.

[bjarnef]

@AndyButland I have modified the code with overload ContentAtRoot(string? culture = null) .. I wonder if this makes a difference from ContentAtRoot(string culture)?

Seems to align with GetAtRoot(string? culture = null) and in template I can use Umbraco.ConentAtRoot("da-DK"), or Umbraco.ConentAtRoot(null) which is the same as Umbraco.ConentAtRoot().

@inject Umbraco.Cms.Core.Web.IUmbracoContextAccessor ContextAccessor
@inject Umbraco.Cms.Core.IPublishedContentQuery ContentQuery
@{
    // UmbracoHelper
    var test1 = Umbraco.ContentAtRoot().FirstOrDefault();
    var test2 = Umbraco.ContentAtRoot(null).FirstOrDefault();
    var test3 = Umbraco.ContentAtRoot("da-DK").FirstOrDefault();

    // IPublishedContentQuery 
    var test4 = ContentQuery.ContentAtRoot().FirstOrDefault();
    var test5 = ContentQuery.ContentAtRoot(null).FirstOrDefault();
    var test6 = ContentQuery.ContentAtRoot("da-DK").FirstOrDefault();

    // IPublishedContentCache
    var test7 = ContextAccessor.GetRequiredUmbracoContext().Content.GetAtRoot().FirstOrDefault();
    var test8 = ContextAccessor.GetRequiredUmbracoContext().Content.GetAtRoot(null).FirstOrDefault();
    var test9 = ContextAccessor.GetRequiredUmbracoContext().Content.GetAtRoot("da-DK").FirstOrDefault();
}

[AndyButland]

I would have thought that would have led to an ambiguous method runtime issue, as a call to ContentAtRoot() could be served by both ContentAtRoot() and ContentAtRoot(string? culture = null).

[bjarnef]

@AndyButland I didn't seen any errors.

@{
    var test1 = Umbraco.ContentAtRoot().FirstOrDefault();
    var test2 = Umbraco.ContentAtRoot(null).FirstOrDefault();
    var test3 = Umbraco.ContentAtRoot("da-DK").FirstOrDefault();

    var test4 = ContentQuery.ContentAtRoot().FirstOrDefault();
    var test5 = ContentQuery.ContentAtRoot(null).FirstOrDefault();
    var test6 = ContentQuery.ContentAtRoot("da-DK").FirstOrDefault();

    var test7 = ContextAccessor.GetRequiredUmbracoContext()?.Content?.GetAtRoot().FirstOrDefault();
    var test8 = ContextAccessor.GetRequiredUmbracoContext()?.Content?.GetAtRoot(null).FirstOrDefault();
    var test9 = ContextAccessor.GetRequiredUmbracoContext()?.Content?.GetAtRoot("da-DK").FirstOrDefault();    
}

Test 1: @test1.Name<br />
Test 2: @test2.Name<br />
Test 3: @test3.Name<br />
Test 4: @test4.Name<br />
Test 5: @test5.Name<br />
Test 6: @test6.Name<br />
Test 7: @test7.Name<br />
Test 8: @test8.Name<br />
Test 9: @test9.Name<br />

Inspecting Umbraco.ContentAtRoot() in VS goes to first/original method, the two others to the new overload.

I guess the use of the second overload without parameter isn't really used directly.
Alternatively I guess ContentAtRoot(string? culture) would be correctly allowing to pass in null value.

In notification handler:

[bjarnef]

ChatGPT says it cause ambiguous method references, but as showns in examples above it compiles fine and works in razor too.

[bjarnef]

Could there be changes in .NET 8 and newer C# that makes this work? 😅

So the answer now
Older compilers (pre-.NET 7 era) → ambiguous.
Modern compilers (.NET 7 / .NET 8) → no ambiguity; exact-match overload wins.

[bjarnef]

A bit more info and linking to this article: https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/named-and-optional-arguments

[AndyButland]

Nice bit of investigative work you and your AI buddy have done there. Sounds convincing.

So if you can't find any issues with what you have we can go with that, but can then modify the interface as follows (as long-term, there's no point keeping the overload taking no parameters), and we can have a default implementation that delegates to the orginal.

[Obsolete("Please use the method overload taking a culture parameter. Scheduled for removal in Umbraco 18.")]
IEnumerable<IPublishedContent> ContentAtRoot();

IEnumerable<IPublishedContent> ContentAtRoot(string? culture = null) => ContentAtRoot();

[bjarnef]

I have added [Obsolete] attribute, but does it make sense, as Umbraco.ContentAtRoot() and ContentQuery.ContentAtRoot() is still valid when the old method is removed as it has an optional parameter - so it is valid to use parameterless method?

Not sure if the obsolete is actually necessary? It seems a bit misleading as the parameterless method will still work.

Was it a typo here? I don't see it use culture parameter:

IEnumerable<IPublishedContent> ContentAtRoot(string? culture = null) => ContentAtRoot();

If I just remove the ContentAtRoot() at root from UmbracoHelper without any warning, it still works:

and inspecting the method goes to the new one:

[Copilot]

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[bjarnef]

@AndyButland shouldn't it be ContentAtRoot(string? culture) instead of ContentAtRoot(string culture)?

[AndyButland]

Sorry, missed updating with a comment as well as pushing the update.

---

Not sure if the obsolete is actually necessary? It seems a bit misleading as the parameterless method will still work.

Yes, I see what you mean. Again the confusion comes from the ambiguous methods - even if the compiler does understand it, it's a bit odd for the human reading the code I agree.

In an ideal world we would just remove the one with no parameters, but given we can't within 13 for backward compatibility reasons, on balance think it's best we go for the "old school" overloads that we had earlier. That'll be quite clear from intellisense. The only thing is you can't pass "null" as the culture parameter, but I think that's OK when you have a clear overload to use that just doesn't take this parameter.

I've pushed that to your branch. Please let me know if you are happy with that and it works as you expect in your tests, and then we can include.

shouldn't it be ContentAtRoot(string? culture) instead of ContentAtRoot(string culture)?

Yes, that'll work actually and restore the ability to pass null. I've updated to that now too.

[bjarnef]

In an ideal world we would just remove the one with no parameters, but given we can't within 13 for backward compatibility reasons, on balance think it's best we go for the "old school" overloads that we had earlier. That'll be quite clear from intellisense. The only thing is you can't pass "null" as the culture parameter, but I think that's OK when you have a clear overload to use that just doesn't take this parameter.

Yeah, I am note sure what would happen as ContentAtRoot(string? culture = null) would still allow using the parameterless method. It seems the compiler it at bit smarter in modern .NET, but I think it is fine to go with this safer approach for now.

The changes looks fine to me :)

I had a brief look at the IDocumentNavigationQueryService as alternative to GetAtRoot() (which is obsolete in v16) and one of the reasons for this PR.
https://forum.umbraco.com/t/replacement-for-ipublishedcache-getatroot/5874

It seems IDocumentNavigationQueryService can be used, but one may lookup content from cache anyway unless ids/Guids are only needed.

[AndyButland]

Cherry-picked via 2c3a2e2 to 16.

[bjarnef]

@AndyButland great, no more TypedContent 😅

or searching code base I found these.
https://github.com/search?q=repo%3Aumbraco%2FUmbraco-CMS%20TypedContent&type=code

I guess it is broken anyway as Umbraco.TypedContent() doesn't exist 😎

[umbracocommunity]

This pull request has been mentioned on Umbraco community forum. There might be relevant details there:

https://forum.umbraco.com/t/replacement-for-ipublishedcache-getatroot/5874/2

[bjarnef]

@AndyButland it seems this has been included in v16, but not v17 yet:
https://forum.umbraco.com/t/replacement-for-ipublishedcache-getatroot/5874/6?u=bjarnef

Umbraco 16
https://github.com/umbraco/Umbraco-CMS/blob/v16/dev/src/Umbraco.Infrastructure/PublishedContentQuery.cs#L144-L148

Umbraco 17
https://github.com/umbraco/Umbraco-CMS/blob/main/src/Umbraco.Infrastructure/PublishedContentQuery.cs#L168-L169

However in v17 it no longer use GetAtRoot(culture) - not sure how this part can handle culture:

private IEnumerable<IPublishedContent> ItemsAtRoot(IPublishedCache? cache)
        => _documentNavigationQueryService.TryGetRootKeys(out IEnumerable<Guid> rootKeys) is false ? []
            : rootKeys.Select(x => cache?.GetById(false, x)).WhereNotNull()

as in v16:

private static IEnumerable<IPublishedContent> ItemsAtRoot(IPublishedCache? cache, string? culture = null)
        => cache?.GetAtRoot(culture) ?? Array.Empty<IPublishedContent>();

and IPublishedCache doesn't have any method to fetch a specific node by culture, e.g. it home page node is published in English, but not created (or published) in Danish.
https://github.com/umbraco/Umbraco-CMS/blob/main/src/Umbraco.Core/PublishedCache/IPublishedCache.cs#L8

[redmorello]

Yes, both IPublishedContentQuery & UmbracoHelper ContentAtRoot both seem to be missing the culture parameter.

[AndyButland]

Yes, I think I may have run into this problem when merging the improvement up from 13, via 16 to 17, got stuck (and then presumably moved onto other things). So although we added it late as a feature for 13, as the existing cache architecture supported it quite easily, it wasn't feasible to bring it up for 17. Seems to me it needs a fair bit of rework to support, and so at the moment, looks like it's necessary to get all root items and filter for the culture afterwards.