On Fri, 29 Jul 2022 at 18:17, Tim Düsterhus <t...@bastelstu.be> wrote:
> Hi! > > I'm currently in the process of cleaning up the error handling of the > new ext/random and now came across the implementation of __unserialize(). > > Looking through the source code for existing examples I found that the > following is used: > > - ext/gmp: Exception > - ext/hash: Exception > - ext/date: Error > - ext/spl: UnexpectedValueException > - ext/random: Currently Exception. > > … all of those with different error messages. unserialize() itself will > emit a regular 'Notice' (yes, you read that right) when encountering > garbage data. > > In the end I've opted to follow ext/date's lead and created a PR > changing ext/random to use an Error, as unserialization failures likely > mean that you are unserializing untrusted data which you should not do > in the first place. Also any errors during unserializing are not really > recoverable: You can't go and fix up the input string. > > The PR can be found here: > > https://github.com/php/php-src/pull/9185 > > During review cmb noted that using an 'Error' here might not be the best > choice, as while it is likely to be a programmer error if unserializing > fails, we do not want to educate users to catch(Error). > I probably have a very different philosophy on this as I don't mind users catching Errors, but it probably be mostly done by Framework/libraries doing some funky stuff. And passing user input which can fail to unserialize is not really a programming error and something that can be "expected" to happen, I agree that an Exception does make more sense here. > As the existing behavior already is inconsistent and the Notice really > should be upgraded to something … more alarming in the future, I'm > putting this topic up for discussion on the list. > > My suggested path forward would be: > > For 8.2: > > - Update ext/random to use the ext/date wording (I like that one best): > > "Invalid serialization data for <classname> object." > > - Revert the change from Exception to Error in my ext/random PR #9185. > Those make sense to me > - Update ext/date to Exception (the Error for __unserialize() was added > in 8.2, but there already was an Error for __wakeup() before). > I'm not a super fan of this, both __wakeup and __unserialize should use the same one. As far as I can tell the reason __wakeup() is still defined is if someone called it explicitly. But in any case if someone was handing an unserialisation failure (that was handled by __wakeup prior to 8.2) and caugh an Error explicitly this would now break. Thus I'd prefer handling this with the rest after 8.2 > For whatever comes after 8.2: > > - Add a new UnserializationFailureException extends Exception (or > whatever color the bikeshed should have). Any existing catch blocks > should still match, as Exception is more general. It would break for > ext/spl, though. Unless UnserializationFailureException extends > UnexpectedValueException. > As we would be breaking BC with ext/date anyway I think having it just extend Exception would be fine > - Add a helper function that throws this Exception with a consistent > wording. > - Upgrade unserialize() from Notice to UnserializationFailureException. > Notice to Exception might be a big jump, E_WARNING definitely an then promote to an Exception in 9.0 is probably more in line with what we did for 7.x/8.0 George P. Banyard
