We have moved to GitHub Issues
Created by Jeavon Leopold 10 May 2017, 13:54:40 Updated by Sebastiaan Janssen 01 Jun 2017, 13:27:08Tags: 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...?
This seems like an oversight unless I'm missing something.
I found a few others that were missing this check!
@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....?
Ah of course, you're quite correct. I didn't realize we had some converters already for years! Reverted, thanks!
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.
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
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?
Ah right, yes that should be changed, i'll do that now
Looks good to me :)
Backwards Compatible: False
Affected versions: 7.6.0, 7.6.1
Due in version: 7.6.2
Sprint: Sprint 60