Umbraco-cms: Multiple Media Picker returns list of IPublishedElement

[simonbrunemark]

Note: Umbraco HQ has marked this as breaking. Some more info is available below:

Unfortunately fixing this issue led to an unintended breaking change, as you can read below, you will run into this is you are currently querying items picked in a media picker in a strongly typed way, like:

@if (post.PreviewImage != null)
{
     <img src="@post.PreviewImage.GetCropUrl("Blog Preview")" alt="@post.PreviewImage.AltDescription">
}

altDescription in this case is a property on you media type, it could also be the default umbracoWidth property for example.

To fix this, you can update your view in a few different ways:

1. Use the returned IPublishedContent directly and get the Value for the property like you are used to from other pickers:

@if (post.PreviewImage != null)
{
     <img src="@post.PreviewImage.GetCropUrl("Blog Preview")" alt="@post.PreviewImage.Value("altDescription")">
}

1. If you enjoy working with strongly typed objects you can cast the item you're working with to the type you know it should be, for example:

@if (post.PreviewImage is Image previewImage)
{
     <img src="@previewImage.GetCropUrl("Blog Preview")" alt="@previewImage.AltDescription">
}

1. For you ModelsBuilder lovers, Stéphane has described some additional options here: https://www.zpqrtbnk.net/posts/umbraco-8-1-4-breaking-models-builder/

The original issue description follows:

---

Hi

When creating a Media Picker data type and setting both "Pick multiple items" and "Pick only images" to true, then it consistently returns IEnumerable<IPublishedElement> instead of IEnumerable<IPublishedContent>.

So if we do a similar setup, like the one in the documentation (https://our.umbraco.com/documentation/Getting-Started/Backoffice/Property-Editors/Built-in-Property-Editors/Media-Picker/) and try to retrieve the value as mentioned in the documentation too, then it will throw an null error.

@{
    var typedMultiMediaPicker = Model.Value<IEnumerable<IPublishedContent>>("sliders");
    foreach (var item in typedMultiMediaPicker)
    {
        <img src="@item.Url" style="width:200px"/>
    }
}

It works just fine in all other combinations, except when the two checkboxes are both set to true.

I have experienced this bug on every Umbraco 8 versions so far.

[kjac]

Hi @simonbrunemark, thanks for reporting this 👍

I can reproduce, and unfortunately it is also the cause of a potential rendering YSOD. I'll have a look at it and report back.

[kjac]

PR in #6034

[stevetemple]

This also causes problems as you can pick folders if that is allowed, but it assumes everything will be an image so errors when trying get the value when both images and folders (or just folders are selected).

I'm able to reproduce on 8.1.3

[nul800sebastiaan]

Fixed in https://github.com/umbraco/Umbraco-CMS/pull/6034

Cherry picked for 8.1.4 27c8c7abf18202a5f97a0b39a63433ffad69d872

[nul800sebastiaan]

I've marked this as breaking while we investigate issues with querying media items, some more info is in the comments here: https://github.com/umbraco/Umbraco-CMS/pull/6034#issuecomment-527425298

[nul800sebastiaan]

As an update: the change in the related pull request caused an unintended breaking change. However, we believe the new behavior is the correct behavior and we will keep it this way. We believe it will cause less disruption to keep this change than to undo it and apply it again at a later time.

We always aim to only make these kinds of invasive changes to a minimum and if we do have to break something they need to be at least in a minor version update and not in a patch. Furthermore breaking changes need to be documented ahead of time.

This was the correct change to make but unfortunately at the wrong time. We do apologize for the inconvenience.

[ronaldbarendse]

@nul800sebastiaan Make sure to update option 2, as it currently can throw a NullReferenceException if the preview image isn't of type Image. @NikRimington's https://github.com/umbraco/Umbraco-CMS/pull/6034#issuecomment-527425298 might also be updated. Better provide correct working examples, especially for the people who like to copy-paste 😉

Using pattern-matching, this can be written as:

@if (post.PreviewImage is Image previewImage)
{
     <img src="@previewImage.GetCropUrl("Blog Preview")" alt="@previewImage.AltDescription">
}

[ronaldbarendse]

The following custom PVC makes sure the returned type is set back to Image or IEnumerable<Image> (if 'Pick only images' is checked), but now only contains items that actually inherit from this type (as opposed to throwing exceptions):

```c#
using System;
using System.Collections.Generic;
using Umbraco.Core;
using Umbraco.Core.Models.PublishedContent;
using Umbraco.Core.PropertyEditors;
using Umbraco.Web.PropertyEditors;
using Umbraco.Web.PropertyEditors.ValueConverters;
using Umbraco.Web.PublishedCache;

public class CustomMediaPickerValueConverter : MediaPickerValueConverter
{
private const string ImageTypeAlias = Constants.Conventions.MediaTypes.Image;
private readonly IPublishedModelFactory _publishedModelFactory;

public CustomMediaPickerValueConverter(IPublishedSnapshotAccessor publishedSnapshotAccessor, IPublishedModelFactory publishedModelFactory)
    : base(publishedSnapshotAccessor, publishedModelFactory)
{
    _publishedModelFactory = publishedModelFactory;
}

public override Type GetPropertyValueType(IPublishedPropertyType propertyType)
{
    var baseType = base.GetPropertyValueType(propertyType);
    if (IsOnlyImagesDataType(propertyType.DataType))
    {
        return (baseType == typeof(IEnumerable<IPublishedContent>))
            ? typeof(IEnumerable<>).MakeGenericType(ModelType.For(ImageTypeAlias))
            : ModelType.For(ImageTypeAlias);
    }

    return baseType;
}

public override object ConvertIntermediateToObject(IPublishedElement owner, IPublishedPropertyType propertyType, PropertyCacheLevel cacheLevel, object source, bool preview)
{
    var baseSource = base.ConvertIntermediateToObject(owner, propertyType, cacheLevel, source, preview);
    if (IsOnlyImagesDataType(propertyType.DataType))
    {
        var imageType = _publishedModelFactory.MapModelType(ModelType.For(ImageTypeAlias));

        if (baseSource is IEnumerable<IPublishedContent> mediaItems)
        {
            var images = _publishedModelFactory.CreateModelList(ImageTypeAlias);

            foreach (var mediaItem in mediaItems)
            {
                if (mediaItem.GetType().Inherits(imageType))
                {
                    images.Add(mediaItem);
                }
            }

            return images;
        }
        else if (baseSource != null)
        {
            return baseSource != null && baseSource.GetType().Inherits(imageType) ? baseSource : null;
        }
    }

    return baseSource;
}

private bool IsOnlyImagesDataType(PublishedDataType dataType)
{
    var config = ConfigurationEditor.ConfigurationAs<MediaPickerConfiguration>(dataType.Configuration);
    return config.OnlyImages;
}

}
```

If this class is added to a project, it will automatically be registered and replace the default PVC. This could be a lot cleaner if the default MediaPickerValueConverter is updated with my comments from https://github.com/umbraco/Umbraco-CMS/pull/6034#issuecomment-528327808, but it works and keeps duplicated code to a minimum.

[jaddie]

Is this now going to stay like this going forwards? Surely there must be a better way to catch this particular situation and correct it to work as originally expected? Unless I am misunderstanding the description on the top of the first post, this change sounds not only like a step backwards both in terms of syntax but also readability & from what I've seen of V7 sites and suchlike as well -- will also cause a horrendous amount of code to break and need to be re-written just to support V8. I've been talking V8 up whereever I can, however seeing something like this massively puts me off that and the idea of staying V7 becomes a lot more tempting instead, I know many other people and clients who will not want to deal with this, clients have limited budget enough as it is just to get to upgrade to v8 let alone re-write half the codebase.

I have everything crossed that I am indeed misunderstanding here, but if I am not then I really think this needs to be reviewed before it is set in stone as all this .Value rubbish and magic strings is a certain step in the wrong direction.

[ronaldbarendse]

@jaddie Umbraco 7 only returned IPublishedContent (returning Image was added in 8), so this actually makes it easier to migrate existing code from 7 to 8 😉

If the Media Picker would get an additional setting to restrict on actual media type(s), returning a specific type (e.g. Image) could be correctly implemented again (just like MNTP and Nested Content currently do).