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
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
