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

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

Closed
hfloyd opened this issue May 14, 2020 · 11 comments
Closed

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

hfloyd opened this issue May 14, 2020 · 11 comments
Labels
community/up-for-grabs state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks status/needs-docs type/bug

Comments

@hfloyd
Copy link
Contributor

hfloyd commented May 14, 2020

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
Copy link
Contributor

kjac commented May 16, 2020

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
Copy link
Contributor

kjac commented May 16, 2020

@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
Copy link
Contributor Author

hfloyd commented May 18, 2020 via email

@kjac
Copy link
Contributor

kjac commented May 18, 2020

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
Copy link
Contributor Author

hfloyd commented May 18, 2020

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
Copy link
Contributor

kjac commented May 18, 2020

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 nul800sebastiaan added the state/needs-investigation This requires input from HQ or community to proceed label May 19, 2020
@nul800sebastiaan
Copy link
Member

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 nul800sebastiaan added type/bug status/needs-docs community/up-for-grabs and removed state/needs-investigation This requires input from HQ or community to proceed labels Jun 16, 2020
@nul800sebastiaan
Copy link
Member

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

@umbrabot
Copy link

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

@bergmania bergmania added the state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks label Jan 14, 2021
@warrenbuckley
Copy link
Contributor

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
Copy link
Contributor Author

hfloyd commented Feb 2, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community/up-for-grabs state/sprint-candidate We're trying to get this in a sprint at HQ in the next few weeks status/needs-docs type/bug
Projects
None yet
Development

No branches or pull requests

7 participants