Fix #12454 by having Coalesce handle null values

[arknu]

Prerequisites

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

This fixes #12454

Description

This makes the Coalesce method able to handle null values again. This seems to have accidentally been removed in v10. Since the whole point of the Coleasce method is to choose the first non-null value from a list, this is a problem.
This PR uses the null coalescing operator to handle null values.

To test, call Html.Coleasce in a view, with one of the values being null. Without this PR, it will crash. With this PR, it should work like v9 and return the non-null value.

[github-actions]

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

[bergmania]

Thanks @arknu 💪

[mikecp]

Thanks for spotting and solving this @arknu 👍

I maybe have a question or two for you and @bergmania as well before merging.

I am wondering if the switch of parameter to object?[] instead of object[] is really necessary. I think that x?.ToString() will work on "object" as well + wouldn't that be a breaking change @bergmania ?

Also, if I may suggest, I would put it all in the FirstOrDefault clause:

var arg = args
            .FirstOrDefault(x => string.IsNullOrWhiteSpace(x?.ToString()) == false)

This way, we check the param arguments one by one and stop at the first one that fits, and therefore we avoid building first a list with all of them transformed ToString and then going through that full list to find the first non-null.

What do you think? 😁

Cheers!

[bergmania]

I am wondering if the switch of parameter to object?[] instead of object[] is really necessary. I think that x?.ToString() will work on "object" as well + wouldn't that be a breaking change @bergmania ?

I guess in a Coalesce method it is expected some arguments can be null? If the object[] is not changed to object?[] it cannot really be null? When nullable references types are enabled, object cannot be null. So yes, it is a breaking changes since 10rc, but is actually reverting a breaking change since v9

[arknu]

@bergmania @mikecp With nullable reference types enabled (which it is by default in v10) the object?[] annotation is needed in order to pass null values to the method without getting a warning from the compiler. It works anyway, of course, since the annotations don't affect runtime behavior, but when the method description says that you can pass null in the arguments, the annotation needs to match that.

@mikecp Regarding your second suggestion, yes that would work as well. I was mainly looking to make the smallest change possible, since v10 is close to release. And also trying to match all the other helper methods. I think that if it should be changed, that should be a separate PR for all the helper methods at once.

[mikecp]

Hey @arknu, @bergmania ,

Thanks for your quick feedback about the nullable reference types (was indeed forgotten), and it's indeed good to remain consistent with the other helper methods 👍

So let's merge this now 🎉

[mikecp]

I do think, however, that normally we now target v10.1.0 for the community contributions, but I let @bergmania and/or @nul800sebastiaan decide if this one is a candidate for the initial v10.0.0 release 😁

[arknu]

@mikecp @bergmania I think this needs to be in 10.0. For me it broke several sites when I upgrade, since we use coalesce quite heavily.

[bergmania]

I'll cherry pick for 10.0