Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118815
Content-Type: multipart/mixed; boundary="------------1iZAER0fgd9dDHI1rRU0awpD"
Message-ID: <c9e61189-7826-844f-56b9-7bb509a0791d@bastelstu.be>
Date: Sat, 15 Oct 2022 00:00:58 +0200
MIME-Version: 1.0
Content-Language: en-US
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>
In-Reply-To: <CAOWwgpmuiRzUwGpU3ShQ7HOAVGhcP=Wb8z50acLfERMn_NEbwQ@mail.gmail.com>
Subject: Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
From: tim@bastelstu.be (=?UTF-8?Q?Tim_D=c3=bcsterhus?=)

--------------1iZAER0fgd9dDHI1rRU0awpD
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

Hi

On 10/14/22 22:42, Nicolas Grekas wrote:
> Not sure why I didn't think about it before but I just ran the test suite
> of Symfony after applying the patch proposed in the RFC to change the way
> exceptions are handled by unserialize.
> 
> This change breaks the test suite of 5 separate components. I created this
> gist to list all the failures:
> https://gist.github.com/nicolas-grekas/3da652a51669baa40c99bd20e4a1b4dd
> 
> Maybe I wasn't convincing enough during the discussion period, but that
> doesn't change the fact that the proposed behavior in the RFC is a very
> clear BC break that will affect userland significantly.
> 
> I'm therefore voting NO on the proposal.
> 

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?

Best regards
Tim Düsterhus
--------------1iZAER0fgd9dDHI1rRU0awpD
Content-Type: text/x-patch; charset=UTF-8; name="PhpSerializerTest.diff"
Content-Disposition: attachment; filename="PhpSerializerTest.diff"
Content-Transfer-Encoding: base64

ZGlmZiAtLWdpdCBpL3NyYy9TeW1mb255L0NvbXBvbmVudC9NZXNzZW5nZXIvVGVzdHMvVHJh
bnNwb3J0L1NlcmlhbGl6YXRpb24vUGhwU2VyaWFsaXplclRlc3QucGhwIHcvc3JjL1N5bWZv
bnkvQ29tcG9uZW50L01lc3Nlbmdlci9UZXN0cy9UcmFuc3BvcnQvU2VyaWFsaXphdGlvbi9Q
aHBTZXJpYWxpemVyVGVzdC5waHAKaW5kZXggYzgzNjA2YTU5Zi4uNjA5MTg3OTk5OSAxMDA2
NDQKLS0tIGkvc3JjL1N5bWZvbnkvQ29tcG9uZW50L01lc3Nlbmdlci9UZXN0cy9UcmFuc3Bv
cnQvU2VyaWFsaXphdGlvbi9QaHBTZXJpYWxpemVyVGVzdC5waHAKKysrIHcvc3JjL1N5bWZv
bnkvQ29tcG9uZW50L01lc3Nlbmdlci9UZXN0cy9UcmFuc3BvcnQvU2VyaWFsaXphdGlvbi9Q
aHBTZXJpYWxpemVyVGVzdC5waHAKQEAgLTc3LDYgKzc3LDE3IEBAIGNsYXNzIFBocFNlcmlh
bGl6ZXJUZXN0IGV4dGVuZHMgVGVzdENhc2UKICAgICAgICAgXSk7CiAgICAgfQogCisgICAg
cHVibGljIGZ1bmN0aW9uIHRlc3REZWNvZGluZ0ZhaWxzV2l0aEJhZERhdGEoKQorICAgIHsK
KyAgICAgICAgJHRoaXMtPmV4cGVjdEV4Y2VwdGlvbihNZXNzYWdlRGVjb2RpbmdGYWlsZWRF
eGNlcHRpb246OmNsYXNzKTsKKworICAgICAgICAkc2VyaWFsaXplciA9ICR0aGlzLT5jcmVh
dGVQaHBTZXJpYWxpemVyKCk7CisKKyAgICAgICAgJHNlcmlhbGl6ZXItPmRlY29kZShbCisg
ICAgICAgICAgICAnYm9keScgPT4gJ086ODoiRGF0ZVRpbWUiOjA6e30nLAorICAgICAgICBd
KTsKKyAgICB9CisKICAgICBwdWJsaWMgZnVuY3Rpb24gdGVzdEVuY29kZWRTa2lwc05vbkVu
Y29kZWFibGVTdGFtcHMoKQogICAgIHsKICAgICAgICAgJHNlcmlhbGl6ZXIgPSAkdGhpcy0+
Y3JlYXRlUGhwU2VyaWFsaXplcigpOwo=

--------------1iZAER0fgd9dDHI1rRU0awpD--