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

precision loss when GMP is not installed? #255

Closed
ben-mckenzie opened this issue Dec 9, 2024 · 3 comments · Fixed by #256
Closed

precision loss when GMP is not installed? #255

ben-mckenzie opened this issue Dec 9, 2024 · 3 comments · Fixed by #256

Comments

@ben-mckenzie
Copy link

Installations without GMP fall back to using PHP's native base_convert function which isn't designed to handle very large inputs like SHA3-512. The resulting ID values have an odd-looking pattern to them (note the amount of 0's, 4's and 8's for example):

bg48okgw0wwooo0g0488s4so
egcs480os8gkk84sgkgcgks8
fc00w044cckow4ock4s0w0sw
iw8ogk8woowk0cw8c8kgoscw
k40040s4kcsgsk0wg8scks8o
pwgo80o4c4kogk4gcs8oko8k
r8000cc8ggcsw0s8s44s440w
uc8gk8gog04wc044k4ck4s8c
zokgk80ww4o0sksws8s44osc
zwowwk4sc40c00ws44ww0gso

The issue is even noticeable in the example IDs shown in the readme 😄

When GMP is installed this issue goes away, but it should probably be made much clearer that GMP is needed for this library to work correctly.

Alternatively the dependency on GMP could be replaced with the library phlib/base_convert which seems to get the job done.

@xaevik
Copy link
Contributor

xaevik commented Dec 9, 2024

Hi @ben-mckenzie

Great catch, will have a fix out shortly.

@ben-mckenzie
Copy link
Author

Thanks for the quick response! By the way, I ran into this mainly because Laravel Sail doesn't include GMP by default. Would you ever consider the idea of using phlib/base_convert (or some other userland implementation) as a fallback? I'm guessing the main reason not to would be the giant performance hit, but for dev environments it might be an acceptable tradeoff. Up to you really.

@xaevik
Copy link
Contributor

xaevik commented Dec 10, 2024

Hi @ben-mckenzie

For libraries such as this, I prefer they have no external dependencies (if it can be helped). With that said I also do not like to impose a requirement on a non-default PHP extension either which is why GMP was originally optional but obviously it is unavoidable given the bug, hence the new release making it required.

I did look at phplib/base_convert but unfortunately, it's licensed under GPL (of which I am not a fan). If I find a viable alternative that is either MIT or Apache-2 licensed, I will make the GMP extension requirement optional again.

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

Successfully merging a pull request may close this issue.

2 participants