Remove the papercuts for frontend rendering (PropertyValueConverters and beyond)

[nul800sebastiaan]

Umbraco version

v9

Description

Over the years we've not been really good at paying attention to make frontend rendering of different datatypes a consistent experience leading to quite a few papercuts over time. Anything can easily be worked around of course but wouldn't it be great if everything "just" worked like you would expect it to.

It's always painful to have to close these issues and patiently explain that we're stuck with this wonky behavior. 😓

We have an opportunity now with the upcoming v9 to do something about it. Why not in v8? Well, all of these changes will introduce breaking changes for people relying on the current behavior. So each of the things listed below would be a breaking change to a significant amount of people.

With v9, people will have to rewrite their views anyway, so why not take the opportunity to make it more consistent!

This work doesn't really fit into the HQ roadmap, so we are seeking assistance from contributors in 2 areas:

1. Listing the things that are not behaving as expected, and getting to a conclusion as to how they should behave
2. Implementing the correct behavior

To get us started, I've been looking into all the issues we had to close because fixing them would mean a behavioral breaking change.

• StripHtmlTags does not remove HTML tags (if first tag is a block level element) StripHtmlTags does not remove HTML tags (if first tag is a block level element) #8789
• Default-value of a Checkbox-property is not used in YesNoValueConverter Default-value of a Checkbox-property is not used in YesNoValueConverter #7409
  / Existing instances are not updated if property (True/false) with default true is added to existing documenttype Existing instances are not updated if property (True/false) with default true is added to existing documenttype #9649
• V8 .GetCropUrl("CropAlias") generates invalid ImageProcessor URL V8 .GetCropUrl("CropAlias") generates invalid ImageProcessor URL #8116
• Numeric properties cannot be nullable Numeric properties cannot be nullable #10369
• Property value converter for Eye Dropper Color Picker Property value converter for Eye Dropper Color Picker #10323
• V8 - Model.Value() default value doesn't show if string is empty V8 - Model.Value() default value doesn't show if string is empty #6680
• Umbraco Multi Url Picker - Recursive not working in RTE Umbraco Multi Url Picker - Recursive not working in RTE #6111

I am not too sure about this one, haven't investigated yet: Textarea Macro Parameter Not Rendering HTML Markup #6286

Then there's also the package SuperValueConverters by @callumbwhyte: https://our.umbraco.com/packages/developer-tools/supervalueconverters/. That package works with:

• Multi-Node Tree Picker
• Nested Content

So what else is missing from this list?
For each item, can we agree what the correct behavior should be, so we can work on it for the upcoming v9?

Let's discuss!

[nul800sebastiaan]

Ah I see there's one from @ronaldbarendse as well: Our.Umbraco.GridValueConverters https://our.umbraco.com/packages/developer-tools/ourumbracogridvalueconverters/

[ronaldbarendse]

The Our.Umbraco.GridValueConverters package is only compatible with Umbraco 7 and although it works really well (it is used in a couple of live sites), it probably changed too much to the existing grid functionality to really recommend it and get more people to use it 😑

[nul800sebastiaan]

I don't know how relevant this v7 one from @Jeavon is any more: Umbraco Core Property Value Converters v3 https://github.com/Jeavon/Umbraco-Core-Property-Value-Converters

[Jeavon]

Not really in itself as it was merged into v7.6. What they did has been standard ever since.

[marcemarc]

Predominantly over the years the number one confusion I see people running into is how to test if something is empty or null... (or if the property has been added)

... eg HasValue and HasProperty were useful for this (when using IPublishedContent and stringly typed models) but it's less clear with Modelsbuilder?
Model.HasValue(Model.GetModelPropertyType(p=>p.PageTitle).PropertyTypeAlias) etc

HasValue doesn't always work for more complex editors, eg the grid, where if a previously used grid is cleared in the backoffice it can still have json residing in it... and still appear to 'have a value' when it doesn't

Additionally, I see lots of code testing whether an integer is null, instead of > 0,
or testing if a string is null, instead of using String.IsNullOrWhitepace/empty etc
or things erroring when they are not tested for null at all...
... calling .Any() on a null collection

Anyway it's a bit of a mismatch of helpers and .net things that differ from one property to another...

and I'm thinking it would be easier if there was a consistency between each property and the experience of accessing the data, or checking if it's safe to do so...

... is that the kind of thing you are talking about?

[nathanwoulfe]

A HasValue extension for ModelsBuilder models would be wonderful =>

Model.MyProperty.HasValue() is a whole lot friendlier than Model.HasValue(Model.GetModePropertyType(p => p.MyProperty).PropertyTypeAlias).

Not sure how that would need to be implemented though, given it would be an extension on the property type, not the model itself. Surely possible though...

[rbottema]

I guess Model.HasValue(p => p.MyProperty) could be a good middle ground? Don't know if a HasValue extension method on object and all value types makes sense and allows for the functionality to work (as you're missing context).

[nathanwoulfe]

Yup, that sounds sensible. So long as there's a consistent and predictable way to check for a value, it will be a good improvement.

[ronaldbarendse]

The HasValue() method uses a different way of checking whether the value is 'not empty' compared to the actual value returned by Value(), as it calls the IsValue() method on the PVC:

Umbraco-CMS/src/Umbraco.Core/PropertyEditors/PropertyValueConverterBase.cs

Lines 17 to 32 in 872731f

	public virtual bool? IsValue(object value, PropertyValueLevel level)
	{
	switch (level)
	{
	case PropertyValueLevel.Source:
	// the default implementation uses the old magic null & string comparisons,
	// other implementations may be more clever, and/or test the final converted object values
	return value != null && (!(value is string stringValue) || !string.IsNullOrWhiteSpace(stringValue));
	case PropertyValueLevel.Inter:
	return null;
	case PropertyValueLevel.Object:
	return null;
	default:
	throw new NotSupportedException($"Invalid level: {level}.");
	}
	}

A lot of PVCs don't correctly implement this to begin with, but as @marcemarc already pointed out, there might actually be data present that just can't be converted anymore (because it became invalid, referenced items got deleted, etc.). This can result in HasValue() returning true, but Value() still returning an empty/null value.

Another thing to note though: the IsValue() method is used to determine whether to use a fallback value or not, so it's not totally useless...

PR #8271 is my attempt to provide an easier base class, which is probably a good thing to start with 😄

[ronaldbarendse]

Helpers for frontend rendering

To make it easier to render Umbraco specific content, we can ship with HTML helper extension methods (and in v9 with ASP.NET Core Tag Helpers, like https://github.com/umbraco-community/Our-Umbraco-TagHelpers). I've already created 2 PRs that make it easier to render Nested Content, Block List and Multi URL Picker/Link items:

• Add Partials and RenderPartials extension methods to replace GetBlockListHtml #8840
• Add Link and BeginLink extension methods #9031

Property value converters

I think we should give all default PVCs a good review/clean-up/refactor, as there are quite a lot of issues with correctly converting the values (like throwing exceptions, causing front-end pages to error out), multiple PVCs being registered for the same editor in different assemblies (that are not replaced/shadowed in the recommended way), the source/intermediate/object value types sometimes being different based on configuration/editor value, XPath querying being fundamentally broken for some editors and some PVCs that already lookup data that then isn't exposed (and would require manual re-querying):

• Fix IndexOutOfRangeException when converting single value to range in SliderValueConverter #8907
• Fix NullReferenceException while deserializing empty value in TagsValueConverter #8906
• Shadow default Umbraco.Core PVCs #8249
• Add generic PropertyValueConverterBase classes #8271
• Not all PVCs return a correct XPath value #6431
• Multi URL Picker enhancements, add Content property and option to hide target checkbox #7533

Having a generic base class for the PVCs will certainly help with some of the exceptions and ensuring the return types are consistent. But I also think the registration part should be simplified, as we've repeatedly made the same mistake with the DefaultPropertyValueConverterAttribute and shadowing existing default PVCs. In most cases it's because we ship with a default JsonValueConverter that registers itself for all editors with the data value type set to JSON, which we actually might want to extend to other value types as well (although that would be breaking, as discussed in #4419).

Property editors

Besides PVCs (for the front-end rendering part), there's also some work we can do on the property/data editors themselves:

• Use strongtyped Models as grid and RTE config #7508
• Allow default values to be set on all core property types #7859
• V8: Multi Url Picker: Ability to show and select content nodes in multiple languages #7546

Or possibly merge some of the functionalities into fewer editors if that makes sense:

• Feature - Reduce the number of "content" pickers down to only Multi-Node Tree Picker #6181
• One picker to rule them all #7636

Other random related

Lastly, I wanted to highlight these items, as they are all beneficial to front-end rendering as well:

• Sort domains (Culture and Hostnames) #8316
• Add Fallback.ToDefaultLanguage #8350
• V8: Allow varying culture for media types #4771