Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118609
MIME-Version: 1.0
References: <530b3a9d-0ee4-6061-8c69-df672d238032@bastelstu.be> <CAOWwgpnQuRMv4Uv=bM+QNBPeUC8BCk4f1kYOcg8EUrYD1WoRzQ@mail.gmail.com>
In-Reply-To: <CAOWwgpnQuRMv4Uv=bM+QNBPeUC8BCk4f1kYOcg8EUrYD1WoRzQ@mail.gmail.com>
Date: Mon, 12 Sep 2022 12:19:25 +0100
Message-ID: <CA+kxMuR+AdK3YUdU_oNb5D1tgqGaK7gMJZZyNO5+jMXbz4HZfg@mail.gmail.com>
To: Nicolas Grekas <nicolas.grekas+php@gmail.com>
Cc: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>, 
	PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Subject: Re: [PHP-DEV] RFC [Discussion]: Improve unserialize() error handling
From: Danack@basereality.com (Dan Ackroyd)

Hi Nicolas,

On Thu, 8 Sept 2022 at 18:06, Nicolas Grekas
<nicolas.grekas+php@gmail.com> wrote:
>
> There needs to be serious justification for it.

I think this is covered by the RFC.

The current behaviour where people have to setup a custom error
handler, and then restore the previous error handler, is pretty
'ungood'.

The proposed replacement behaviour, where people can actually find out
the problem that happened is better.

> 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?

A new option might be a good idea, but unserialize should have
sensible defaults, and the current behaviour is really not that
sensible.

So, imo, an opt-out would be a better choice. It would allow anyone
who cares enough to keep backwards compatibility when they upgrade to
the next version of PHP by adding that to the options array, but
everyone who is unaware of the option gets the more sensible behaviour
by default.

cheers
Dan
Ack