-
-
Notifications
You must be signed in to change notification settings - Fork 139
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
Polyfill the random extension's interfaces and Secure engine #413
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I was thinking to submit a similar one too, I'm glad you did.
Should we wait for the discussion on mailing list to conclude? I remember Go Kudo was following up the RFC to make some suggestions?
Also, let's add a provide
entry to Php82/composer.json
:
"provide": {
"ext-random": "*"
}
The engines and interfaces are basically set in stone now. There is not much that can be changed about them.
No:
|
@TimWolla how about including |
This is not acceptable, as it would not actually polyfill the engine (which is separate from the |
Indeed. More specifically it would not allow for two separate Engines to exist either. |
Crazy idea: implement MT19937 by looking at https://en.wikipedia.org/wiki/Mersenne_Twister#Pseudocode and/or php-src and/or by leveraging https://github.com/ruafozy/php-mersenne-twister/ (but as optional dep if we go this way) |
This package relies on |
Frankly I don't see much benefit there. Even reimplementing Mt19937 is non-obvious, because PHP only has signed integers that will convert to double instead of wrapping. Also the engines are pretty much useless without the Randomizer and you shouldn't use FWIW: This library provides an (almost) drop-in backport: https://github.com/arokettu/php-random-polyfill. |
Thanks for the link to arokettu/php-random-polyfill. Given this work, wouldn't it make sense to not merge this PR? What about merging arokettu/php-random-polyfill into symfony/polyfill? We'd go with a dedicated package named /cc @arokettu would that make sense to you? |
I believe there is some value in having the interfaces and exceptions available, e.g. to create a package providing custom engines without pulling in the full polyfill, as the polyfill is pretty heavy. I don't have a strong preference, though. |
@nicolas-grekas I'm not opposed, but please review the code whether you really want all that crazy and hackish code in Symfony, also note https://github.com/arokettu/unsigned it depends on |
bd65979
to
a39d1a9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rebased and pushed a second commit to fix some details I borrowed from @arokettu's work on the topic.
After reviewing both this PR and https://github.com/arokettu/php-random-polyfill more carefully, I agree it makes sense to provide what you propose @TimWolla.
I also agree that merging @arokettu's polyfill might be too involving. Instead, I added a link to it in the readmes.
I'm also going to send a PR to your work @arokettu, to leverage polyfill-php82 ^1.27.
a39d1a9
to
ebc6b56
Compare
Thank you @TimWolla. |
No description provided.