Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118841
MIME-Version: 1.0
References: <22177032-fe72-c39b-63fe-fa4368a70852@bastelstu.be>
In-Reply-To: <22177032-fe72-c39b-63fe-fa4368a70852@bastelstu.be>
Date: Tue, 18 Oct 2022 14:04:34 +0200
Message-ID: <CA+kxMuR=371QXKbD_OVRu+oS7=_qPtL1yN93651wmH-vVVzGhw@mail.gmail.com>
To: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
From: Danack@basereality.com (Dan Ackroyd)

Hi Tim,

On Fri, 14 Oct 2022 at 15:44, Tim D=C3=BCsterhus <tim@bastelstu.be> wrote:
>
> Hi
>
> as announced on Wednesday [1] I've now opened the vote for:
>
> "Improve unserialize() error handling" [2]


I'm going to have to vote no for changing to throwing an exception.
The BC break is too big imo, and too hard for people to be aware of.

IMO There needs to be a clear path of deprecating the current
behaviour that people are alerted to (rather than having to read
upgrade notes), so that they know they need to change their code from:

function getUserPreferences($data) {
  $user_preferences =3D @unserialize($data);
  if ($user_preferences =3D=3D=3D false) {
    $user_preferences =3D get_default_preferences();
  }
  return $user_preferences;
}

to something like:

function getUserPreferences($data) {
  try {
    return unserialize($data);
  }
  catch (UnserializationFailedException $ufe) {
return get_default_preferences();
  }
}

but I'm not having great ideas of how that could be done 'nicely'.

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.

IMO these isn't just a BC break that needs to wait for a major version
release; it's something that needs to have a clear upgrade path no
matter which version they are proposed for.

btw I think there are underlying interlinked problems with PHP that
causes proposed changes like these to be more difficult than anyone
would want:

1. The PHP core library is 'ungood' in quite a few aspects, and many
people would like to improve it, but some of those changes are hard to
do in a BC way.

2. Because of the way PHP works, it's not possible for one
library/module to use different versions of the same function. This is
different from JavaScript where with their (ridiculously convoluted*)
tools, each module is able to specify exactly which versions of which
libraries it wants to use.

3. Although PECL was good for the time it was created in (where it was
uncommon for individuals to have places to share files online) it
really isn't a tool fit for purpose any more. It's not really fit for
purpose any more, as even when there are (allegedly) great libraries
like ds (https://www.php.net/manual/en/book.ds.php) relatively few
people using PHP use them.

I think trying to improve the situation, so that changes to individual
functions avoid even requiring a discussion in internals in the first
place, is a worthy goal. But it's one that would require many multiple
years of effort, so is far too difficult to be done by individual
people volunteering their time.

Addressing problems that are far too large for individual contributors
to fix, is possibly a discussion to be taken up by the PHP Foundation.
If anyone who works for a for-profit company has read this far and
their company isn't already sponsoring something like 1% of their
engineers salaries to the foundation, then I encourage you to do so at
https://opencollective.com/phpfoundation .

For the  "Increase the severity of emitted E_NOTICE to E_WARNING"
part... I offer no opinion on the change. I personally think any
project that doesn't convert any unsilenced warning to an exception is
just asking for trouble.

cheers
Dan
Ack

* I wouldn't want to see a 'standard' library for PHP split across
80,000 repositories, but I think that the one (1) standard library
that PHP has is too few. Which possibly gives somes bounds to the
desirable number of libraries for a programming language.



// Example for __unserialize exception

class UserNotFoundException extends \Exception {}

function check_user_exists($user_id)
{
    // check user account hasn't been deleted, otherwise throw:
    throw new UserNotFoundException("exceptions for flow control are great.=
\n");
}

class User
{
    private $id =3D 5;

    function __unserialize(array $data) {
        check_user_exists($data["\x00User\x00id"]);
    }
}


$user =3D new User();
$data =3D serialize($user);

echo "$data \n";

try {
    $result =3D unserialize($data);
}
catch (UserNotFoundException $unfe) {
    // This exception would no longer be caught if the exception type
was changed.
    echo "redirect user to login page\n";
    exit(0);
}

echo "User is valid.\n";