Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:110492
MIME-Version: 1.0
References: <CAF0C5SL8c_2TGPKCxo37nbRseA=2ZLoQBtUzZ01DDTgmJNSC8A@mail.gmail.com>
 <aabd8ab36a5894b06bbcb284dc18b56cafaf852e.camel@schlueters.de> <CAF0C5S+920VXCk5ba5KoyYT-8-GmH-xtTPquD4b3_Z=mTTJkVA@mail.gmail.com>
In-Reply-To: <CAF0C5S+920VXCk5ba5KoyYT-8-GmH-xtTPquD4b3_Z=mTTJkVA@mail.gmail.com>
Date: Thu, 11 Jun 2020 16:41:51 -0500
Message-ID: <CAESVnVrpmL3pYHwx973CZMhvAEMEWVijUiXoHt4mdyYi9aGjpg@mail.gmail.com>
To: Eddie Kohler <kohler@seas.harvard.edu>
Cc: =?UTF-8?Q?Johannes_Schl=C3=BCter?= <johannes@schlueters.de>, 
	PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="000000000000bb7bc405a7d5d4ac"
Subject: Re: [PHP-DEV] New functions `hash_serialize` and `hash_unserialize`?
From: pollita@php.net (Sara Golemon)

--000000000000bb7bc405a7d5d4ac
Content-Type: text/plain; charset="UTF-8"

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

--000000000000bb7bc405a7d5d4ac--