On Thu, Jun 11, 2020 at 11:59 AM Eddie Kohler <koh...@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
