umco / umbraco-ditto

Ditto - the friendly view-model mapper for Umbraco
http://our.umbraco.org/projects/developer-tools/ditto
MIT License
79 stars 33 forks source link

Rejected TypeConverter value returns null #109

Closed leekelleher closed 9 years ago

leekelleher commented 9 years ago

I found that if a TypeConverter's CanConvertFrom returns false, then the property's value is assigned a null reference.

I have added a fix, so that we check if the value type matches the target property's type, then we can assigned it.

It's probably easier to review the attached unit-test: ClassLevelTypeConverterTests

leekelleher commented 9 years ago

Just waiting for the next round of @JimBobSquarePants's "ValueResolvers vs TypeConverters" battle :stuck_out_tongue_closed_eyes:

tweetle_beetles_b2_trimmed

JimBobSquarePants commented 9 years ago

Cheeky!

The logic is definitely sound in this case since we want the user to get their property so it's a good fix. The question begs though, why would someone assign a TypeConverter to a property that it can't convert?

leekelleher commented 9 years ago

It's a bit of an edge-case I have on a Carlsberg project.

I have a class, with a TypeConverter, which I use with regular GetPropertyValue<T> calls (outside of Ditto). The property-editor is an Archetype, and the TypeConverter was originally written to only convert Archetype objects.

The complexity comes in as I need to have logic to recursively get Archetype values from up the tree (up to 3 levels) and aggregate (well, merge) the values. (It's for an age-gate component, it gets complicated)

I'm using a ValueResolver to do this. (I bet you're already shaking your head? 😜)

So the ValueResolver is returning the target Type (of my custom class) ... but the TypeConverter didn't know what to do with it, so rejected it... and I got a null value.

I've fixed my TypeConverter to handle that scenario, but it got me thinking if other devs hit a similar issue.

Hope this makes sense? (Sent from my mobile, excuse typos, etc)

JimBobSquarePants commented 9 years ago

You kids and your ValueResolvers! :older_man:

Certainly an edge case but yeah, since we already do the same thing under a different logic path

https://github.com/leekelleher/umbraco-ditto/blob/a0ba43f56663794b3799edfec1051b509d8867b2/src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs#L624

It's definitely wise to do it here also.