The description for the output of Crypt::randomBytesBase64() says it will be up to 4 * $count, but that is not true.
Using the Base64 encoding the output of $count bytes is 4/3 * $count bytes.
Instead of describing the maximum output length, which is not specific for the Drupal implementation, the description for the output should be similar to the one used for Crypt::hashBase64(), for example.
A base-64 encoded string, with + replaced with -, / with _ and any = padding characters removed.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | drupal-fix-Crypt-randomBytesBase64-output-description-3118581-2.patch | 738 bytes | avpaderno |
Comments
Comment #2
avpadernoComment #3
ilgnerfagundes commentedLooks good, thanks for your work.
Kind regards, Ilgner
Comment #4
avpadernoComment #5
alexpottYeah that documentation is wrong. The new docs are more helpful. Backported to 8.8.x as this is a docs only fix.
Committed d2d8fc9 and pushed to 9.0.x.
Committed 192a49e and pushed to 8.9.x.
Committed 341d1c5ac8 and pushed to 8.8.x. Thanks!
Comment #9
ilgnerfagundes commented@alexpott can you give me credit for testing?
Comment #10
alexpottHi @ilgnerfagundes thank you for reviewing the patch. Assigning credit is one of the hardest parts of being a committer. When you do post a review, be sure to describe what you reviewed and how. This helps other reviewers understand why you considered the issue RTBC (and is considered for issue credit). For a simple change like this it's hard to credit an RTBC - but you could have detailed why the new documentation is sufficient and removing details about the length of the return value is okay. That's what I what I had to check when I reviewed the patch prior to committing it.
Comment #14
quietone commentedClosed an earlier issue #2866495: Crypt::randomBytesBase64 documentation for length of returned string is wrong as a duplicate, adding credit.