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).
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.
- Update ext/date to Exception (the Error for __unserialize() was added
in 8.2, but there already was an Error for __wakeup() before).
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.
- Add a helper function that throws this Exception with a consistent
wording.
- Upgrade unserialize() from Notice to UnserializationFailureException.
Opinions?
Best regards
Tim Düsterhus
--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: https://www.php.net/unsub.php
