Hi

Either I guess? Honestly we should decide that in advance on the list. :-) E_WARNING+Exception in 9 is what I'd probably favor, with "Exception now" as a second choice.

I'm a new-ish contributor here in internals, so I don't know how things
were done in the past for similar situations/issues.

I'm not sure, though if it makes sense to already decide on something
for PHP 9. If it's not baked into code shortly after the vote finishes,
then people might forget that "there's something that still needs to be
done". For a deprecation one can at least go through all the
deprecations once PHP 9 opens, as a deprecation effectively is defined
to be a removal in the next major. For a warning this is less obvious.

Personally my first choice would be "Straight to Exception", so I might
not be the best person to decide on that :-)

Best regards
Tim Düsterhus

We've done this kind of two-step thing before; a lot of PHP 8 changes were voted on well in advance of 8.0's release. A "Warning now, Exception in 9" vote would not be unprecedented.

--Larry Garfield

Hi

Either I guess? Honestly we should decide that in advance on the list.
:-) E_WARNING+Exception in 9 is what I'd probably favor, with "Exception
now" as a second choice.

I'm a new-ish contributor here in internals, so I don't know how things
were done in the past for similar situations/issues.

Welcome, Tim :-)

Do give it more thought regarding the callbacks what Nikolas said, sleep on
it.

Not saying it matters, but also double check how error handling MAY differ
when in a Class::_unserialize() context. Maybe it doesn't diff, I haven't
checked in a while, but food for thought.

In any case, good cleanup initiative.

I'm not sure, though if it makes sense to already decide on something
for PHP 9. If it's not baked into code shortly after the vote finishes,
then people might forget that "there's something that still needs to be
done". For a deprecation one can at least go through all the
deprecations once PHP 9 opens, as a deprecation effectively is defined
to be a removal in the next major. For a warning this is less obvious.

Personally my first choice would be "Straight to Exception", so I might
not be the best person to decide on that :-)

Best regards
Tim Düsterhus

--

To unsubscribe, visit: https://www.php.net/unsub.php

Hi Nicolas

[long response from a week ago]

Did you find the time to read through my response from September, 13th, yet?

Best regards
Tim Düsterhus

Hi Tim,

I'm a bit busy with conferences these days...

On 9/12/22 21:46, Nicolas Grekas wrote:>> unserialize() is a generic

function that will also call arbitrary

callbacks. You already have no guarantees whatsoever about the kind of
exception that is thrown from it. I am unable to think of a situation
where the input data is well-defined enough to reliably throw a specific
type of exception, but not well-defined enough to successfully
unserialize.

So on the contrary wrapping any Exceptions with a well-defined Exception
allows you to rely on a specific and stable Exception to catch in the
first place.

As you specifically mention catch(Throwable), I'd like to point out that
this will continue to work, because UnserializationFailedException
implements Throwable.

You might have misunderstood my point so let me give two examples:

Example 1.
set_error_handler(fn ($t, $m) => throw new ErrorException($m));
try {
$ser = serialize($value);
catch (ErrorException $e) {
// deal with $e
finally {
restore_error_handler();
}

Example 2.
try {
$ser = @serialize($value);
catch (Exception $e) {
// deal with $e but don't catch Error on purpose, to let them bubble
down
}

Changing serialize to wrap any throwables into an
UnserializationFailedException would break both examples. That's what I
mean when I say this breaks BC. As any BC-break, this might cause big
troubles for the community and this should be avoided. The release
process
policy I mentioned above is a wise document.

First I'd like to note that these examples use 'serialize()' (which is
not touched by my RFC, because serialize rarely fails). I consider this
a typo and will answer as if you used 'unserialize()'. If that was not a
typo, then this might explain the confusion.

It was a typo! I meant unserialize() yes.

Are those two examples based on real-world use cases or did you craft
them specifically to point out how the proposal would introduce a
behavioral change?

They were inspired by what I found in the Symfony codebase.

I just conducted a quick lookup at this code base: most call to
unserialize() are not checked at all, but I hghlighted the checked ones in
this gist:
https://gist.github.com/nicolas-grekas/5dd3169f94ed3b4576152605330824fe

I'm asking, because the examples fail to explain the "why". Why is the
code written like this? I fail to see how catching "Exception", but not
catching "Error" during unserialization is ever going to be useful.

It's useful because these two roots mean very different things.

That's why Error has been introduced in the first place: instances of Error
are not just exceptions, they're really programming mistakes. As such the
best practice to me is to not catch them in domain layers, and let them
bubble down to a generic Error catch layer instead.

Likewise I don't see how catching ErrorException specifically is useful
when '__unserialize()' might throw arbitrary Throwables.

It's useful when all you need is eg to care about the exceptions thrown by
a custom error handler, and want to ignore the other ones (because other
exceptions aren't part of the current domain layer.)

Specifically

see the list in this email: https://externals.io/message/118311. Code
outside of the function that actually calls unserialize() is not going
to be prepared to usefully handle unserialization failure. This
effectively means that the Exception will bubble up to the global
Exception handler (or the Exception handler middleware), resulting in an
aborted request.

Yep, this is fine in many cases.

Quoting myself, because the examples didn't answer the question: "I am
unable to think of a situation where the input data is well-defined
enough to reliably throw a specific type of exception, but not
well-defined enough to successfully unserialize".

And don't get me wrong: I'm not attempting to say that it is appropriate
to break logic that I personally consider wrong without justification. I
believe the benefits of having the unified Exception outweigh the
drawbacks of introducing a behavioral change in code that is already
subtly broken depending on the exact input value passed to unserialize().

The thoughts you describe might make sense as a rule of thumb principle
when you write your code. It might even be upgraded to a best practice.
Generally turning an Error to an Exception is not by my book, but I'm not
arguing about this. My point is : /!\ BC break ahead, do not cross /!\

If you look at the gist I linked before, wrapping all throwables would
break most if not all the cases I listed. That's bad numbers, and I don't
think this is specific in any way to the Symfony codebase.

This is similar to the attribute syntax breaking hash comments that
start with a square bracket without any space in the middle. Or the '@@'
attribute syntax that was also proposed. It would have broken code
containing duplicated error suppression operators.

Similarly to the attribute breakage, it's also reasonably easy to find
possibly affected logic by grepping for 'unserialize('.

Sorry to disagree, that's a very different sort of break. One that changes
the behavior of perfectly fine apps.

About 2., unserialize() accepts a second argument to give it options.
Did

you consider adding a 'throw_on_error' option to opt-in into the
behavior
you're looking for? I think that would be a nice replacement for the
current logics based on set_error_handler(). If we want to make PHP 9
throw
by default, we could then decide to deprecate not passing this
option.

I did not consider this when writing the RFC. I now considered it, and I
do not believe adding a flag to control this a good thing.

1. "No one" is going to set that flag, because it requires additional
   effort. I strongly believe that the easiest solution should also the
   correct solution for the majority of use cases. The flag fails this
   "test", because the correct solution should be "don't fail silently".

Unless we deprecate calling the method without the argument as I
suggested.
I agree, it might not be ideal to ask the community to rewrite every call
to serialize($v) to serialize($v, ['throw_on_error' => true/false]) but
that's still way better than a BC break.
Maybe there's another way that doesn't break BC. I'm looking forward to
one.

See (4) and above regarding my opinion on the BC break.

2. If you actually want to set that option in e.g. a library, then you

break compatibility with older PHP versions for no real gain. If you go
all the way and remember to add that extra flag, then you can also write
an unserialize wrapper that does the set_error_handler dance I've shown
in the introduction of the RFC. Similar effort to adding the flag, but
better compatibility.

I think my proposal provides BC with older versions of php: passing extra
(unused) options is allowed and polyfilling
UnserializationFailedException
is trivial, so one could set_error_handler(fn (...) => throw new
UnserializationFailedException(...)) and be both BC and FC.

About writing a wrapper, that's what we all do right now, each with our
own
wrapping logic. You're looking for unifying those logics and I support
that
goal.

I come from a userland developer background, but without the well-known
frameworks in the back. I must admit that I am not currently using a
wrapper around unserialize(). Partly because the application I work on
uses a global throwing error handler (see (3)), partly because the
non-E_SOMETHING error cases were not obvious to me before researching
this RFC which came to life because of my work on PHP 8.2's ext/random.

So to clarify: What I'm looking for is making unserialize() safe and
well-defined to use, so that wrapping logic is no longer required.

But this leads to another idea : what about adding a new function, let's
name it unserialize2() for now, and deprecate unserialize()? That'd
provide
the upgrade path we need.

This sounds like another instance of mysql_real_escape_string() to me.
Also the default choice and the one documented all around the Internet
is still going to be 'unserialize()', because it's the more obvious one.

3. It does literally nothing for users that use a throwing error
   handler, which to my understanding includes the vast majority of
   framework code.

I'm not sure what you mean here. The flag would allow removing those
error
handlers, which is what you want to achieve in the end I believe (well,
except when BC with older versions is desired of course, see my previous
comment.)

I likely wasn't clear here: I am talking about the global throwing error
handler that is added by frameworks during the bootstrapping of the
request.

I've tested with Symfony (controller attached): When I just call
unserialize('asdf') within a controller, then an ErrorException comes
out of it, because of the global error handler. If I don't catch that
one then Symfony's Exception handler will show me a pretty error page
with a ghost.

The same is true for Laravel the last time I tested and I suspect this
also is true for other frameworks.

For all users that use such a global error handler setting the option or
not setting the option will result in 100% identical behavior.

4. Even for users that do not use a throwing error handler, omitting the
   option literally does nothing, because unserialize() might already throw
   depending on what the unserialize handlers of unserialized objects do.

Sure. I don't see how that's a problem. Throwing is what almost any lines
of PHP code can do. We're used to dealing with that and I don't see why
serialize() needs special care. I might have missed the point you want to
make here though.

The difference is that for the majority of functions the possible
failure cases are known and can be ruled out statically. As an example I
can be sure that count() will not throw if I pass it an array and I
don't change the $mode. This is not true for unserialize() which
internally might call arbitrary __unserialize() callbacks.

True, that's expected to me given what unserialize does. I don't see the
need to wrap sorry (even ignoring the BC break.) Turning Error into
Exception would be especially bad to me, eg a ParseError should remain a
ParseError so that it can follow the usual path for reporting parse errors.

Lastly I'd like to add a 3. to your proposal, because there is one more
thing that makes unserialization annoying: the
unserialize_callback_func
f the results are copies of php-src that match the .phpt tests.

PHP creates a __PHP_Incomplete_Class object, but I regularly use this
callback to throw instead when I don't want those strange incomplete
objects (note that I also have seen use cases where generating a
__PHP_Incomplete_Class is the desired behavior.)

I see. So you are not so much interested in the callback specifically,
but a "throw_for_unknown_classes" flag would also solve your use case.
Personally I think such having such a flag would be clearer with regard
to the intent.

I checked at the code and we use varying domain exceptions + messages so a
flag wouldn't be enough.
There's also a case where we call trigger_error() to connect missing
classes to the regular error handler.

I proposed that because in many situations unserializing a payload to

something that contains __PHP_Incomplete_Class objects is not desired and
should be leveled up to an error instead. That's the relation with error
handling and thus with the RFC.

That said, I believe adding a flag to generate additional errors (be
it via a callback or via my proposal) is orthogonal to handling errors
and thus is out of scope for this RFC. Your suggestion likely has
plenty of bikeshedding potential on its own.

I think that being able to cleanly make unknown classes an error would
be a useful addition, though. I recommend that you create a separate RFC
for that once mine went through the vote, so that your's can build on
the results of mine.

Thanks, I hoped I could slip in this idea but I'll keep it on my todo list
for php-src :)

Nicolas