Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118834
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>
 <c69c45f7-9836-1858-d90e-df7042bd3f88@bastelstu.be> <CAOWwgpmZ82zp7apmrwmb+xKOrwJvMuOJ=OMBuZa3n44ZPX5Uwg@mail.gmail.com>
In-Reply-To: <CAOWwgpmZ82zp7apmrwmb+xKOrwJvMuOJ=OMBuZa3n44ZPX5Uwg@mail.gmail.com>
Date: Mon, 17 Oct 2022 12:46:24 -0700
Message-ID: <CAMrTa2H=W0w_9NcE130E9B3A+LHahdetsf1BL50vJ6er5h-PEg@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="000000000000ad522605eb403b5e"
Subject: Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
From: jordan.ledoux@gmail.com (Jordan LeDoux)

--000000000000ad522605eb403b5e
Content-Type: text/plain; charset="UTF-8"

Sorry for double send Nicolas, I hit reply instead of reply-all on my first
message. :)

On Mon, Oct 17, 2022 at 1:57 AM Nicolas Grekas <nicolas.grekas+php@gmail.com>
wrote:

>
> Yes, the specific error message should be part of the BC promise. This
> allows building test suites that can assert the message in a stable way.
> This is also why we don't change the output of var_dump/print_r/var_export:
> they're written now, in the same of BC, for the best of stability. (I've
> barely read any PHP code where exception's code is used in any useful BTW -
> that can't be a solution.)
>

While I *definitely* sympathize with the "BC break" for tests, this doesn't
actually break *code* unless you are switching behaviors in catch based on
specific exception messages, which does not seem like a workflow that PHP
needs to guarantee, as that is not the purpose of exception messages.

Moreover, the actual messages change in php-src all the time in PR's,
sometimes in PR's not even attached to RFCs. This has not, to my knowledge,
ever been previously considered a BC promise, and it certainly hasn't ever
been treated that way. If php-src took the position you are saying here,
then any error message in PHP would need to remain constant until major
versions, and we also probably could change between NOTICE, WARNING, or
anything like that, as it would present similar issues. This would make it
a BC break to provide deprecations ahead of the major versions, unless we
did an *entire* version ahead, which I do not think is worth the benefit of
providing that level of BC guarantee personally.

As I said, I definitely sympathize with your tests example, because I have
had libraries I work on with similar tests that break and/or are fragile
due to the message changing. But the situations in which I was doing this
in tests I realized were all because I was throwing the *same* exception
for *different* problems and trying to ensure which path caused the throw
within the test. In that case, I refactored the code to provide subclasses
of that exception instead so they could be differentiated, which was the
much more maintainable way to handle it.

Basically, this change may break the Symfony tests here, and could
definitely break other tests as well, but the tests it breaks are incorrect
tests in my opinion, and don't actually guarantee the correctness that the
green result implies. It is unlikely to break *code* (not tests) in ways
that it wasn't already broken before. (Tim has explained this, RE:
unserialization of various possible inputs). I still have yet to see an
example (even a contrived one) in which this RFC would *introduce a failing
path to code that wasn't there before* instead of *promote a hidden
existing failing path into something that the developer can now respond to
intelligently*.

As far as I can tell from the examples provided so far, this RFC reduces
the failing paths of 8.2 -> 8.3 code by promoting and exposing those paths
to existing tests in a way that actually matches the documentation for
unserialize. I might be still misunderstanding some nuance here.

Jordan

--000000000000ad522605eb403b5e--