Added more entropy to uniqid #133

Merged
merged 1 commit into from Feb 14, 2017

Projects

None yet

3 participants

@Nyholm
Member
Nyholm commented Feb 9, 2017 edited

Thank you for this PR. I cannot merge this in the current state though... According to RFC2046 a valid boundary is:

     boundary := 0*69<bchars> bcharsnospace
     bchars := bcharsnospace / " "
     bcharsnospace := DIGIT / ALPHA / "'" / "(" / ")" /
                      "+" / "_" / "," / "-" / "." /
                      "/" / ":" / "=" / "?"

Be aware that:

echo uniqid();          // 589c62cd00001
echo uniqid('', true);  // 589c62cd000005.45285185

The tricky part is the extra period that this PR introduces. The RFC says that such bounderies must be quoted.

The following is not valid:

Content-Type: multipart/mixed; boundary=gc0pJq0M.08jU534c0p

and must instead be represented as:

Content-Type: multipart/mixed; boundary="gc0pJq0M.08jU534c0p"

We are not quoting at all currently. We should make sure to quote the boundary where ever we are using it.

@lyrixx
Contributor
lyrixx commented Feb 9, 2017

Or I can simply hash the boundary ?

@Nyholm
Member
Nyholm commented Feb 9, 2017

Sure, I created a small script to measure times. Here is the output:

uniqid():         11.343955993652 ms
uniqid('', true): 1.3971328735352 ms
sha1:             2.5057792663574 ms
md5:              2.4011135101318 ms
base64_decode:    2.471923828125 ms

"sha1" means sha1(uniqid('', true))

So yes, Sure. Hashing makes sense, It will work and you will get the speed performance.

@Nyholm Nyholm referenced this pull request in php-http/multipart-stream-builder Feb 9, 2017
Merged

Use uniqid('', true) for performance reasons #28

@lyrixx
Contributor
lyrixx commented Feb 9, 2017

PR Updated.

@Nyholm
Nyholm approved these changes Feb 9, 2017 View changes
@sagikazarmark
Contributor

This is great, thanks.

@sagikazarmark sagikazarmark merged commit 662ca4c into guzzle:master Feb 14, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lyrixx lyrixx deleted the lyrixx:uniqid branch Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment