umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.53k stars 2.71k forks source link

MultipleTextStringValueConverter is all wrong #11421

Open LennardF1989 opened 3 years ago

LennardF1989 commented 3 years ago

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

v9.0.1

Bug summary

Looks like this class hasn't been touched in quite a while, it's full of legacy code, assumes XML as a potential format (most likely coming from the XPath days of Umbraco), but most important of all: it simply doesn't work anymore.

When saving a MultipleTextString property, you get a JSON-array of objects, which have a "value" containing the string.

Right now, it takes that JSON-array, ToString's it, and puts it as-is in a list. This causes the first entry in the list to be [] when the list was empty, or the full JSON-array on a single line.

I'm not sure how backwards compatible we have to stay to at this point in time, but the proper implementation is:

    public class MultipleTextStringPropertyValueConverter : PropertyValueConverterBase
    {
        public override bool IsConverter(IPublishedPropertyType propertyType)
            => global::Umbraco.Cms.Core.Constants.PropertyEditors.Aliases.MultipleTextstring.Equals(propertyType.EditorAlias);

        public override Type GetPropertyValueType(IPublishedPropertyType propertyType)
            => typeof (List<string>);

        public override PropertyCacheLevel GetPropertyCacheLevel(IPublishedPropertyType propertyType)
            => PropertyCacheLevel.Element;

        public override object ConvertSourceToIntermediate(IPublishedElement owner, IPublishedPropertyType propertyType, object source, bool preview)
        {
            if (source == null)
            {
                return null;
            }

            var sourceArray = JArray.Parse(source.ToString());

            return sourceArray
                .Select(x => x["value"].ToString())
                .ToList();
        }
    }

While I'm pretty sure the original implementation is just an oversight, only Umbraco HQ can tell if we have to remain backwards compatible. Otherwise above source can be PR-ed by someone.

Specifics

No response

Steps to reproduce

  1. Use a repeatable textstring

Expected result / actual result

It's actually converted to an List of strings.

nul800sebastiaan commented 3 years ago

, only Umbraco HQ can tell if we have to remain backwards compatible

We do 😄 We can make breaking changes in v10 though.

umbrabot commented 3 years ago

Hi @LennardF1989,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly Umbraco GitHub bot :-)

LennardF1989 commented 3 years ago

@nul800sebastiaan Done, see #11456.