leekelleher / umbraco-contentment

Contentment for Umbraco - a state of happiness and satisfaction
https://marketplace.umbraco.com/package/umbraco.community.contentment
Mozilla Public License 2.0
157 stars 72 forks source link

Certain (bad) Data List configurations result in an obscure Null Reference Exception. #311

Closed JasonElkin closed 1 year ago

JasonElkin commented 1 year ago

Which Contentment version are you using?

4.4.2

Which Umbraco version are you using? For example: 8.14.1 - don't just write v8

10.4.1

Bug summary

Certain configurations of the Data List editor (and possibly others, I haven't checked) can result in the Property Value Converter returning a null for GetPropertyValueType.

This is the same as the issue as reported here: https://github.com/umbraco/Umbraco-CMS/issues/13515

Umbraco should probably null check the type from the Property Value Converters and handle more gracefully, but I think it would be friendlier if contentment could make it harder to create a broken property.

Steps to reproduce

Simplest way is:

  1. Create a new Data List property.
  2. Use the Enum data source, but don't choose an Enum.
  3. Save and publish.

Expected result / actual result

I wouldn't expect the editor to work, but I think I'd expect a more graceful failure (i.e. not breaking the whole document) but, in an ideal world, there would be some validation to stop an editor from doing this (or at least make it harder).

Update: Umbraco won't throw for this and break the document now. Though, strictly speaking, the Property Value Converter still shouldn't return null for the type.

Do you have Umbraco ModelsBuilder enabled?

What browsers are you seeing the problem on?

Firefox, Chrome, Safari, Microsoft Edge

JasonElkin commented 1 year ago

There will be a type check in place in Umbraco from 11.3.0 that will stop this from breaking the document: https://github.com/umbraco/Umbraco-CMS/pull/13553

leekelleher commented 1 year ago

@JasonElkin Thank you for the update. 😁

I'll put something in place on Contentment's side too - once Umbraco Spark week is out the way! 😬

leekelleher commented 1 year ago

I took a little longer than post-Spark, had a holiday too. I've replaced the potential null with a default fallback (for Data List that'll be a type(string)). This will be in the next patch release.