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

The ImageCropperPropertyValueEditor doesn't convert values in ConvertDbToString correctly #5134

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

Shazwazza
Copy link
Contributor

It is trying to serialize as json but it is failing since it's just concatenating objects and the ToString() of the crops is not the actual json of the crops. Have fixed this to properly serialize.

@cleversolutions
Copy link
Contributor

Hey @Shazwazza would this by why media.GetCropUrl("alias") returns null?

@Shazwazza
Copy link
Contributor Author

@cleversolutions i don't know - i'm not sure what version you are using? is it latest 8.1 from code or 8.0.x ?

@azure-devops-sync
Copy link

azure-devops-sync bot commented Jun 26, 2019

This item has been added to our backlog AB#1518

@cleversolutions
Copy link
Contributor

cleversolutions commented Jun 26, 2019

8.0.2. Without getting too deep into the code it seemed like maybe the Codes property was null which causes getcropurl to return a null. Since codes came from property values it seemed likely at first blush this might cause that. I didn’t debug this, just read through some code in github to see if something stuck out.

@warrenbuckley
Copy link
Contributor

@Shazwazza do you have repro steps for me to test before/after the PR is applied, please.

Repro Steps Used

As I have tested before the PR is applied and done the following steps without any issues:

  • V8 with SK installed
  • Update Image Media Type -> Update umbracoFile Image Cropper DataType settings to include two crop types
  • Goto Media Section & Choose a Product Image
  • Set manual crops for the image
  • Update Product.cshtml / template with <img src='@Model.Photos.GetCropUrl("square")' />
  • View page on frontend & confirm image renders out all OK

@cleversolutions
Copy link
Contributor

@warrenbuckley I followed your repro steps with 8.0.2. It works in 8.0.2 only for images you have saved after adding the new crop alias. One would not expect to have to re-save all their images after adding a new crop alias. I haven't tested this PR to see if it changes that behavior.

@cleversolutions
Copy link
Contributor

I just tested this PR, it doesn't seem to have changed the behavior. Crops is still null unless you save the image after creating the crop alias.

@warrenbuckley
Copy link
Contributor

@Shazwazza I am not sure this change affects anything as far as I know, as the usage seems to refer to legacy XML cache.

When you get time do you mind if you can see if you agree with me on this or are able to give me some repro steps for this?

I agree the serialization is better and be OK to merge this, but I can never seem to hit a breakpoint at the moment for where this code was changed, to see the fix before & after.

Let me know what you want me to do with this please mate :)

@cleversolutions
Copy link
Contributor

Sorry I threw a red herring in there with my comments about GetCropUrl. I’ll create a new issue for it.

@warrenbuckley
Copy link
Contributor

Merged :)

@warrenbuckley warrenbuckley merged commit ef19633 into v8/dev Jun 27, 2019
@warrenbuckley warrenbuckley deleted the v8/bugfix/image-cropper-converter branch June 27, 2019 11:20
@cleversolutions
Copy link
Contributor

cleversolutions commented Jun 27, 2019

I added issue #5737 to track the problem I noted above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants