Remove private toBool methods and use Object extension method instead

[bjarnef]

Prerequisites

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

Description

I noticed we have re-invented the wheel several places to check for a truthy value by creating private toBool() methods.
However we already have the extension method toBoolean() on Object, which already is used most places.
https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Web.UI.Client/lib/umbraco/Extensions.js#L340-L357

It might be a breaking change as the current implementation return true for numeric values greather than 0, where I think the Object.toBoolean() works similar to Convert.ToBoolean() in .NET although it also handle null and undefined values by returning false for these values.

Furthermore with the current implementation a string value True or TRUE would be handle as false since it only match on true value.

[nathanwoulfe]

@bjarnef I think the method in Extensions needs to be updated - a non-zero number should return true, as that's javascript's native behaviour so as it stands now Object.toBoolean(2) returns a different result to new Boolean(2), or even just to type coercion (which is a bit harder to read):

const x = 2;
const a = Object.toBoolean(x) // false
const b = Boolean(x) // true
const c = !!x // true

Would need to check all the usages of Object.toBoolean, but I have a feeling it's only going to be passed '1' or '0', to convert property values/prevalues to bools...

[bjarnef]

@nathanwoulfe not sure we can change the existing extension methods as it will probably affect the existing places, where it is used and maybe some packages already use this extension.

In C# Convert.ToBoolean() will throw an error for string representation of int values, e.g. 0 and 1. I guess that is why we use TryConvertTo many places in core. Not sure if "2".TryConvertTo<bool>() would return true or false though.

Currently the string extension method only return true for a boolean value (true) and "1", 1 or string values like "true", "TRUE", "True".

The local function removed in this PR however returns true for all positive numbers and false for negative number incl. zero.

But I think it would be difficult to change this - either the behavior where the local function was used or changing the string extension method - or both if we want a non-zero number should return true.

In any case it could be used in projects and by package developers, so I guess it would be a breaking change.
// cc @nul800sebastiaan

[nathanwoulfe]

Yeah, it's a strange one - probably should have been more specific when the extension was named, given it's only used internally for managing property values/prevalues rather than a generic converter, because it behaves differently to how JS would internally manage converting to Boolean and may return unexpected results.

All good though, I won't be losing any sleep over it! Something to avoid in the backoffice rebuild though.

[bjarnef]

@nathanwoulfe if using a numeric value in the orderBySystemField I would probably expect 0, "0", 1 0r "1" to be used, but default value if true in these resources. So it would be an issue if developers used the resources and passed in options.orderBySystemField = 1000, which with these changes would return false, but previous true.

options.orderBySystemField = "True" would also previous return false since it didn't take into account casing.

However any of these values would still work as previous: 0, 1, "0", "1", true, false, "true", "false".

[nul800sebastiaan]

Yeah, I was reviewing before reading the comments, this has the potential to blow up as a big breaking change for some usages unfortunately. I'll close this one, we're kinda stuck with what we have now.