Consistency for Nullable reference types in LINQ extensions methods

[DanDiplo]

Which exact Umbraco version are you using? For example: 9.0.1 - don't just write v9

10.0.1

Bug summary

In the LINQ extensions methods there are inconsistencies as to whether methods are marked to return null. For example the Children() extension method on IPublishedContent is marked as nullable but Descendants() isn't.

Specifics

In the class Umbraco.Extensions.FriendlyPublishedContentExtensions then these methods are marked as nullable reference types:

public static IEnumerable<T>? Children<T>(this IPublishedContent content, string? culture = null)

public static IEnumerable<T>? SiblingsAndSelf<T>(this IPublishedContent content, string? culture = null)

But the Descendant methods are marked so they can never return null eg.

public static IEnumerable<IPublishedContent> Descendants(this IPublishedContent content, string? culture = null)

This seems inconsistent. I believe it would be better if all extension methods that return collections could never return null but instead always returned an empty collection instead (i.e. Enumerable.Empty<T>()).

This would make chaining LINQ calls much simpler. For instance, if you write this method and have compile-time checking for nullable reference types you get a warning or error:

var results = CurrentPage.Children().Where(x => x.TemplateId.HasValue);

The compiler will say that Children() could be null which makes writing the query difficult. But if it was marked as not nullable and always returned an empty collection for children it would work. But you can write:

var results = CurrentPage.Descendants().Where(x => x.TemplateId.HasValue);

That doesn't emit any warnings. So there should be consistency.

I'm happy to make a PR for this, but wondered first if there are any good reasons or issues that I should be aware of?

Steps to reproduce

Perform a LINQ query with Children() and Descendants() when nullable reference type checking is enabled - https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references

Expected result / actual result

They should be consistent in whether they can return null or not.

[nul800sebastiaan]

Thanks @DanDiplo - we did indeed miss some things and we've announced that for v11 we want to update this: https://github.com/umbraco/Announcements/issues/3

I believe it would be better if all extension methods that return collections could never return null but instead always returned an empty collection instead (i.e. Enumerable.Empty<T>()).

Agreed!

We would love your help there and thanks for the offer! We have a v11/dev branch that you can branch off of to start making changes for a PR.

[github-actions[bot]]

Hi @DanDiplo,

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 :-)

[CarlSargunar]

This sounds indeed like a useful fix - I see the mentioned PR https://github.com/umbraco/Umbraco-CMS/pull/12703 has some merge conflicts, is this PR still in progress? Can I offer any help with it?

[nul800sebastiaan]

Fixed in https://github.com/umbraco/Umbraco-CMS/pull/12703 for v11