U4-10178 - Core Valueconverter for legacy mediapicker does not work for single picker

Created by Dave Woestenborghs 18 Jul 2017, 12:32:26 Updated by Dave Woestenborghs 19 Jul 2017, 10:48:46

Tags: PR

What did you do? I upgraded a site from 7.5.13 to 7.6.4. After I removed the legacy value converters package from @crumpled_jeavon I enabled the core value converters in umbracosettings.config and rebuild my models

What did you expect to happen? That this code for getting media items for a single media picker keeps working

return this.GetPropertyValue<IPublishedContent>("image");

What actually happened? I received this error

FormatException: String "4545" is not a valid udi.]
   Umbraco.Core.Udi.ParseInternal(String s, Boolean tryParse, Udi& udi) +812
   Umbraco.Core.Udi.Parse(String s) +23
   System.Linq.WhereSelectArrayIterator`2.MoveNext() +80
   System.Linq.Buffer`1..ctor(IEnumerable`1 source) +153
   System.Linq.Enumerable.ToArray(IEnumerable`1 source) +106
   Umbraco.Web.PropertyEditors.ValueConverters.MediaPickerPropertyConverter.ConvertDataToSource(PublishedPropertyType propertyType, Object source, Boolean preview) +193
   Umbraco.Web.PublishedCache.XmlPublishedCache.XmlPublishedProperty.get_Value() +73
   Umbraco.Web.PublishedPropertyExtension.GetValue(IPublishedProperty property, Boolean withDefaultValue, T defaultValue) +102
   Umbraco.Web.PublishedContentExtensions.GetPropertyValue(IPublishedContent content, String alias) +75
   Project.Models.ContentModels.ContactPerson.get_Image() in Projects.Models\ContentModels\ContactPerson.generated.cs:54

So it seems to be using the converter for the new media picker that stores UDI

Comments

Sebastiaan Janssen 18 Jul 2017, 12:35:54

Sounds like it duplicates U4-10013 exactly? :-)


Dave Woestenborghs 18 Jul 2017, 12:45:31

Added a PR for this one : https://github.com/umbraco/Umbraco-CMS/pull/2055


Sebastiaan Janssen 18 Jul 2017, 12:47:29

Can you check if it's fixed for you with U4-10013 already please?


Dave Woestenborghs 18 Jul 2017, 12:47:37

Probably related...but when I copied the latest source in to my project it didn't work yet. Had to apply my fix as well. It would never hit the second if.


Sebastiaan Janssen 18 Jul 2017, 12:48:03

The build is here: https://ci.appveyor.com/project/Umbraco/umbraco-cms-hs8dx/build/7812/artifacts


Sebastiaan Janssen 18 Jul 2017, 12:48:28

@dawoe I only merged this last night.. What, to you, is "the latest source"?


Dave Woestenborghs 18 Jul 2017, 12:56:34

And I created the PR just now...so I would have the latest source (I hope). I copied the new value converter from jeavon to my project

In the PR by @crumpled_jeavon you have the check doesn't work correctly in the IsConverterFor method

public override bool IsConverter(PublishedPropertyType propertyType)
        {
            if (UmbracoConfig.For.UmbracoSettings().Content.EnablePropertyValueConverters)
            {
                return propertyType.PropertyEditorAlias.Equals(Constants.PropertyEditors.MultipleMediaPickerAlias);
            }

            if (UmbracoConfig.For.UmbracoSettings().Content.EnablePropertyValueConverters)
            {
                // this is the double legacy media picker, it can pick only single media items
                return propertyType.PropertyEditorAlias.Equals(Constants.PropertyEditors.MediaPickerAlias);
            }
            return false;
        }

The second if here is never reached. That is what my PR fixes


Sebastiaan Janssen 18 Jul 2017, 13:23:56

Cheers @dawoe! I'll have to put is on my list to review then! Maybe @crumpled_jeavon has time to take a look too, I might have tested the wrong thing! :-)


Jeavon Leopold 18 Jul 2017, 14:16:28

@dawoe interesting. Yes @sebastiaan I have a look now


Jeavon Leopold 18 Jul 2017, 14:57:36

Yes, this is good, I might change it slightly to:

public override bool IsConverter(PublishedPropertyType propertyType) { if (UmbracoConfig.For.UmbracoSettings().Content.EnablePropertyValueConverters && propertyType.PropertyEditorAlias.Equals(Constants.PropertyEditors.MultipleMediaPickerAlias)) { return true; }

        if (UmbracoConfig.For.UmbracoSettings().Content.EnablePropertyValueConverters && propertyType.PropertyEditorAlias.Equals(Constants.PropertyEditors.MediaPickerAlias))
        {
            // this is the double legacy media picker, it can pick only single media items
            return true;
        }

        return false;
    }

Just so that the comment makes slightly more sense for the future

Thanks @dawoe


Dave Woestenborghs 18 Jul 2017, 15:12:36

@crumpled_jeavon , @sebastiaan

Implemented the changes suggested by @crumpled_jeavon


Jeavon Leopold 18 Jul 2017, 15:33:19

Thanks @dawoe !


Sebastiaan Janssen 19 Jul 2017, 10:32:10

Awesome, I see what's going on now.. subtle! All merged, thanks! :-)


Sebastiaan Janssen 19 Jul 2017, 10:33:38

Oh! @dawoe you didn't have the core contrib badge yet.. here you go! https://our.umbraco.org/member/2054


Dave Woestenborghs 19 Jul 2017, 10:48:47

@sebastiaan woohoo


Priority: Major

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category: Architecture

Backwards Compatible: True

Fix Submitted: Pull request

Affected versions: 7.6.4

Due in version: 7.6.5

Sprint:

Story Points:

Cycle: