Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:118835
Feedback-ID: i8414410d:Fastmail
User-Agent: Cyrus-JMAP/3.7.0-alpha0-1047-g9e4af4ada4-fm-20221005.001-g9e4af4ad
Mime-Version: 1.0
Message-ID: <13af26f4-2925-44a7-a5b3-6e282ebc77a0@app.fastmail.com>
In-Reply-To: <0d950416-372b-1ff5-21c2-0c6720d47a07@bastelstu.be>
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>
 <0d950416-372b-1ff5-21c2-0c6720d47a07@bastelstu.be>
Date: Mon, 17 Oct 2022 17:01:32 -0500
To: "php internals" <internals@lists.php.net>
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] [VOTE] Improve unserialize() error handling
From: larry@garfieldtech.com ("Larry Garfield")

On Mon, Oct 17, 2022, at 12:33 PM, Tim D=C3=BCsterhus wrote:

>>> 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.
>>>
>>=20
>> 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 w=
ay.
>
> I'm not talking about test suites here. I believe makes sense to verif=
y=20
> the error message to ensure a specific error message is emitted to the=20
> human observer in the error log.
>
> I was talking about code that does something like this, which I consid=
er=20
> to be inherently unsafe:
>
> try { =E2=80=A6 }
> catch (SomeException $e) {
>    if ($e->getMessage() =3D=3D=3D 'Foobar') doSomething();
>    else doSomethingElse();
> }
>
> As a library author I want to be able to provide the best possible=20
> Exception message to ease debugging for the user. This is not possible=20
> if I am locked into a bad choice forever.

Just to be clear, such code is sometimes necessary.  If the exception do=
esn't include sufficient information as dedicated properties, parsing ou=
t the string becomes the only option.  I've had to do this myself.

In 100% of cases, without exception (no pun intended), that's because th=
e code that throws the exception is bad and wrong and should be fixed.  =
But such code absolutely exists in the wild, including in php-src.  I re=
cently needed to sscanf() and then explode the message of \ArgumentCount=
Error as that was the only way I could find to get the class/method name=
s out of it.  I died inside a little.

So yes, such code is inherently unsafe, but is sadly not as uncommon as =
it should be.

All that said, I agree that we have not and should not treat error messa=
ge strings as part of the API guarantee.  If anything, maybe that will h=
elp incentivize people to stop writing bad (unparsable) exceptions.

Which I think gets at the crux of the issue here, and what is often the =
issue in BC discussions.

PHP does not, and has never, guaranteed full BC for all code.  It has tr=
ied very hard to maintain BC for "well-behaved code."  Well-behaved code=
 can generally update to a new version, even a new major, very easily wi=
th minimal work.  Any deprecations are alerted at least a year in advanc=
e, sometimes several, and usually have easy-to-automate transitions that=
 tools like Rector can implement.  (The discussion about deprecations be=
ing "too noisy" in some tooling is another matter that has been hashed o=
ut before, so I won't belabor it here.)  Code that is following common b=
est-practices rarely has an issue.

The problem is that "well-behaved" is not well-defined.  Is relying on a=
n error message string well-behaved?  I'd argue no, but as noted above s=
ometimes there's no better option.  Others may disagree.  Is relying on =
the exact exception class thrown well-behaved?  Well... sometimes.  Is r=
elying on an error condition being swallowed rather than turning into an=
 exception well-behaved?  I could argue either way here, honestly.  Is c=
ode relying on undefined variables silently becoming null well-behaved? =
 Absolutely not, and hasn't been for a decade, but it wasn't until PHP 8=
.0 that the language called people out for it by default, so many projec=
ts had a lot of catch up to do.  (Eg, I did that catch up for TYPO3.)

I don't know that we can make a clear definition of "well-behaved" that =
everyone could agree on.  It would be nice, but it's a very squishy topi=
c.  But it may be prudent, when not tied down to this specific RFC, to h=
ave a broader discussion about better codifying (meaning, writing down) =
what we consider a BC break and what we don't, what some heuristics are =
for "well-behaved," and so on.  That could help avoid, or at least short=
en, a lot of discussions in the future.

--Larry Garfield