U4-9894 - UDI Pickers and all 'v2' property editors shouldn't observe the EnablePropertyValueConverters setting

Created by Jeavon Leopold 10 May 2017, 13:54:40 Updated by Sebastiaan Janssen 01 Jun 2017, 13:27:08

Tags: Unscheduled

Relates to: U4-9974

The 'v2' property editors shouldn't observe the EnablePropertyValueConverters setting because these are brand new editors which should always have the converters applied. The EnablePropertyValueConverters setting is specifically for existing editors to maintain compat.

Original report This maybe intentional as it's a new editor in which case this is not a issue, however I noticed the Value Converter for the MediaPicker2 does not disable with the EnablePropertyValueConverters setting.

https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerPropertyConverter.cs#L125 is missing the check https://github.com/umbraco/Umbraco-CMS/blob/dev-v7.6/src/Umbraco.Web/PropertyEditors/ValueConverters/MultipleMediaPickerPropertyConverter.cs#L59

However, if it is intentional should we be observing this setting for the other UDI "v2" editors?

@Shandem or @zpqrtbnk may know...?

Comments

Sebastiaan Janssen 15 May 2017, 17:45:27

This seems like an oversight unless I'm missing something.

I found a few others that were missing this check!

https://github.com/umbraco/Umbraco-CMS/pull/1946


Jeavon Leopold 15 May 2017, 17:51:08

@nul800sebastiaan I'm not sure about those other 3 converters, they have been like that for years, so disabling them with this setting is probably not a good thing....?


Sebastiaan Janssen 15 May 2017, 20:12:16

Ah of course, you're quite correct. I didn't realize we had some converters already for years! Reverted, thanks!


Shannon Deminick 19 May 2017, 01:49:26

This is like this on purpose, i don't see any reason we need to change anything: https://github.com/umbraco/Umbraco-CMS/pull/1946#discussion_r117235250

The reason this isn't here is because MediaPicker2Alias is a brand new property editor, one that wouldn't exist in previous versions so there would be no reason to disable the property converter for this since it's new and if people are using it it will just return what it is supposed to.


Shannon Deminick 19 May 2017, 01:50:54

NOTE: the EnablePropertyValueConverters is 100% for backwards compatibility reasons only, it's not actually to globally disable all property value converters, it's just to disable the new ones that were added in 7.6


Sebastiaan Janssen 19 May 2017, 08:34:54

👍


Jeavon Leopold 19 May 2017, 14:42:00

Totally makes sense but in that case shouldn't we make all of the "v2" pickers not observe the "EnablePropertyValueConverters" setting, currently MediaPicker2Alias is only one that doesn't....? Thoughts?


Shannon Deminick 29 May 2017, 07:42:11

Ah right, yes that should be changed, i'll do that now

PR: https://github.com/umbraco/Umbraco-CMS/pull/1965


Jeavon Leopold 29 May 2017, 10:26:02

Looks good to me :)


Priority: Normal

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: False

Fix Submitted:

Affected versions: 7.6.0, 7.6.1

Due in version: 7.6.2

Sprint: Sprint 60

Story Points:

Cycle: