U4-5005 - YesNoValueConverter fails for int

Created by Lee Kelleher 28 May 2014, 10:46:18 Updated by Shannon Deminick 20 Aug 2014, 19:10:54

This may be a core issue, or maybe how a 3rd-party package (Archetype) handles checkbox values.

I've hit a scenario where I an using a "True/false" checkbox within an Archetype. When I am getting the values out of Archetype, the internals are making a call to the appropriate IPropertyValueConverter.ConvertDataToSource for the specific data-type.

However the value of the checkbox is being stored as an int, rather than a string, this means that when value is passed to YesNoValueConverter.ConvertDataToSource it always returns false, as the casting as string returns null.

The question is, should the YesNoValueConverter be able to handle all object types? or should it be fixed to string only?

I can submit a PR with a code fix so it can handle both string and int values.

Comments

Matt Brailsford 28 May 2014, 14:04:34

I've had a look at this and it is for sure an interesting issue, and one I think we might see more of for property editor reusing property converters (which there is a lot of this going on now).

Ultimately I've narrowed it down to the following.

When any PE is serialized to the DB, it ultimately ends up as a string when it is returned to a value converter, however the issue is, when a complex object is returned as json, it is the object that is stringified, rather than the raw value, ie:

bool = "1" obj = ""

So when archetype runs the value converters, it does so on it's value that it parsed out, which at that point is now an int. But the TrueFalseValueConverter only handles a value of type string (which ordinarily would be if it was just a raw standalone property value).

So the question is, who is at fault here? Should Archetype always stringify a sub value before passing it off to the ValueConverters? or should ValueConverters be able to handle more than just strings?

The tricky thing for Archetype if it was their responsibility is that it would have to try and serialize the value back down to a matching state as Umbraco would when it serializes a standalone value, which isn't necessarily the same as running it back through the JSON serializer.

From the other perspective though, when we define a PE we state why type it's value is stored as (Which True False is "INT") so is it unreasonable to assume the value converter should be able to handle an object of that type, not just string?

As I say, it's a tricky one, but one I think we are going to see more of, so would be good to get some input on what people think.

Matt


Kevin Giszewski 28 May 2014, 14:21:04

IMHO I think everything should be stored as strings whether a standalone value or JSON. My idealistic (and possibly irrational) view of the future of Umbraco values would be all JSON. Everything would be a string to Model conversion.

Now on a slight tangent, furthermore I think the DB properties should only store strings (as opposed to dates and integers).

I probably muddied the water on this :)


Lee Kelleher 19 Aug 2014, 17:43:18

Pull request submitted: https://github.com/umbraco/Umbraco-CMS/pull/454


Priority: Normal

Type: Bug

State: Fixed

Assignee: Shannon Deminick

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.1.3

Due in version: 7.1.5

Sprint:

Story Points:

Cycle: