Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118846
Message-ID: <312c7bee-2258-d4f4-a53d-892f5c66a07f@wikimedia.org>
Date: Wed, 19 Oct 2022 15:14:06 +1100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Content-Language: en-US
To: =?UTF-8?Q?Tim_D=c3=bcsterhus?= <tim@bastelstu.be>, internals@lists.php.net
References: <22177032-fe72-c39b-63fe-fa4368a70852@bastelstu.be>
 <ef9fa4be-09e7-56a3-9552-76ded6af2e87@wikimedia.org>
 <5d5849c5-ca79-71e9-ccc8-5c69952fd408@bastelstu.be>
In-Reply-To: <5d5849c5-ca79-71e9-ccc8-5c69952fd408@bastelstu.be>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
From: tstarling@wikimedia.org (Tim Starling)

On 18/10/22 22:32, Tim Düsterhus wrote:
>
> false is a valid value that might've been serialized. The manual 
> specifically notes:
>
>>  false is returned both in the case of an error and if 
>> unserializing the serialized false value. It is possible to catch 
>> this special case by comparing data with serialize(false) or by 
>> catching the issued E_NOTICE. 

Yes, that is fair enough.


>
> Note that your example code also errors out if the $result is an 
> empty array, an empty string or another falsy values. This might be 
> an acceptable result in your specific case, but is not the correct 
> behavior in the general case.

Right, there was no instance in the 150 callers I reviewed yesterday 
in which unserialize() was explicitly checked for errors while 
unserializing a falsy value. Most often, there is an array wrapper 
around the actual value, or the possibility is otherwise excluded.


>
> The example in the RFC was written to be correct for the general 
> case, without imposing any constraints in the input data.

As I said, if throwing was optional, like in json_decode(), I would be 
all for it. I'm just doing a cost/benefit analysis. The common case 
should be easy, while the general case should be possible.

As a matter of style, I think PHP's false returns are fine. I don't 
think we need to follow Python into making every error an exception.

-- Tim Starling