search

umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.5k stars 2.69k forks source link

V8 .GetCropUrl("CropAlias") generates invalid ImageProcessor URL #8116

Closed hfloyd closed 3 years ago

hfloyd commented 4 years ago

In the process of migrating a v7 website to v8 I came across an issue with "GetCropUrl()"

This code was not providing a cropped/resized image:

featuredPost.BlogPostThumbnail.GetCropUrl("blogThumbnail")

but when I changed it to this:

featuredPost.BlogPostThumbnail.GetCropUrl(610,329)

it worked.

Checking the generated image src urls...

featuredPost.BlogPostThumbnail.GetCropUrl("blogThumbnail")

generates:

/media/4395/shutterstock_256320622_girl-with-tree.jpg?center=0.46832782948063639,0.58439388147611493&mode=crop&width=610&height=329&rnd=132339387248830000

but featuredPost.BlogPostThumbnail.GetCropUrl("blogThumbnail")

generates:

/media/4395/shutterstock_256320622_girl-with-tree.jpg?crop=0,0.093333333333333338,0,0.097650273224043641&cropmode=percentage&width=610&height=329&rnd=132339387248830000

It seems that those parameters aren't valid for ImageProcessor. If I edit the url in the HTML via Dev console in the browser, and remove "&width=610&height=329" - the image displays properly.

So, perhaps this is a bug in the v8 implementation of .GetCropUrl(ALIAS)...?

_This item has been added to our backlog AB#9962_

kjac commented 4 years ago

Hi @hfloyd 🙂

Thanks for reporting. That doesn't sound too good. Strange it's not been reported sooner...!

I'll dig into it and see what I can come up with 👍

kjac commented 4 years ago

@hfloyd for the life of me I can't reproduce this...

I have an image with some crops defined:

image

GetCropUrl("Crop 1") yields /media/2qujjs5u/refactoring.png?crop=0.53542564655172409,0.60588639277393064,0.17721803160919541,0.26205647402796256&cropmode=percentage&width=400&height=200&rnd=132341346325870000 which works perfectly and produces this:

image

I have also tried applying your parameters (?crop=0,0.093333333333333338,0,0.097650273224043641&cropmode=percentage&width=610&height=329&rnd=132339387248830000) to the same image and it worked - or, I assume it worked, it produced an image at least 😄

hfloyd commented 4 years ago

Hmm… Well, this is a migrated site (7 to 8), so perhaps something is slightly different with the data stored for crops? I don’t know. Or some weird caching thing?

I appreciate you looking into it, Kenn.

kjac commented 4 years ago

Hi again, Heather.

I would be surprised if this was a migration or caching issue. Like I said, the URL parameters you originally posted work just fine on my V8 site. Of course you can try creating a new image with a crop and see what output GetCropUrl() produces.

Question is if something weird is going on in your project? Something that prevents ImageProcessor from working properly?

hfloyd commented 4 years ago

Well, this is a migration, so there is likely plenty of "weird" stuff... but, if I use the width/height version of GetCropUrl(), it does output the correct version of the image. My ImageProcessor configs match up with a standard "new" 8.6.1 install, and the dlls match as well (all compared to the release ZIP contents), so I'm not sure what else to check.

kjac commented 4 years ago

Hmm. Could you post a few more invalid/broken image crop URLs? Maybe I can manage to make one of them fail on a local install.

nul800sebastiaan commented 4 years ago

I can reproduce now, when I put an image cropper directly on a content item (so it's not a media picker) then something like

featuredPost.BlogPostThumbnail.GetCropUrl("blogThumbnail")

Doesn't give the correct result.

After looking in the docs, I got here: https://our.umbraco.com/documentation/getting-started/backoffice/property-editors/built-in-property-editors/image-cropper/#mvc-view-example-to-output-a-banner-crop-from-a-cropper-property-with-the-alias-image

The preferred new way in v8 is apparently to use the UrlHelper, unfortunately the docs are not correct, so I tried this:

@Url.GetCropUrl(featuredPost.BlogPostThumbnail, "blogThumbnail")

This doesn't work because we need to give an IPublishedContent as the first item, so the working code would be:

@Url.GetCropUrl(featuredPost, "blogPostThumbnail", "blogThumbnail")

So: from the IPublishedContent item, get the property with alias blogPostThumbnail and then use the crop named blogThumbnail.

So, a few things here:

  1. We should fix the failing GetCropUrl method
  2. We need to update the docs with the correct code
  3. You can make it work now with the new UrlHelper notation
nul800sebastiaan commented 4 years ago

We'd love some help with this, luckily there's a workaround for now!

umbrabot commented 4 years ago

Hi @hfloyd,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly Umbraco GitHub bot :-)

warrenbuckley commented 3 years ago

GetCropUrl() is marked as obsolete already on the ImageCropperValue strongly typed ModelsBuilder object from the PropertyValueConvertor for using an ImageCropper property editor.

Changing this functionality from returning just the querystring portion of the URL as opposed to the full image url would be deemed a breaking change and with this already marked as obsolete and the documentation suggesting to use the newer approach with the UrlHelper approach.

https://our.umbraco.com/documentation/getting-started/backoffice/property-editors/built-in-property-editors/image-cropper/#mvc-view-example-to-output-a-banner-crop-from-a-cropper-property-with-the-alias-image

Before (obsolete)

Model.FeaturedImage = Umbraco.Core.PropertyEditors.ValueConverters.ImageCropperValue <img src="@Model.FeaturedImage.GetCropUrl("square")" />

After (With new UrlHelper)

Model.FeaturedImage = Umbraco.Core.PropertyEditors.ValueConverters.ImageCropperValue <img src="@Url.GetCropUrl(Model.FeaturedImage, cropAlias: "square")" />

I will close this issue, if we need to relook at this then please re-open this with more details

hfloyd commented 3 years ago

Thanks, @nul800sebastiaan and @warrenbuckley . It looks like the documentation is now matching what Warren suggested, so I guess all is well. :-)