Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118827
Message-ID: <c69c45f7-9836-1858-d90e-df7042bd3f88@bastelstu.be>
Date: Sat, 15 Oct 2022 23:37:01 +0200
MIME-Version: 1.0
To: Nicolas Grekas <nicolas.grekas+php@gmail.com>
Cc: PHP internals <internals@lists.php.net>
References: <22177032-fe72-c39b-63fe-fa4368a70852@bastelstu.be>
 <CAOWwgpmuiRzUwGpU3ShQ7HOAVGhcP=Wb8z50acLfERMn_NEbwQ@mail.gmail.com>
 <c9e61189-7826-844f-56b9-7bb509a0791d@bastelstu.be>
 <CAOWwgpnpBTx9-aLz-bg1xZ9FT5JyWsX44TLwfvanDS_aAW7qdQ@mail.gmail.com>
Content-Language: en-US
In-Reply-To: <CAOWwgpnpBTx9-aLz-bg1xZ9FT5JyWsX44TLwfvanDS_aAW7qdQ@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

Resending the message without attachments, because the mailing list 
rejected the original due to exceeding 30000 bytes:

> ezmlm-reject: fatal: Sorry, I don't accept messages larger than 30000 bytes (#5.2.3)

You can find the attachments at: 
https://gist.github.com/TimWolla/376dd162f7684daef38f76a07254871c

Original message below:

------------------

On 10/15/22 11:06, Nicolas Grekas wrote:
>> I'm not surprised by the “no” on the first vote based on the previous
>> discussion. I am surprised however that you also disagree with raising
>> the E_NOTICE to E_WARNING for consistency.
>>
>> I don't plan on repeating all the previous discussion points with you,
>> but as you mention the Symfony tests specifically: Please find the patch
>> attached. Do you believe the expectations in this new test would a
>> reasonable expectation by a Symfony user?
>>
> 
> Since the beginning my point is not that the RFC doesn't have merits. It's
> that the proposed approach breaks BC in a way that will affect the
> community significantly. We have policies that say we should avoid BC
> breaks and they should apply here.
> 
> To anyone wondering about the reality of the BC break if you're not
> convinced there is one because the original code is broken anyway (which is
> your main argument Tim IIUC): please have a look at the failures I reported

Yes, you understood this correctly: I consider the existing 
implementation to be broken. Unfortunately you did not answer my 
question whether the attached patch would be a reasonable expectation by 
a Symfony user.

Let's presume it is: If I use the 'PhpSerializer' then I expect to 
receive exactly the 'MessageDecodingFailedException' whenever the 
message fails to decode. When decoding an erroneous DateTime (or an 
erroneous SplDoublyLinkedList or whatever) a different Exception will be 
thrown and not caught, thus the contract by the 'PhpSerializer' is violated.

Let's fix this using test driven development. We first add the failing 
test cases. This is step1.diff (patches are in 
https://gist.github.com/TimWolla/376dd162f7684daef38f76a07254871c). When 
running the tests we get the following output:


> There were 2 failures:
> 
> 1) Symfony\Component\Messenger\Tests\Transport\Serialization\PhpSerializerTest::testDecodingFailsWithBadDateTimeData
> Failed asserting that exception of type "Error" matches expected exception "Symfony\Component\Messenger\Exception\MessageDecodingFailedException". Message was: "Invalid serialization data for DateTime object" at
> /app/src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php:95
> /app/src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php:54
> /app/src/Symfony/Component/Messenger/Tests/Transport/Serialization/PhpSerializerTest.php:99
> .
> 
> 2) Symfony\Component\Messenger\Tests\Transport\Serialization\PhpSerializerTest::testDecodingFailsWithBadDoublyLinkedListData
> Failed asserting that exception of type "UnexpectedValueException" matches expected exception "Symfony\Component\Messenger\Exception\MessageDecodingFailedException". Message was: "Incomplete or ill-typed serialization data" at
> /app/src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php:95
> /app/src/Symfony/Component/Messenger/Transport/Serialization/PhpSerializer.php:54
> /app/src/Symfony/Component/Messenger/Tests/Transport/Serialization/PhpSerializerTest.php:110
> .

The tests fails (as expected) and we need to fix the code. To fix this 
we add a new 'catch(Throwable)' to the existing 'try'. Within the catch 
we throw a new 'MessageDecodingFailedException' that wraps the Throwable 
we caught using the '$previous' parameter. You can find the change in 
step2.diff. Let me re-run the tests:

> There was 1 failure:
> 
> 1) Symfony\Component\Messenger\Tests\Transport\Serialization\PhpSerializerTest::testDecodingFailsWithBadClass
> Failed asserting that exception message 'Could not decode message using PHP serialization: O:13:"ReceivedSt0mp":0:{}.' matches '/class "ReceivedSt0mp" not found/'.

Okay, now the Exception message changed. Personally I do not consider 
this a BC break: I believe Exception messages are meant for human 
consumption, not for programs. Otherwise fixing a typo in the message 
would be a BC break. If the code wants to learn about the cause, it 
should either use the '$code' or different types of Exception should be 
thrown to clarify the cause by entering a different catch() block.

There are two options here: We could fix the expectation (that's what I 
would do, see step3a.diff), or we could use the message of the original 
Exception as the message of the wrapper Exception (step3b.diff), or we 
could rethrow the original exception, if it already is of the correct 
class (step3c.diff).

In all three variants of Step 3 we now fixed the bug for DateTime and 
SplDoublyLinkedList. PhpSerializer will now consistently throw 
MessageDecodingFailedException in all cases, just the message might have 
changed (in variant 'a').

> above and wonder about the changes the RFC would impose. I cannot think
> about one that would not imply some sort of "if the version of PHP is >=
> 8.3 then A, else B". This is the fact that highlights the BC break.
> 

Now that we fixed the implementation of PhpSerializer for PHP 8.2, let's 
have a look at what changes with my RFC. Let's take step3a.diff as the 
basis. Let me first write a userland implementation of the new 
unserialize, as this makes it easier to demonstrate the behavior to 
folks without knowledge about the internals. I'll put the implementation 
into PhpSerializer.php to keep things simple.

You can find the implementation in rfc.diff, but I'll repeat it here:

> class UnserializationFailedException extends \Exception {}
> 
> function unserialize_php83(string $data, array $options = []): mixed
> {
>     try {
>         return \unserialize($data, $options);
>     } catch (\Throwable $e) {
>         throw new UnserializationFailedException(
>             'An Exception was thrown during unserialization',
>             0,
>             $e
>         );
>     }
> }

Now, let us use the custom implementation, instead of unserialize() 
directly. To do so we replace unserialize() with unserialize_php83(). 
You can find the result in rfc-use.diff. What happens if I run the tests?

> PHPUnit 9.5.25 #StandWithUkraine
> 
> Testing Symfony\Component\Messenger\Tests\Transport\Serialization\PhpSerializerTest
> .........                                                           9 / 9 (100%)
> 
> Time: 00:00.030, Memory: 8.00 MB
> 
> OK (9 tests, 14 assertions)

All green! We **did not have to change anything** after fixing the 
implementation in step3a.diff for PHP 8.2 and earlier!

Okay, let's assume the actual Exception message must not change, e.g. 
using the solution in step3b.diff and step3c.diff. Again there are 
multiple solutions:

1. The implementation of the RFC could be adjusted, such that the 
wrapper exception implicitly uses the original Exception message or 
original code. So the native implementation could do something similar 
to step3b. I would be willing to do this, if you believe that would 
reduce the BC impact.

2. Alternatively a little change to the code is required, let me 
demonstrate how that would look. The solution is based on 'step3c.diff':

We just need to add these three lines to the catch block:

>             if ($e instanceof UnserializationFailedException && $e->getPrevious()) {
>                 $e = $e->getPrevious();
>             }

This will unwrap the original Exception that is stored in the 
'$previous' parameter. You can find the full patch in 
'rfc-use-alternative.diff'.

Of course this is fully compatible with PHP 8.2 and earlier: The 
instanceof will simply not match, because 
'UnserializationFailedException' does not exist.

-------------------

I believe with this example I have sufficiently demonstrated how fixing 
the code for *the existing PHP versions* will cause the test failures to 
go away automatically. If you consider the Exception message to be 
relevant for backwards compatibility, then just three simple lines need 
to be added - or alternatively the RFC implementation which currently 
just is a proof of concept (!) could be adjusted. At no point is it 
necessary to check for the PHP version.

I believe the changes to the other classes with failing tests are 
equally simple based on my demonstration.

Feel free to use my attached patches to make the necessary changes in 
Symfony. There is no need to credit me in any way. Alternatively I would 
be happy to send a pull request. Just let me know which of the 3 
alternatives of step 3 you prefer and I'll send a PR.

Best regards
Tim Düsterhus