Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118818
MIME-Version: 1.0
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>
In-Reply-To: <CAOWwgpnpBTx9-aLz-bg1xZ9FT5JyWsX44TLwfvanDS_aAW7qdQ@mail.gmail.com>
Date: Sat, 15 Oct 2022 12:49:21 +0200
Message-ID: <CADyq6sJOBhTpRBp=ueJXhLqQXxEP26H9EaEucJ-ZVJ_Sa7MPfA@mail.gmail.com>
To: Nicolas Grekas <nicolas.grekas+php@gmail.com>
Cc: =?UTF-8?Q?Tim_D=C3=BCsterhus?= <tim@bastelstu.be>, 
	PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary="00000000000072e9e005eb107f35"
Subject: Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
From: ocramius@gmail.com (Marco Pivetta)

--00000000000072e9e005eb107f35
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Tbh, this affects a new minor and a new major, and only in an unhappy path
scenario, where the current PHP API is bad.

PHP 9 will introduce a hard crash: good.
It's a BC break: yes, native de-serialization is one of the most unsafe
parts of the language, and it requires hardening.

The fact that Symfony declared that it already supports PHP 8.3 and PHP 9.0
is your problem, and mostly your misunderstanding of SemVer

On Sat, 15 Oct 2022, 11:06 Nicolas Grekas, <nicolas.grekas+php@gmail.com>
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 th=
at
> > > doesn't change the fact that the proposed behavior in the RFC is a ve=
ry
> > > clear BC break that will affect userland significantly.
> > >
> > > I'm therefore voting NO on the proposal.
> > >
> >
> > I'm not surprised by the =E2=80=9Cno=E2=80=9D on the first vote based o=
n 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 patc=
h
> > 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 report=
ed
> 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 >=
=3D
> 8.3 then A, else B". This is the fact that highlights the BC break.
>
> Nicolas
>

--00000000000072e9e005eb107f35--