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

misc: Replace sha1() by hash('sha1', ...) #544

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Conversation

TimWolla
Copy link
Contributor

@TimWolla TimWolla commented Jul 5, 2024

The dedicated sha1() function is proposed for deprecation with PHP 8.4: https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_md5_sha1_md5_file_and_sha1_file

For a future change it might also be considered to move to a cheaper hash function. The xxHash family (available as xxh*) is faster and should be sufficiently collision resistant for use within the cache.

The dedicated `sha1()` function is proposed for deprecation with PHP 8.4:
https://wiki.php.net/rfc/deprecations_php_8_4#deprecate_md5_sha1_md5_file_and_sha1_file

For a future change it might also be considered to move to a cheaper hash
function. The xxHash family (available as `xxh*`) is faster and should be
sufficiently collision resistant for use within the cache.
@romm
Copy link
Member

romm commented Jul 19, 2024

Hi @TimWolla thank you!

For a future change it might also be considered to move to a cheaper hash function.

Could we not use it already? What particular function(s) would work here?

@TimWolla
Copy link
Contributor Author

TimWolla commented Jul 19, 2024

Could we not use it already?

We probably can. For this PR I just proposed the minimally viable change, because I did not have the time to thoroughly investigate what replacement would be appropriate.

What particular function(s) would work here?

As suggested in the commit message, the xxHash family might be appropriate. An alternative might be using preg_replace() to replace any special characters by an "escape sequence". Something like the following:

preg_replace_callback('/[^a-zA-Z0-9]/', static function ($matches) {
  return sprintf('_%02x', ord($matches[0]));
}, $key);

This would be guaranteed to be collision resistant, because it's a bijection. It would also make the cache identifiers human-readable.

@romm
Copy link
Member

romm commented Jul 23, 2024

For future reference: symfony/symfony#47094

@romm
Copy link
Member

romm commented Sep 18, 2024

Hello @TimWolla, thanks for the original work and explanations.

After reading a bit about this topic (check this php.watch article for instance), I've decided to use the xxh128 hash, as Valinor requires PHP 8.1+.

I also moved the hash logic into one place only.

I'll wait for your approbation before merging.

NB: squash commit message to be used:

misc: use `xxh128` hash algorithm for cache keys

This algorithm is way faster than the originally used `sha1`.

This commit also moves the hash logic into one place, saving even more
processing time. 

Reference: https://php.watch/versions/8.1/xxHash

@TimWolla
Copy link
Contributor Author

I'll wait for your approbation before merging.

Changes look good to me. You are the Valinor expert anyway 😄 The primary goal of this PR was not to be merged directly, but to start a discussion for you to make a good decision.

@romm
Copy link
Member

romm commented Sep 18, 2024

I wouldn't be an expert without the community around this tool, composed by many excellent PHP developers. 😉

Thanks again!

@romm romm merged commit 546c458 into CuyZ:master Sep 18, 2024
11 checks passed
@TimWolla TimWolla deleted the no-sha1 branch September 18, 2024 13:32
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