What type of Exception to use for unserialize() failure? View as Markdown

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