Re: [VOTE] Improve unserialize() error handling

Larry, > On Oct 17, 2022, at 6:01 PM, Larry Garfield <larry@garfieldtech.com> wrote: > > On Mon, Oct 17, 2022, at 12:33 PM, Tim Düsterhus 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. >>>> >>> >>> 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. >> >> I'm not talking about test suites here. I believe makes sense to verify >> the error message to ensure a specific error message is emitted to the >> human observer in the error log. >> >> I was talking about code that does something like this, which I consider >> to be inherently unsafe: >> >> try { … } >> catch (SomeException $e) { >> if ($e->getMessage() === 'Foobar') doSomething(); >> else doSomethingElse(); >> } >> >> As a library author I want to be able to provide the best possible >> Exception message to ease debugging for the user. This is not possible >> if I am locked into a bad choice forever. > > Just to be clear, such code is sometimes necessary. If the exception doesn't include > sufficient information as dedicated properties, parsing out 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 the 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 recently needed to sscanf() and then explode the message of > \ArgumentCountError as that was the only way I could find to get the class/method names 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 message strings as part of > the API guarantee. If anything, maybe that will help incentivize people to stop writing bad > (unparsable) exceptions. I am curious what you would envision a better, "parsable" exception from sscanf() would look like? I ask not because I disagree with you here but because maybe the question of whether errors messages should be part of BC is focusing on the wrong question? -Mike P.S. Also, looking at https://www.php.net/manual/en/function.sscanf.php <https://www.php.net/manual/en/function.sscanf.php>, it is interesting that the exceptions it triggers are not documented here. (I am not picking on sscanf() — AFAIK few if any functions document their exceptions.) Shouldn't more thought (and documentation) be given to this information which obviously is part of its API/usage?