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

3.x-use-xxh3 #396

Merged
merged 1 commit into from
Jun 25, 2024
Merged

3.x-use-xxh3 #396

merged 1 commit into from
Jun 25, 2024

Conversation

nlemoine
Copy link

@nlemoine nlemoine commented May 17, 2024

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?

@nlemoine nlemoine changed the base branch from master to 3.x May 17, 2024 07:35
@ADmad
Copy link
Collaborator

ADmad commented May 26, 2024

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 #153 (comment). 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
Copy link
Author

@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
Copy link
Collaborator

ADmad commented May 27, 2024

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

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.

@ADmad ADmad changed the title 3.x-use-xxh3 3.x-use-xxh128 Jun 21, 2024
@ADmad ADmad changed the title 3.x-use-xxh128 3.x-use-xxh3 Jun 21, 2024
@ADmad ADmad merged commit 9afc803 into thephpleague:3.x Jun 25, 2024
9 checks passed
@nlemoine nlemoine deleted the 3.x-use-xxh3 branch June 25, 2024 18:30
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.

2 participants