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

Polyfill the random extension's interfaces and Secure engine #413

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

TimWolla
Copy link
Contributor

No description provided.

Copy link
Contributor

@Ayesh Ayesh left a 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": "*"
}

@TimWolla
Copy link
Contributor Author

Should we wait for the discussion on mailing list to conclude? I remember Go Kudo was following up the RFC to make some suggestions?

The engines and interfaces are basically set in stone now. There is not much that can be changed about them.

Also, let's add a provide entry to Php82/composer.json:

No:

  • The extension is always included and cannot be compiled out.
  • This does not polyfill the entirety of the extension.
    • Specifically the Randomizer is missing, as that one is much harder to polyfill compared to the engines if the behavior should be 100% identical (which is an important requirement).
    • Also PcgOneseq128XslRr64 is a 128 bit unsigned engine which is very expensive to emulate with signed 64 bit integers (or even signed 32 bit integers).

@IonBazan
Copy link
Contributor

IonBazan commented Sep 9, 2022

@TimWolla how about including Random\Engine\Mt19937 into the polyfill? It should be possible to implement it using mt_rand but seeding the generator will pollute global scope - not sure if that's acceptable.

@stof
Copy link
Member

stof commented Sep 9, 2022

It should be possible to implement it using mt_rand but seeding the generator will pollute global scope - not sure if that's acceptable.

This is not acceptable, as it would not actually polyfill the engine (which is separate from the mt_rand state as one of its main features)

@TimWolla
Copy link
Contributor Author

TimWolla commented Sep 9, 2022

This is not acceptable, as it would not actually polyfill the engine (which is separate from the mt_rand state as one of its main features)

Indeed. More specifically it would not allow for two separate Engines to exist either.

@nicolas-grekas
Copy link
Member

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)

@stof
Copy link
Member

stof commented Sep 9, 2022

or by leveraging https://github.com/ruafozy/php-mersenne-twister/ (but as optional dep if we go this way)

This package relies on eval (for no good reason, as they seems to generate code instead of writing it), which is a no-go to me as it would make that polyfill unusable on servers disabling it.

@TimWolla
Copy link
Contributor Author

TimWolla commented Sep 9, 2022

Crazy idea: implement MT19937 by looking

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 Mt19937 except for compatibility purposes. I've only added Secure, because the implementation is trivial and the interfaces / Exceptions to type against.

FWIW: This library provides an (almost) drop-in backport: https://github.com/arokettu/php-random-polyfill.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 3, 2022

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 symfony/polyfill-php82-random.

/cc @arokettu would that make sense to you?

@TimWolla
Copy link
Contributor Author

TimWolla commented Nov 3, 2022

Given this work, wouldn't it make sense to not merge this PR?

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.

@arokettu
Copy link

arokettu commented Nov 3, 2022

@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

Copy link
Member

@nicolas-grekas nicolas-grekas left a 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.

@nicolas-grekas
Copy link
Member

Thank you @TimWolla.

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.

6 participants