V8: Hard coded "image" type in media picker value converter causes YSOD #6034

kjac · 2019-07-31T20:08:03Z

Prerequisites

I have added steps to test this contribution in the description below

If there's an existing issue for this PR then this fixes #6033

Description

If you configure a media picker to pick only images, the media picker value converter assumes that all picked elements in the media picker are of a content type with the alias Image (which is the default "Image" media type).

This is handy as you can access the built-in image properties of Image like UmbracoHeight and UmbracoWidth directly without having to cast it or use e.g. .Value<int>("umbracoHeight").

Unfortunately this hard coded content type alias also results in a YSOD when rendering, if an editor has picked another "image" media type - e.g. if you have a custom "image" media type alongside the built-in one. Here's a GIF that demonstrates it:

Clearly the media picker doesn't prevent the user from selecting other media types, as long as they have a thumbnail (which qualifies them as "images") - and nor should it. However... this needs to be mirrored in the media value converter; it can't assume that the value is made up solely by Image items when it's in "only images" mode.

This PR removes this assumption from the value converter. When it's applied you're able to pick and render different "image" media types:

The value converter now always returns IEnumerable<IPublishedContent> if the media picker is configured to allow multiple selected items, or IPublishedContent if it is only allowed to pick one - just like the rest of the content pickers.

This both fixes #6033 and ensures that the media picker works the way it is documented.

Breaking change?

This isn't strictly a breaking change from a code perspective. But it probably qualifies as a behavioral breaking change, as it may break implementations that access the Image content type properties (UmbracoHeight, UmbracoWidth etc.) directly on the picked items of a media picker in "only images" mode.

Testing this PR

Create a new media type by copying the built-in "Image" media type
Create a new image of the new media type.
Add a media picker configured as shown above to a content type.
Create content of this type and pick items of both the built-in "Image" media type and the new media type.
Render the content with a template that fetches the value of the media picker. You can use the sample template below, if your media picker has the alias imagePicker.
If things do not blow up, this PR works 😄

@inherits Umbraco.Web.Mvc.UmbracoViewPage
@{
	Layout = null;
	var images = Model.Value<IEnumerable<IPublishedElement>>("imagePicker").ToArray();
}
<!DOCTYPE html>
<html>
    <body>
        <p>
            Image picker contains <b>@(images.Count())</b> images of the following types:
        </p>
        <ul>
            @foreach(var image in images) {
                <li>@image.ContentType.Alias</li>
            }
        </ul>
    </body>
</html>

emmaburstow · 2019-08-02T20:39:35Z

Ah Kenn. Nice find and nice fix. Thanks for the pics! We'll take a look and let you know if we need anything.

Em

stevetemple · 2019-08-22T13:46:12Z

It's worth noting the bug shows up using just the default media types if you have allowed folder selection and select a folder and an image then trying to get this value causes an error when it tries to convert Folder to Image

nul800sebastiaan · 2019-08-29T12:20:16Z

👍 👍 👍

NikRimington · 2019-09-03T11:51:26Z

I think this might have been a breaking change. Upgrading a site from 8.1.3 to 8.1.4 means that any single image media picker throws an error if using Models Builder as the returned type is no longer a Image type instead it's IPublishedContent.

The results are if you are accessing any model explicit properties then it now YSODs and a cast is required.

e.g:
This used to work but after upgrade fails saying AltDescription property isn't available

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

This is what I had to update to

//updated code (8.1.4)
@if (post.PreviewImage != null)
{
      var previewImage = post.PreviewImage as Image;
      <img src="@previewImage.GetCropUrl("Blog Preview")" alt="@previewImage.AltDescription">
}

nul800sebastiaan · 2019-09-03T12:32:44Z

It seems so :-( investigating

I've marked the issue with breaking for now, not sure what we should do yet, we could revert, risking breaking it again, which would also never fix the original issue.. or get good guidance in place for people who run into the problem.

If I understand correctly this happens on custom media types? Or all media types?

NikRimington · 2019-09-03T12:57:44Z

This issue for me occurred when I had a Media Picker set to only pick a single Image. The Property Value Converter would convert the property to type Image explicitly instead of IPublishedContent. The only thing that's "custom" is that the default media type Image had had an additional property added to it. I suspect if you were accessing properties such as File Extension / Size (which are default on Image / Media) you'd have the same issue as IPublishedContent doesn't have those properties.

nul800sebastiaan · 2019-09-03T13:15:44Z

Unfortunately, it also breaks querying for default properties like UmbracoHeight and UmbracoWidth, that's not nice. :-(

nul800sebastiaan · 2019-09-03T13:17:49Z

The workaround as mentioned by @NikRimington does work and will keep working when this change is reverted, which I think might be the correct thing to do for 8.1.5 while we rethink how the original issue can be fixed.

However, it will cause pain again down the line. So I am not sure how we can fix it nicely without forcing people to do a forced cast to get property values. That is not very.. friendly.

zpqrtbnk · 2019-09-03T13:19:47Z

For people using Models Builder... the easiest solution is to force-implement the property in a partial, and cast the returned value to Image - that will work for single images. As for multiple images... you'd have to cast the enumerable.

:(

callumbwhyte · 2019-09-03T14:45:15Z

Also experiencing this issue - exact steps as @NikRimington above: all media pickers set to only return images (regardless of singular or multiple) fail to cast the value to Image.

@zpqrtbnk's solution is a decent one, but in my case the site has 40+ content types with an image picker on most... A little laborious sadly...

In lieu of a scaleable fix here, I'm going to re-introduce the Media Picker converter to my SuperValueConverters package and override the core converter - hopefully this might help others in the same position too until a "proper" fix exists :-)

nul800sebastiaan · 2019-09-03T14:49:44Z

Just note that the previous version of the property value converter has a serious error in it, it should never have returned all picker media items as type Image - obviously that can not be true.

To summarize the issue, as I understand it now:

Previously, there was a big bug in that all picked media items were forced to be of type Image
This is completely inconsistent with any other pickers, which always return IPublishedContent
8.1.4 fixes that problem, but some people rely on the return type to be Image
You will need to update your code to be consistent with any other code for iterating over picked items: item.Value("myProperty")
You can also cast the item to Image but this is not required, querying the Value is what you do everywhere else in Umbraco

callumbwhyte · 2019-09-03T14:59:27Z

@nul800sebastiaan I slightly disagree with your statement that the behaviour is "completely inconsistent with any other pickers" - while the MNTP (for example) does always return IPublishedContent regardless of configuration, Nested Content returns a strongly typed model if there is only one element type allowed (and IPublishedElement otherwise)... So it seems all quite inconsistent 😆

I think the best approach here is having a real way to determine if a media type is an "image" - as pointed out in this thread, that's currently determined by whether a media item has a specific property on it...

I'm doing a bit of digging to see if there's a nice way to determine if a media type might have a "thumbnail"

nul800sebastiaan · 2019-09-03T15:02:10Z

Fair enough, however, always returning Image is definitely incorrect. :-)

callumbwhyte · 2019-09-03T15:03:02Z

Indeed - I was a little too trigger happy with posting ;-) Updated my comment a bit. Think we can find a nice-ish way to determine what type to return, Nested Content style

nul800sebastiaan · 2019-09-03T15:05:25Z

But even if a media type has a "thumbnail" it could absolutely still be a custom media type with custom properties. I hope we can reliably (non-hacky) find the correct type, but as far as I understand it now, I don't think we can.

ronaldbarendse · 2019-09-03T15:10:31Z

Just like @kjac mentioned, 'Pick only images' allows picking all media types that have a thumbnail. This is determined by checking whether the umbracoFile property contains a file with a configured ImageFileTypes extension:

Umbraco-CMS/src/Umbraco.Core/Configuration/UmbracoSettings/ContentImagingElement.cs

Lines 8 to 18 in 853087a

    
           [ConfigurationProperty("imageFileTypes")] 
        
           internal CommaDelimitedConfigurationElement ImageFileTypes => 
        
               new OptionalCommaDelimitedConfigurationElement( 
        
                   (CommaDelimitedConfigurationElement)this["imageFileTypes"], 
        
                   //set the default 
        
                   GetDefaultImageFileTypes()); 
        
           internal static string[] GetDefaultImageFileTypes() 
        
           { 
        
               return new[] {"jpeg", "jpg", "gif", "bmp", "png", "tiff", "tif"}; 
        
           }

So this means you can also select the default File media type that contains an image file. Having the PVC return a concrete Image type was indeed a bug, as that might not always be the case.

callumbwhyte · 2019-09-03T15:13:52Z

As @ronaldbarendse and @kjac have pointed out, the thumbnail is used as the criteria for determining an "image" currently in the backoffice - so this would seem a sensible flag to check and see if the media type supports? Or we could determine an "image" by a media type that's umbracoFile property is an image cropper (or something other than the generic upload field)?

The logic could be something like:

If only one "image" media type exists, return that type - e.g. Image or MyCustomMediaType
If multiple "image" media types exist, return IPublishedContent

...Which facilitates the OOTB behaviour (returning Image), custom media types, or both...

ronaldbarendse · 2019-09-03T20:25:33Z

@callumbwhyte As an image is determined by the file extension of the umbracoFile property, all media types with this property could actually be an image, including the default File media type.

Besides, having the return type change when you add another custom media type (without changing the settings of the media picker) would result is very strange behavior.

A better solution would be to specify the allowed media types in the settings and if there's only one selected, use that as return type. There's some discussion about extending existing pickers with features from MNTP and this is already mentioned by @kjac: #6181 (comment).

To guarantee the picked items are of a specific type, you can always use var images = Model.Images.OfType<Image>(); or for a single item var image = Model.Image as Image;. Even better: if you also allow selecting folders, you can get all images (including within nested folders) using var images = Model.Images.DescendantsOrSelf<Image>().

ronaldbarendse · 2019-09-05T11:53:12Z

The required changes to fix this are now done, but the class needs some cleanup:

Remove ImageTypeAlias (should've been Constants.Conventions.MediaTypes.Image anyway):

Umbraco-CMS/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs

Lines 18 to 19 in 3457018

    
           // hard-coding "image" here but that's how it works at UI level too 
        
           private const string ImageTypeAlias = "image";

Remove IPublishedModelFactory field and constructor overload (breaking):

Umbraco-CMS/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs

Line 21 in 3457018

private readonly IPublishedModelFactory _publishedModelFactory;

Umbraco-CMS/src/Umbraco.Web/PropertyEditors/ValueConverters/MediaPickerValueConverter.cs

Lines 24 to 30 in 3457018

    
           public MediaPickerValueConverter(IPublishedSnapshotAccessor publishedSnapshotAccessor, 
        
               IPublishedModelFactory publishedModelFactory) 
        
           { 
        
               _publishedSnapshotAccessor = publishedSnapshotAccessor ?? 
        
                                            throw new ArgumentNullException(nameof(publishedSnapshotAccessor)); 
        
               _publishedModelFactory = publishedModelFactory; 
        
           }

To make inheriting the PVC easier, maybe also change:

readonly field(s) to protected readonly
IsMultipleDataType to protected
Change custom FirstOrDefault method to LINQ method

Don't use a hardcoded "image" type in media picker value conversion

a2a2853

kjac mentioned this pull request Jul 31, 2019

Multiple Media Picker returns list of IPublishedElement #6033

Closed

nul800sebastiaan merged commit 3457018 into umbraco:v8/dev Aug 29, 2019

kjac deleted the v8-fix-media-picker-hardcoded-media-type branch August 29, 2019 13:49

callumbwhyte mentioned this pull request Aug 3, 2020

Return Image only type when datatype is configured as such #8444

Closed

callumbwhyte mentioned this pull request Jun 9, 2021

Media Picker v3 support callumbwhyte/super-value-converters#18

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V8: Hard coded "image" type in media picker value converter causes YSOD #6034

V8: Hard coded "image" type in media picker value converter causes YSOD #6034

kjac commented Jul 31, 2019 •

edited

Loading

emmaburstow commented Aug 2, 2019

stevetemple commented Aug 22, 2019

nul800sebastiaan commented Aug 29, 2019

NikRimington commented Sep 3, 2019

nul800sebastiaan commented Sep 3, 2019

NikRimington commented Sep 3, 2019

nul800sebastiaan commented Sep 3, 2019

nul800sebastiaan commented Sep 3, 2019 •

edited

Loading

zpqrtbnk commented Sep 3, 2019

callumbwhyte commented Sep 3, 2019

nul800sebastiaan commented Sep 3, 2019

callumbwhyte commented Sep 3, 2019 •

edited

Loading

nul800sebastiaan commented Sep 3, 2019

callumbwhyte commented Sep 3, 2019 •

edited

Loading

nul800sebastiaan commented Sep 3, 2019

ronaldbarendse commented Sep 3, 2019

callumbwhyte commented Sep 3, 2019 •

edited

Loading

ronaldbarendse commented Sep 3, 2019

ronaldbarendse commented Sep 5, 2019

V8: Hard coded "image" type in media picker value converter causes YSOD #6034

V8: Hard coded "image" type in media picker value converter causes YSOD #6034

Conversation

kjac commented Jul 31, 2019 • edited Loading

Prerequisites

Description

Breaking change?

Testing this PR

emmaburstow commented Aug 2, 2019

stevetemple commented Aug 22, 2019

nul800sebastiaan commented Aug 29, 2019

NikRimington commented Sep 3, 2019

nul800sebastiaan commented Sep 3, 2019

NikRimington commented Sep 3, 2019

nul800sebastiaan commented Sep 3, 2019

nul800sebastiaan commented Sep 3, 2019 • edited Loading

zpqrtbnk commented Sep 3, 2019

callumbwhyte commented Sep 3, 2019

nul800sebastiaan commented Sep 3, 2019

callumbwhyte commented Sep 3, 2019 • edited Loading

nul800sebastiaan commented Sep 3, 2019

callumbwhyte commented Sep 3, 2019 • edited Loading

nul800sebastiaan commented Sep 3, 2019

ronaldbarendse commented Sep 3, 2019

callumbwhyte commented Sep 3, 2019 • edited Loading

ronaldbarendse commented Sep 3, 2019

ronaldbarendse commented Sep 5, 2019

kjac commented Jul 31, 2019 •

edited

Loading

nul800sebastiaan commented Sep 3, 2019 •

edited

Loading

callumbwhyte commented Sep 3, 2019 •

edited

Loading

callumbwhyte commented Sep 3, 2019 •

edited

Loading

callumbwhyte commented Sep 3, 2019 •

edited

Loading