Re: New functions `hash_serialize` and `hash_unserialize`?

Hi all, I've opened up a pull request and responded to this message there. I'd love any further comments. https://github.com/php/php-src/pull/5702 Eddie On Thu, Jun 11, 2020 at 5:42 PM Sara Golemon <pollita@php.net> wrote: > WARNING: Harvard cannot validate this message was sent from an authorized > system. Please be careful when opening attachments, clicking links, or > following instructions. For more information, visit the HUIT IT Portal and > search for SPF. > ------------------------------ > On Thu, Jun 11, 2020 at 11:59 AM Eddie Kohler <kohler@seas.harvard.edu> > wrote: > >> Thanks for this suggestion. I've updated the implementation to make >> HashContext implement Serializable. >> >> I'd still be grateful for more feedback, or perhaps I should just create >> an RFC? >> >> Be careful what you ask for. :) > > Overall +1 on the concept with a few notes: > > 1. Please put this on a branch and make it a PR so we can comment on it > directly. > 2. Consider using zend_parse_parameters_throws() and family so that the > exception which is thrown contains the type error information rather than > the generic RETURN_THROWS() macros. > 3. Consider using hex or base64 to serialize the contexts. This will > reduce various transport/storage issues. > 4. It's great that you've thought about endianness, but the current > implementation simply bails on endian mismatch. It'd be a nice-to-have for > the user if these serializations were portable. I know this represents a > lot of work for sort of an edge case so I won't hold it against you if you > say 'no' and/or save this for later work if demand surfaces. > 5. Storing $key makes me nervous. I don't have a good solution to this > since the deserialization doesn't actually give us a chance to specify it > in the deserialization process. I wish I'd made $key/hmac an option to > hash_final rather than hash_init. Maybe we can think about allowing that > to be specified at either end. Let's expand on this topic while you work > on your RFC. > 6. Yeah... I think you need an RFC because of #5. Sorry. > 7. TABS v SPACES indentation issues. > > -Sara > >