TrueFalse data type's initial value not respected by YesNoPropertyValueConverter

[jamiepollock]

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

10.5.1

Bug summary

The built in Umbraco TrueFalse data type's initial value prevalue is not respected by the YesnoPropertyValueConverter.

Specifics

I discovered this issue when creating a block list which had a boolean property for visibility in the settings which had an initial state of true.

I noticed if a user did not actively view the settings for the block item that the value would be false instead of the expected true value from the data type prevalues.

Steps to reproduce

1. Create a TrueFalse data type with initial value set to true
2. Create a content element content type with any properties
3. Create a settings element content type with the data type created in step 1
4. Create a block list data type with a configuration that uses the element content types from steps 2 & 3
5. Create a content type that uses the block list data type from step 4
6. Create content using the content type from step 5
7. Create a block in the content but do not set the settings for this block
8. Write code which access these block list property and output the value of the TrueFalse setting

Expected result / actual result

Expected

A property which has no CMS provided value should return the data type's configured initial prevalue.

Actual

A property which has no CMS provided value returns false.

[github-actions[bot]]

Hi there @jamiepollock!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

• We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
• If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
• We'll replicate the issue to ensure that the problem is as described.
• We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

[jamiepollock]

Fortunately I think the fix is fairly simple. It would be a case of updating this line:

https://github.com/umbraco/Umbraco-CMS/blob/33adbf41fa1f5c5d0759c70a7116114107addf56/src/Umbraco.Core/PropertyEditors/ValueConverters/YesNoValueConverter.cs#L55

To something like:

var config = propertyType.DataType.ConfigurationAs<TrueFalseConfiguration>();

return config is null ? false : config.Default;

[jamiepollock]

I've now added a PR to fix this here: https://github.com/umbraco/Umbraco-CMS/pull/14305.

It looked like my initial code snippet didn't quite cover all the angles. Fortunately the tests project showed me the edge cases I missed.

[jamiepollock]

While I initially reported this as a v10 bug it likely affects v11 and v12 too as the implementation of YesNoValueConverter has not changed.

[abjerner]

I think this is similar to the issue originally reported by @sebastiandammark: https://github.com/umbraco/Umbraco-CMS/issues/10160

Back then, a problem in fixing this was that for Umbraco 8, the TrueFalseConfiguration is located in the Umbraco.Web project, while the YesNoValueConverter class is located in the Umbraco.Core project. So YesNoValueConverter doesn't know about TrueFalseConfiguration. Umbraco 9+ seems to have fixed that so they are in the same namespace, so now a fix is possible with involving lots of refactoring.

But as mentioned by @nul800sebastiaan in the original issue, a change like this is seen as a breaking change. So we should we careful about making a change like this.

Since this would be a breaking change, I created the Limbo Boolean package so we could have the expected behavior even if it wasn't addressed directly in Umbraco.

[kjac]

Hi @jamiepollock,

Thank you for all your work here 👍

As @abjerner correctly points out, this is a functionally breaking change. The fix would be nice to get in, though, so I'll see if we can include it in V12.

[nul800sebastiaan]

Closing for now with an explanation added to the PR: https://github.com/umbraco/Umbraco-CMS/pull/14305#issuecomment-1564034910

[jamiepollock]

Hey folks. Ah that’s regrettable. I respect the reasoning.

Fortunately the PVC system is pluggable so I’ve already replaced the behavior on my project with local code until a core feature is added later on.

Thank you for the quick turn around in feedback.

[Nicholas-Westby]

@nul800sebastiaan In response to this:

We're discussing at HQ when we can, for example, make SuperValueConverters and/or Limbo Boolean part of the core in v14 and mark it as breaking.

Is there any reason this change can't be included in v13 (considering it hasn't been released yet)? Aren't major version changes the right time to make a "breaking" change?

Also, I wouldn't say it "works with quirks". People are having to build around this, and they're having to spend time on finding the solution (whether that be a custom fix or using some plugin they aren't initially aware of). It's hard to imagine a scenario where fixing this would break someone's implementation. Can you think of a realistic scenario where it would break their implementation that is remotely likely?

My perspective is that the only time we change the output is when the default value configured on the boolean configuration screen is true and, I think, when the property editor is not initially viewable (such as when it's in a settings screen). Given how uncommon that is and how unlikely it is that someone would want the value to be false by default in that situation, it seems like fixing this makes a lot of sense, even if it has to wait for v13.

[nul800sebastiaan]

@Nicholas-Westby Our deprecation policy is to obsolete things / announce a breaking at least 2 major versions ahead of time v12-RC is out, which means anything we want to break now will need to wait until v14.

I didn't emphasize enough on the PR that there are quite a lot of quirks in the various PVCs right now and we'd like to see if we can come up with something coherent for all of them so that workarounds that people have been making for years are no longer necessary. And as noted, there's packages with much more consistent behavior that you can use until then.

[jamiepollock]

@nul800sebastiaan thanks for this clarification. I agree the PVCs are a useful system to handle quirks and edge cases by the developer themselves or through a package.

Hopefully in the future the core PVC system will do a better job at handling these so developer intervention isn't required.

[Nicholas-Westby]

@nul800sebastiaan I wouldn't say fixing this issue fits within the deprecation policy as this isn't something that is being deprecated, and the upgrade section that mentions breaking changes (if this could even be considered one) doesn't outline anything like the 2 major version announcement you mentioned (i.e., that policy is purely for deprecations). On top of that, it's a very gray area regarding whether or not a release candidate should be considered the cutoff point for a release.

In any event, it's good to hear something is being planned to revamp the core PVCs, even if it will be a several years before it will be incorporated into a LTS release (v16 around mid to late 2025, I imagine).