Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple Media Picker returns list of IPublishedElement #6033

Closed
simonbrunemark opened this issue Jul 31, 2019 · 8 comments · Fixed by #6034

Comments

@simonbrunemark
Copy link

commented Jul 31, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

PR in #6034

@stevetemple

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

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

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

Fixed in #6034

Cherry picked for 8.1.4 27c8c7a

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

I've marked this as breaking while we investigate issues with querying media items, some more info is in the comments here: #6034 (comment)

@nul800sebastiaan

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

@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 #6034 (comment) 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

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

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):

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 #6034 (comment), but it works and keeps duplicated code to a minimum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.