Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118852
Message-ID: <96df967d-c5e8-9c56-bc6d-9858e28294fa@bastelstu.be>
Date: Wed, 19 Oct 2022 19:11:26 +0200
MIME-Version: 1.0
Content-Language: en-US
To: Dan Ackroyd <Danack@basereality.com>
Cc: PHP internals <internals@lists.php.net>
References: <22177032-fe72-c39b-63fe-fa4368a70852@bastelstu.be>
 <CA+kxMuR=371QXKbD_OVRu+oS7=_qPtL1yN93651wmH-vVVzGhw@mail.gmail.com>
In-Reply-To: <CA+kxMuR=371QXKbD_OVRu+oS7=_qPtL1yN93651wmH-vVVzGhw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Subject: Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)

Hi

On 10/18/22 14:04, Dan Ackroyd wrote:
> function getUserPreferences($data) {
>    $user_preferences = @unserialize($data);
>    if ($user_preferences === false) {
>      $user_preferences = get_default_preferences();
>    }
>    return $user_preferences;
> }
> 
> […]

I agree that this implementation 'getUserPreferences()' is somewhat 
reasonable code if the sole use case for the error handling is dealing 
with an empty string or NULL default value in your database (the latter 
will break with 
https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg, though). 
That's why I've only left the "change this in 9.0" option when starting 
the vote, instead of also providing the "change this in 8.x" option.

FWIW: The same pattern exists in the application I maintain, but I 
already started on transforming this to remove the '@' and add a 
'catch(Throwable)'.

> but I'm not having great ideas of how that could be done 'nicely'.

Yes, this specific example suffers from the '@' already being in place 
and thus suppressing a deprecation.

> Similar for the exception hierarchy change (example is below).
> Changing the type of exception thrown would break valid (if not super
> great) code in a way that would be hard to detect, and I'm not sure we
> have a good way of performing this type of change.

While the behavior would in fact change, it would not introduce a 
"security issue" if this what you were hinting at. This would just 
result in an uncaught Exception which should be very visible in your 
error tracking service.

I'd also like to note that the behavior is not really well-defined for 
current versions either. Consider that the user object internally stores 
an array with other User objects ($followedUsers). Now when a followee 
(is that a legal word?) is deleted, this would result in a spurious logout.

> btw I think there are underlying interlinked problems with PHP that
> causes proposed changes like these to be more difficult than anyone
> would want:
> 

I agree with all three points. The API surface of what can be considered 
a default installation of PHP is huge and subsystems can't be versioned 
independently, thus holding subsystems in need to a modernization 
hostage to the version number. I'm in a very similar situation for the 
application I maintain, it also has large API surface with a unified 
version number.

Best regards
Tim Düsterhus