search

umco / umbraco-property-list

Property List - a repeatable list (of selected datatype) for Umbraco 7.6+
https://our.umbraco.com/projects/backoffice-extensions/property-list/
MIT License
7 stars 2 forks source link

Property List of RTEs returns list of NULL #9

Closed pgregorynz closed 5 years ago

pgregorynz commented 5 years ago

When using property list with RTEs, when the value is converted by the property value converter the result is all nulls. The returned type is correct, IEnumerable<IHtmlString> but all the items in that list are null.

leekelleher commented 5 years ago

Hey @pgregorynz - thanks for raising this one.

I haven't had chance to dive into the code to debug this yet. (It'll most likely be late Thurs before I do). Although I have my suspicions of what it might be... I'm leaving some notes here, mostly for myself.

TinyMceValueConverter sets the PropertyValueType as IHtmlString, then ConvertSourceToObject returns HtmlString ... which should be castable, but maybe it isn't? 🤔

Property List is using Umbraco's TryConvertTo<T> to perform the casting... so worth checking if (HtmlString).TryConvertTo<IHtmlString>() works as expected, or whether Property List needs to do extra work for interface-types?

leekelleher commented 5 years ago

Curses this has been playing on my mind... and it's lunch time.

It turns out that it is Umbraco's TryConvertTo methods doing different things, e.g.

The generic version works as expected...

(new System.Web.HtmlString("foo")).TryConvertTo<System.Web.IHtmlString>().Success // is true

However the non-generic version fails...

(new System.Web.HtmlString("foo")).TryConvertTo(typeof(System.Web.IHtmlString)).Success // is false

... and Property List is using the non-generic version, (since we only know the object-type at runtime).

Digging into Umbraco's TryConvertTo<T> (generic version) code, this line will check if the non-generic version was successful, if not, it'd attempt a direct cast. Which in this case explains why the generic version works and the non-generic version doesn't.

OK, where does this leave us?

  1. Change Umbraco core so that TinyMceValueConverter return type is a HtmlString, and not an IHtmlString. (not ideal, too much work involved convincing people and waiting for release cycle and asking you to upgrade)
  2. Ask you to develop a custom TypeConverter that will cast an HtmlString to an IHtmlString, (again, not ideal, throws the problem over your side of the fence ... and other people will ask me about this in future, etc)
  3. Add code to Property List to follow a similar pattern as Umbraco's TryConvertTo<T>, check for failure and attempt a direct cast.

While I don't think this is Property List's concern, (I think it should be TryConvertTo responsibility), the last option will be frictionless. I'll see what I can do. 😃

leekelleher commented 5 years ago

@pgregorynz I've added a fix for this. If you want to try it out, the latest package files from the CI build are here: https://ci.appveyor.com/project/UMCO/umbraco-property-list/build/1.0.1.47/artifacts

Preparing a release takes a little bit more time, I'll schedule a v1.0.1 release soon.

leekelleher commented 5 years ago

@pgregorynz Just to let you know that v1.0.1 is out! https://github.com/umco/umbraco-property-list/releases/tag/1.0.1