Actually decided to post so On Sat, 2 Jul 2016 at 09:16 Leigh <lei...@gmail.com> wrote:
> On Sat, 2 Jul 2016 at 08:36 Yasuo Ohgaki <yohg...@ohgaki.net> wrote: > >> Hi all, >> >> Currently session module uses obsolete MD5 for session ID. With >> CSPRNG, hashing is redundant and needless. It adds hash module >> dependency and inefficient (There is no reason to use hash for CSPRNG >> generated bytes). >> >> This proposal cleans up session code by removing hash. >> >> https://wiki.php.net/rfc/session-id-without-hashing >> >> I set vote requires 2/3 support. >> Please describe the reason why when you against this RFC. Reasons are >> important for improvements! >> > > So I have a few issues that span the RFC and the implementation. Your RFC states > hardcoded default and php.ini-* default values are the same. This is not the case. Originally the session id length and character set were controlled by session.hash_function and/or session.hash_bits_per_character. These customisations to configuration will be lost when the user upgrades. You have provided a mechanism to control length and charset, but it will require new changes to the default settings. This needs to be noted as a breaking change. Your default for session.sid_length is 48. Up to 7.1 the session id length is 32. Your default for session.sid_bits_per_character is 5, up to 7.1 the session id uses 4 bits per character. This is a breaking change. (Imagine custom session handlers that validate session id character sets, or database schemas that will truncate after 32 characters) Your patch updates session.use_strict_mode from 0 to 1. I actually don't know what this changes, but it's an undocumented change. Overall your patch looks very similar to the one I was working on earlier in the year, although you appear to have deleted a bunch of tests that you could have just updated. You should probably put those back, and update them.
