Make GetByIds in ContentService extensions non nullable

[bjarnef]

Prerequisites

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

Description

In this PR #15289 I noticed GetByIds() extensions for IContentService and IMediaService wasn't consistent.

I have changes this for v14 and at the same time I noticed redundant and inconsistency or PackageMedia() method. Ideally this method should be moved, so it isn't duplicated and since it is private at both usage.

[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 🤖 🙂

[bjarnef]

@nul800sebastiaan @bergmania

PackagesRepository and CreatedPackageSchemaRepository both inherits ICreatedPackagesRepository, but PackagesRepository is marked as obsolete.

Not sure if it can be removed in v14 though or should it mention when is intended to be removed? Most obsolete methods mention v13, v14 etc.

But searching the codebase it isn't really consistent https://github.com/search?q=repo%3Aumbraco%2FUmbraco-CMS%20obsolete&type=code v13 or V14 or Umbraco 14 in Obsolete attribute. In other part it is just mentioned as comment, but may not be clear to developers implementing these method (besides they of course in major version will notice the method is obsolete).

It wonder if it is possible to extend ObsoleteAttribute - would be nice if it optionally just need to specify a version perhaps:
dotnet/runtime#33089

Otherwise agree on a consistent format to include version. It was a bit off-topic for changes in this PR :)

[JasonElkin]

I'm not sure what you're trying to achieve in terms of making things "consistent", but the nullability in the code is correct as it stands and shouldn't be changed just because other related methods happen to be nullable.

The point of nullable reference types is so that developers consuming our code can know at compile-time what references can and cannot be null and avoid unnecessary null checks.

[JasonElkin]

Sorry, previous review missed this comment out.

[bjarnef]

Well it media can't be nullable, how can content be then?

Umbraco-CMS/src/Umbraco.Core/Services/ContentServiceExtensions.cs

Line 22 in 1e6ada7

public static IEnumerable<IContent>? GetByIds(this IContentService contentService, IEnumerable<Udi> ids)

The extensions are pretty much doing the same thing looking up content/media from cache.

Content

Umbraco-CMS/src/Umbraco.Core/Services/IContentService.cs

Lines 100 to 108 in a7cacc9

	/// <summary>
	/// Gets documents.
	/// </summary>
	IEnumerable<IContent> GetByIds(IEnumerable<int> ids);
	/// <summary>
	/// Gets documents.
	/// </summary>
	IEnumerable<IContent> GetByIds(IEnumerable<Guid> ids);

Media

Umbraco-CMS/src/Umbraco.Core/Services/IMediaService.cs

Lines 19 to 21 in a7cacc9

	IEnumerable<IMedia> GetByIds(IEnumerable<int> ids);
	IEnumerable<IMedia> GetByIds(IEnumerable<Guid> ids);

At a second look, it should probably be the GetByIds() extension for IContentService which shouldn't be nullable.

Now we also have these, which are both non-nullable:

IEnumerable<IContent> GetByIds(IEnumerable<Udi> ids); 

IEnumerable<IMedia> GetByIds(IEnumerable<Udi> ids); 

as these both call the underlying GetByIds(IEnumerable<Guid> ids) which is non-nullable.

[JasonElkin]

At a second look, it should probably be the GetByIds() extension for IContentService which shouldn't be nullable.

I think that's it @bjarnef - looking at the commit history, the nullability of the service was changed more recently and the extension method hasn't been updated to match.

[bjarnef]

Yes :) regarding the PackageMedia() it seem the non-obsolete class was updated at some point, but not the same method in the obsolete class, so I will leave that for now.

[nul800sebastiaan]

All good now, thanks for this one @bjarnef! 👍