MultiNodeTreePickerConverter<T> throws when passed an empty string

[akatakritos]

This pertains to Umbraco 7.2.4 and Ditto 0.6.1

Hello! I've been playing with integrating Ditto into a test project and ran into an issue when using the MultiNodeTreePickerConverter: it would throw when passed an empty string, which I believe happens when the user hadn't selected any content.

public class Event : BasePage
 {
    public Event(IPublishedContent content) : base(content)
    {
    }

    public DateTime StartTime { get; set; }
    public DateTime EndTime { get; set; }
    public string EventName { get; set; }
    public string EventDescription { get; set; }

    [TypeConverter(typeof(MultiNodeTreePickerConverter<Location>))]
    [UmbracoProperty("location")]
    public IEnumerable<Location> Locations { get; set; }
}

I would get a NotSupported exception if the user didn't select any locations in the back office.

I fixed it by subclassing:

public class AllowEmptyMultiNodeTreePickerConverter<T> : MultiNodeTreePickerConverter<T> where T:class
{
    public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value)
    {
        if (value is string && value.Equals(""))
        {
            return Enumerable.Empty<T>();
        }

        return base.ConvertFrom(context, culture, value);
    }
}

And using that type as the TypeConverter in the attribute.

Looking at the code, MultiNodeTreePickerConverter returns an empty enumerable if passed a null to convert.

Am I using this attribute incorrectly?

If not, I was wondering if you were interested in a PR that treated empty string the same?

Thanks for your time!

[JimBobSquarePants]

Thanks for this. I've a funny feeling I've already fixed this in dev but I'll double check.

You are using the attribute properly by the looks of things.

MultiNodeTreePickerConverter returns an enumable by default but Ditto will check if your property implements IEnumerable and yield a single item if it doesn't.

[akatakritos]

I just checked out and built the develop branch and it exhibits the behavior I described. I saw there was a PR accepted a while back that added support for null, but it doesn't look like empty string is supported yet.

I'd be happy to do a PR for this issue (and double check the other converters) if that's a feature you're interested in supporting.

[JimBobSquarePants]

We definitely should support empty string so if you want to do a PR then please do. You're more than welcome 😄

Write the fix on the TypeConverters themselves though rather than any extensions.

[akatakritos]

sorry for the spurious commits showing in this issue. I cant spell and kept having to --amend and force push to my fork. The PR should only include one commit.

[leekelleher]

Thanks @akatakritos, very much appreciated!

Closing this issue, as discussions can take place over on PR #59