Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118588
MIME-Version: 1.0
References: <530b3a9d-0ee4-6061-8c69-df672d238032@bastelstu.be>
In-Reply-To: <530b3a9d-0ee4-6061-8c69-df672d238032@bastelstu.be>
Date: Thu, 8 Sep 2022 19:06:16 +0200
Message-ID: <CAOWwgpnQuRMv4Uv=bM+QNBPeUC8BCk4f1kYOcg8EUrYD1WoRzQ@mail.gmail.com>
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="0000000000003e50d605e82d7304"
Subject: Re: [PHP-DEV] RFC [Discussion]: Improve unserialize() error handling
From: nicolas.grekas+php@gmail.com (Nicolas Grekas)

--0000000000003e50d605e82d7304
Content-Type: text/plain; charset="UTF-8"

Hi Tim,

Thanks for the RFC.

I've now written up an RFC as a follow-up for the "What type of
> Exception to use for unserialize() failure?" thread [1]:
>
> ----
>
> RFC: Improve unserialize() error handling
> https://wiki.php.net/rfc/improve_unserialize_error_handling
>
> Proof of concept implementation is in:
>
> https://github.com/php/php-src/pull/9425
>
> Discussion period for that RFC is officially opened up.
>
> ----
>
> The primary point of discussion in the previous mailing list thread and
> in the PR comments is whether unserialize() should continue to emit
> E_WARNING or whether that should consistently be changed to an
> Exception. As of now I plan to explicitly vote on this and the RFC
> contains some opinions on that matter.
>

From what I understand, there are two things in the RFC:

   1. a proposal to wrap any throwables thrown during unserialization
   inside an UnserializationFailedException
   2. a discussion to figure out a way to turn these notices+warnings into
   exceptions.

About 1., I read that you think "this is not considered an issue", but to
me, changing the kind of exceptions that a piece of code might throw is a
non negligible BC break. There needs to be serious justification for it. I
tried looking for one, but I'm not convinced there is one: replacing the
existing catch(Throwable) by catch(UnserializationFailedException) won't
provide much added value to userland, if any. And "reliably include more
than just the call unserialize() in the try" is not worth the BC break IMHO.

About 2., unserialize() accepts a second argument to give it options. Did
you consider adding a 'throw_on_error' option to opt-in into the behavior
you're looking for? I think that would be a nice replacement for the
current logics based on set_error_handler(). If we want to make PHP 9 throw
by default, we could then decide to deprecate *not* passing this option.

Lastly I'd like to add a 3. to your proposal, because there is one more
thing that makes unserialization annoying: the unserialize_callback_func
<https://www.php.net/manual/var.configuration.php#ini.unserialize-callback-func>
ini setting. It would be great to be able to pass a 'callback_func' option
to unserialize to provide it, instead of calling ini_set() as we have to
quite often right now.

Would that make sense to you?

Kind regards,
Nicolas

--0000000000003e50d605e82d7304--