NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

Removed SecureRandom class#15880

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

pierredup wants to merge1 commit intosymfony:masterfrompierredup:remove_secure_random

Closed

Removed SecureRandom class#15880

pierredup wants to merge1 commit intosymfony:masterfrompierredup:remove_secure_random

Conversation

Copy link

Contributor

pierredup commentedSep 23, 2015

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

Remove theSymfony\Component\Security\Core\Util\SecureRandom class and use therandom_bytes function instead. Also include theparagonie/random_compat library for non PHP 7 installs

Note This PR depends on#15879 to be merged first

carsonbot added the Status: Needs Review label

Sep 23, 2015

pierredup mentioned this pull request

Sep 23, 2015

Use random_bytes function if it is available for random number generation#15875

Merged

pierredup force-pushed theremove_secure_random branch 3 times, most recently fromea7d682 to2e2b762Compare

September 24, 2015 07:12

Removed Symfony\Component\Security\Core\Util\SecureRandom class in fa…

a7dd362

…vour of the random_bytes function

pierredup force-pushed theremove_secure_random branch from2e2b762 toa7dd362Compare

September 24, 2015 07:22

Copy link

Member

stof commentedSep 24, 2015

We cannot remove it from the master branch without deprecating it first in 2.8 according to our policy. Otherwise it makes the upgrade to 3.0 painful

Copy link

ContributorAuthor

pierredup commentedSep 24, 2015

@stof the deprecation PR is pending here#15879

Copy link

Member

stof commentedSep 24, 2015

Which means this PR should not exist yet (merging the deprecation PR will make this PR unmergeable because of conflicts)

Copy link

ContributorAuthor

pierredup commentedSep 24, 2015

I'll fix the conflicts when the deprecation PR is merged, I also updated the description to say that this is dependant on the deprecation PR to be merged first.
Having this PR open already means I can get quicker feedback and fix any issues without having to wait for the other PR's to be merged first and then only start with the removal.

Copy link

Member

stof commentedSep 24, 2015

@pierredup most of the work done in this PR would become unnecessary once the deprecation PR is implemented the proper way though. The PR would just remove the class, thesecure_random service and the tests for the class. So getting feedback on the current state of this PR is useless.