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

Make sure the hash key changes after cropping an image. #142

Merged
merged 5 commits into from
Sep 22, 2023

Conversation

maethu
Copy link
Member

@maethu maethu commented Sep 1, 2023

Problem:

Even though an image gets changed via cropping it does maintain the same url. Which leads cache/proxy and browsers to potentially not download the new version of the cropped image.

Example:
After uploading a new image the url to the thumb scale is http://nohost/plone/testimage/@@images/image-128-fba13e5e2040bf30b80360aeaf2bb42f.png

After cropping the url is still http://nohost/plone/testimage/@@images/image-128-fba13e5e2040bf30b80360aeaf2bb42f.png

Explenation:
One part of the hash key in the default storage (IImageScaleStorage) is the _p_mtime (modification time if an object got changed during a transaction). This is basically the only parameter that we can change, without rewriting the storage, image factory and utilities.

Solution:
Mark the ImageBlobField as changed if a scale gets cropped.
This way we force the storage to generate a new has key for (all) the scales.

@maethu
Copy link
Member Author

maethu commented Sep 1, 2023

@petschki
I'm sorry can't really tell what's wrong with the tests. I might need help to fix those.

Copy link
Member

@petschki petschki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
I've fixed some minor CI issues. Should be green now.

@MrTango MrTango merged commit 1f54ffe into master Sep 22, 2023
8 checks passed
@petschki petschki deleted the mle-force-new-hash-key branch September 25, 2023 07:37
@petschki
Copy link
Member

Somehow the test is broken after merging it into master. @maethu could you eventually take a look again?

@MrTango
Copy link
Contributor

MrTango commented Sep 25, 2023

strange, the strings look exactly the same

@petschki petschki mentioned this pull request Sep 25, 2023
@MrTango
Copy link
Contributor

MrTango commented Sep 25, 2023

strange, the strings look exactly the same

which is the problem, they shouldn't :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants