search

thephpleague / glide

Wonderfully easy on-demand image manipulation library with an HTTP based API.
http://glide.thephpleague.com
MIT License
2.54k stars 198 forks source link

3.x-use-xxh3 #396

Closed nlemoine closed 1 month ago

nlemoine commented 3 months ago

Since Glide now requires php ^8.1, replace md5 with xxh3 for shorter and faster hash, see: https://php.watch/versions/8.1/xxHash

Note this is a breaking change that will trigger all images to be recreated.

Maybe that could be introduced behind an optin flag?

ADmad commented 2 months ago

There have been concerns in the past regarding the use of md5 as stated in this issue #153, so I am fine changing it.

While we are at it the code should also be updated as per suggestions in this comment https://github.com/thephpleague/glide/issues/153#issuecomment-242756953. Though I am not sure if hash_hmac() supports xxh3.

I also don't want to inconvenience existing users who are upgrading, so it would be nice to provide an option to continue using the existing hashing mechanism.

nlemoine commented 2 months ago

@ADmad Oh wait, just to clarify, my PR was only targeting the hashing algorithm used to handle unique file names. It was not about signing requests (I'm only using the API part of Glide, e.g. not the "server" part).

As it's stated in the link posted above:

xxHash is not a cryptographic hashing algorithm. For password hashing, use password_hash and its friends. Furthermore, none of the xxHash variants are allowed in hash_hmac function.

xxh is not meant to provide security, just fast hashing.

ADmad commented 2 months ago

Oops sorry, I totally misread the PR. Shorter and faster hashes for filenames is great.

Given that this lib is for generating images "on the fly" I don't think having all the cached images regenerated is an issue. We can document this change in the release notes so that people can clean up existing cached images and/or handle cache warm up themselves.

nlemoine commented 2 months ago

I don't think having all the cached images regenerated is an issue

Depends how much images we're talking about :) I personally don't bother. I'll let you merge this if this ok the way it is then.