Add ImageCrops from DataType if empty

[cleversolutions]

Prerequisites

This fixes issue #5737

Description

When you add a crop alias to Image Cropper after media already exists in your Media library it doesn't pick up any of the crops. This PR updates ImageCropperTemplateExtensions to check for this condition and configure crops from the DataType.Configuration.

Steps to reproduce

As per issue #5737

• V8 with SK installed
• Update Image Media Type -> Update umbracoFile Image Cropper DataType settings to include a crop type (ie square)
• Update Products.cshtml / template with
• View page on frontend & confirm images render (they don't before PR)
• Goto Media Section & Choose a Product Image
• Set manual crops for an image
• View page on frontend & confirm all images render (only the updated image does before PR)

With this PR, I've also tested another scenario

• Update Image Media Type -> Update umbracoFile Image Cropper DataType settings to include another crop type (ie 1x1)
• Update Products.cshtml / template with <img src='@product.Photos.GetCropUrl("1x1")' />
• View page on frontend & confirm all images render including the updated one.

Caveat

There may be a better way of solving this, I'm new to the Umbraco code base. The fact that the image which was edited has 2 crops in imageCropperValue.Crops the second time makes me wonder if the real issue lies somewhere. Someone with a better understanding of the caching/converters may have a better idea.

[poornimanayar]

Hey @cleversolutions ,

Welcome here and congratulations on your first PR! We will review the work and get back to you if we need anything more. Watch out this space :-)

Regards,
Poornima, Pull Request Team

[cleversolutions]

I added a comment to the change, I felt it needed an explanation.

[nul800sebastiaan]

Really cool @cleversolutions - I didn't think of dynamically adding these crops if they don't yet exist. I will need to talk to some people at HQ to see if this is a workable solution. Will get back to you!

[cleversolutions]

Hey @nul800sebastiaan is there anything you need from me for this PR?

[nul800sebastiaan]

Thanks for the reminder @cleversolutions - I am not sure if we should do this, it seems like the performance of the GetCropUrl would suffer quite severely if we have to go to the database for each call until someone actually saves the crops for all the affected media items. I'll have to ask around.

[Shazwazza]

Hi, this should instead be done in the ImageCropperValueConverter instead of in this ext method which is responsible for populating the ImageCropperValue model. You can apply the same/similar technique there and it should be fine. However, you can probably use the method DataType.ConfigurationAs<ImageCropperConfiguration>() instead of doing all that try/cast.

Since this data is coming from the front-end cache, the way access to a DataType works is that it does hit the DB once but not over and over again so the way you are accessing the DataType is fine.

[cleversolutions]

Thanks @Shazwazza, I’ll work it in like that instead.

[cleversolutions]

So it would seem that I did something very wrong while rebasing this pull request. I did a

git fetch upstream
git rebase upstream/v8/dev

Then committed, but now this PR is polluted with every other change that's happened in the past few months. Is the best bet now to just close this PR and I'll open a new one?

@Shazwazza thanks for tip about looking into the PropertyValueConverters. The code to do this is already there, but it wasn't executing because of some null reference checking. Essentially this can all work with 2 small lines of code change.

[cleversolutions]

I couldn't figure out how to fix the mess I made of this so I re-created the branch and have submitted a new pull request #6950