# Throwable and Error exceptions break existing PHP 5.x code

- Thread: https://externals.io/message/90485
- Original archive: https://news-web.php.net/php.internals/90485
- Started by Giovanni Giacobbi on 2016-01-11T13:31:18+00:00
- Messages: 9
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #90485 — Giovanni Giacobbi — 2016-01-11T13:31:18+00:00

URL: https://externals.io/message/90485#90485

Greetings,

Short premise before I get flamed: I know PHP 7 is rolling and it is way
too late for this, that I should've tested the RCs, follow the mailing list
and so on, but I'm a dev like you guys and struggle with the time to do
everything by the book, including reading the previous threads because I'm
pretty sure this was brought up by somebody, but as there are hundreds of
message under the "Throwable" topic and I can't read all of them.

In short I have this situation:

<?php
class SpecificHandler {
  public function exception(Exception $e) {
    .. do something very useful...
  }
}

function global_handler($e) {
  SpecificHandler::exception($e);
}

set_exception_handler("global_handler");
?>

This happens because the specific handler will output the exception
according to the expected format (HTML, JSON, or even an image).

Now after upgrading to PHP7 when one of the new Error exception is thrown,
I get the following error:

PHP Fatal error:  Uncaught TypeError: Argument 1 passed to
SpecificHandler::exception() must be an instance of Exception, instance of
Error given, called in [...]

Having been a PHP dev for 10 years now I know that the policy is to try to
break as little as possible while implementing new features, and I like
that. I know that sometimes things must be broken to progress, but in this
case maybe there is a very simple fix that I can suggest to avoid breaking
existing code, change set_handler_exception like this:

set_exception_handler(callback function, bool also_throwables = false);

The new parameter "also_throwables" defaults to false, so the same
behaviour as before is preserved. If you want it to catch also the new PHP7
Error exceptions, you can just set it to true.

What is your take on this? I know I can easily fix my code to work on both
PHP 5.x and PHP 7.x, but I really disliked this kind of BC.

Kind regards
Giovanni

---

## #90490 — Markus Fischer — 2016-01-11T14:00:06+00:00

URL: https://externals.io/message/90485#90490 · In reply to: #90485

Dear Giovanni,

I brought this up last July, see https://bugs.php.net/bug.php?id=70050 ;
because in a few code bases I tested back then, this was the first issue
I hit.

Unfortunately, it was only deemed a "Documentation Problem" back then.
AFAIK this is the most up to date information. Sorry I don't have
further news.

cheers,
- Markus


> Greetings,
> 
> Short premise before I get flamed: I know PHP 7 is rolling and it is way
> too late for this, that I should've tested the RCs, follow the mailing list
> and so on, but I'm a dev like you guys and struggle with the time to do
> everything by the book, including reading the previous threads because I'm
> pretty sure this was brought up by somebody, but as there are hundreds of
> message under the "Throwable" topic and I can't read all of them.
> 
> In short I have this situation:
> 
> <?php
> class SpecificHandler {
>   public function exception(Exception $e) {
>     .. do something very useful...
>   }
> }
> 
> function global_handler($e) {
>   SpecificHandler::exception($e);
> }
> 
> set_exception_handler("global_handler");
> ?>
> 
> This happens because the specific handler will output the exception
> according to the expected format (HTML, JSON, or even an image).
> 
> Now after upgrading to PHP7 when one of the new Error exception is thrown,
> I get the following error:
> 
> PHP Fatal error:  Uncaught TypeError: Argument 1 passed to
> SpecificHandler::exception() must be an instance of Exception, instance of
> Error given, called in [...]
> 
> Having been a PHP dev for 10 years now I know that the policy is to try to
> break as little as possible while implementing new features, and I like
> that. I know that sometimes things must be broken to progress, but in this
> case maybe there is a very simple fix that I can suggest to avoid breaking
> existing code, change set_handler_exception like this:
> 
> set_exception_handler(callback function, bool also_throwables = false);
> 
> The new parameter "also_throwables" defaults to false, so the same
> behaviour as before is preserved. If you want it to catch also the new PHP7
> Error exceptions, you can just set it to true.
> 
> What is your take on this? I know I can easily fix my code to work on both
> PHP 5.x and PHP 7.x, but I really disliked this kind of BC.
> 
> Kind regards
> Giovanni

---

## #90492 — Rowan Collins — 2016-01-11T14:05:47+00:00

URL: https://externals.io/message/90485#90492 · In reply to: #90485

Giovanni Giacobbi wrote on 11/01/2016 13:31:
> set_exception_handler(callback function, bool also_throwables = false);
>
> The new parameter "also_throwables" defaults to false, so the same
> behaviour as before is preserved. If you want it to catch also the new PHP7
> Error exceptions, you can just set it to true.

This is an interesting idea, but other than the specific transition to 
PHP 7, I'm not sure when it would be used.

Put it this way: if Errors existed when you first wrote this code, would 
you want to handle them using your custom handler functions, or would 
you prefer them to the fall through to the default server White Screen 
of Doom? My guess is that you'd want to pass them to the handler, even 
if it immediately checked "if ($e instanceOf Error)" and used a 
different output style.

Since `set_exception_handler()` is intended as a last-ditch "something's 
gone very wrong" function anyway, I think it receiving all Throwables 
makes sense, even if the BC break in your scenario is unfortunate.

Regards,
-- 
Rowan Collins
[IMSoP]

---

## #90516 — bjorn.x.larsson@telia.com — 2016-01-11T19:47:39+00:00

URL: https://externals.io/message/90485#90516 · In reply to: #90485

Den 2016-01-11 kl. 14:31, skrev Giovanni Giacobbi:
> Greetings,
>
> Short premise before I get flamed: I know PHP 7 is rolling and it is way
> too late for this, that I should've tested the RCs, follow the mailing list
> and so on, but I'm a dev like you guys and struggle with the time to do
> everything by the book, including reading the previous threads because I'm
> pretty sure this was brought up by somebody, but as there are hundreds of
> message under the "Throwable" topic and I can't read all of them.
>
> In short I have this situation:
>
> <?php
> class SpecificHandler {
>    public function exception(Exception $e) {
>      .. do something very useful...
>    }
> }
>
> function global_handler($e) {
>    SpecificHandler::exception($e);
> }
>
> set_exception_handler("global_handler");
> ?>
>
> This happens because the specific handler will output the exception
> according to the expected format (HTML, JSON, or even an image).
>
> Now after upgrading to PHP7 when one of the new Error exception is thrown,
> I get the following error:
>
> PHP Fatal error:  Uncaught TypeError: Argument 1 passed to
> SpecificHandler::exception() must be an instance of Exception, instance of
> Error given, called in [...]
>
> Having been a PHP dev for 10 years now I know that the policy is to try to
> break as little as possible while implementing new features, and I like
> that. I know that sometimes things must be broken to progress, but in this
> case maybe there is a very simple fix that I can suggest to avoid breaking
> existing code, change set_handler_exception like this:
>
> set_exception_handler(callback function, bool also_throwables = false);
>
> The new parameter "also_throwables" defaults to false, so the same
> behaviour as before is preserved. If you want it to catch also the new PHP7
> Error exceptions, you can just set it to true.
>
> What is your take on this? I know I can easily fix my code to work on both
> PHP 5.x and PHP 7.x, but I really disliked this kind of BC.
>
> Kind regards
> Giovanni
Interesting idea, reminds me of the discussion in august, see:
- http://marc.info/?l=php-internals&m=143980755928040

Then it was about if a set_throwable_handler should be introduced.

Regards //Björn Larsson

---

## #90518 — Adam Harvey — 2016-01-11T20:15:55+00:00

URL: https://externals.io/message/90485#90518 · In reply to: #90492

> Since `set_exception_handler()` is intended as a last-ditch "something's gone
> very wrong" function anyway, I think it receiving all Throwables makes
> sense, even if the BC break in your scenario is unfortunate.

Agreed entirely (as I also said last time this came up). I also don't
want a third path involving a set_throwable_handler() or similar; we
have one too many error handling paths already.

One problematic thing that we _can_ help mitigate is that the first
section of the backward incompatible changes page in the manual starts
with the changes to error and exception handling, but never mentions
this specific issue (type declarations on the exception handler
breaking). That's entirely my fault (I've mentioned it enough in
conference talks that I guess I thought I'd documented it, but
hadn't), and I'll go fix that now.

Adam

---

## #90535 — Andrea Faulds — 2016-01-12T04:47:46+00:00

URL: https://externals.io/message/90485#90535 · In reply to: #90518

Hi Adam,

Adam Harvey wrote:
> 
> > […]
>
> Agreed entirely (as I also said last time this came up). I also don't
> want a third path involving a set_throwable_handler() or similar; we
> have one too many error handling paths already.
>
> One problematic thing that we _can_ help mitigate is that the first
> section of the backward incompatible changes page in the manual starts
> with the changes to error and exception handling, but never mentions
> this specific issue (type declarations on the exception handler
> breaking). That's entirely my fault (I've mentioned it enough in
> conference talks that I guess I thought I'd documented it, but
> hadn't), and I'll go fix that now.

Since functions like that no longer concern just exceptions, but now the 
broader notion of "throwables", perhaps they should be renamed? It would 
make sense to me if it were now set_throwable_handler(), with an alias 
in place for the old name to avoid breaking compatibility.

Thanks.

-- 
Andrea Faulds
https://ajf.me/

---

## #90538 — Giovanni Giacobbi — 2016-01-12T09:37:30+00:00

URL: https://externals.io/message/90485#90538 · In reply to: #90492

I want to thank Markus and Björn for getting me up to speed with the
pointers.
Please see the rest of my response below.



> Giovanni Giacobbi wrote on 11/01/2016 13:31:
>
> > […]
>
> This is an interesting idea, but other than the specific transition to PHP
> 7, I'm not sure when it would be used.
>
> Put it this way: if Errors existed when you first wrote this code, would
> you want to handle them using your custom handler functions, or would you
> prefer them to the fall through to the default server White Screen of Doom?
> My guess is that you'd want to pass them to the handler, even if it
> immediately checked "if ($e instanceOf Error)" and used a different output
> style.
>
>
I would *obviously* use this mechanic if it was available, I'm sure I'm not
the only one frustrated by the fact that it was not possible to cach fatal
errors and had to make stunts with apache's error log to do some complete
error auditing.But the fact is that it did NOT exist, so when you say
"other than the specific transition to PHP 7", well I think it is a big
deal and motivates the introduction of a new feature just for that.

Since `set_exception_handler()` is intended as a last-ditch "something's gone
> very wrong" function anyway, I think it receiving all Throwables makes
> sense, even if the BC break in your scenario is unfortunate.
>
>
Yes, it is very unfortunate. Not for the two lines of codes that required
to fix the issue (I wrapped it in a if ($e instanceof Exception) { ... }
block), but because of the feeling this gave me about the general attitude
towards the community.

I love PHP and I enjoy programming in it, and I always defend it when my
peers and collegues talk [really] bad about it. In the past I always felt
that BCs I got in my project were inevitable or caused by poor
implementation or not strictly adhering to the documentation guidelines.
This is why when we migrated from PHP 4 to 5.2 we rewrote most of the code.
It was a big effort, but we did it by checking the documentation all the
time, respecting deprecates, and so on. Because of this migrations from 5.2
to 5.3 and 5.3 to 5.6 were painless, close to zero changes or just bugs
catching. Everything was done by the book.

Now comes this breakage, but this time, funny to say, I felt stub in the
back. That's why after years of being in read-only mode I decided to speak
up.

I identified two possible indipendent change paths, which I'd like you to
consider separately. Both seem better than my initial idea of the boolean
flag in `set_exception_handler()`:

1) Check the handler's signature compatibility before calling it.
Indeed there is no reason to fix a my_hnd(Exception $e) and not a
my_hnd(ExceptionA $e) where class ExceptionA extends Exception, so this
proposal would be to simply ignore the set_handler_exception()'s callback
if it is not compatible, or maybe even better issue an `E_NOTICE` or `E_STRICT` 
in such case.
Although it doesn't completely solve the original problem, it mitigates it
and makes the engine look better, as it comes at very little cost and
*zero* BC breaks (code wouldn't work anyway).

2) Introduce set_throwable_handler() and leave `set_exception_handler()` as
it was in PHP 5.x, i.e. catching only Exception descendants.
Indeed this is what I expected to see, i.e. everything worked as before,
and if I want to exploit this new powerful feature I have to "enable" it by
change just one single line of code!
Unfortunately as PHP7 is released this would hironically be BC break itself
now, so I understand it is never going to happen, but I want you to
consider it nonetheless to at least introduce set_throwable_handler() even
if `set_exception_handler()` stays broken (maybe mitigated by the change 1).
Also think about the possible upcoming changes to the Throwable definition,
I saw in another thread that you are thinking about dropping
Throwable::getCode(), which would make things even worse, in case of
something as simple as:
set_exception_handler(function($e) { print $e->getCode(); });

Thank you for listening to me, I want to stress that I really appreciate
what you do even when I disagree with it.

Kind regards
Giovanni

---

## #90540 — Umberto Salsi — 2016-01-12T10:59:35+00:00

URL: https://externals.io/message/90485#90540 · In reply to: #90538

giovanni@giacobbi.net (Giovanni Giacobbi) wrote:

> Also think about the possible upcoming changes to the Throwable definition,
> I saw in another thread that you are thinking about dropping
> Throwable::getCode(), which would make things even worse, in case of
> something as simple as:
> set_exception_handler(function($e) { print $e->getCode(); });

Don't be afraid, Giovanni, it was only my proposal to avoid another
function returning "mixed", because just this was the issue: while in
PHP 5 that method is expected to return a number, under PHP 7 it may now
return anything, possibly something that you cannot pass to the "print"
function whitout causing another error (printing an array generates a
 `E_NOTICE` an then only displays a bare "Array"); this means that you code
above not only is useless (you are printing the "code" of what?) but it is
also unsafe (it may generate another error, which is not a good thing).

Regards,
 ___ 
/_|_\  Umberto Salsi
\/_\/  www.icosaedro.it

---

## #90541 — Rowan Collins — 2016-01-12T11:18:49+00:00

URL: https://externals.io/message/90485#90541 · In reply to: #90538

Giovanni Giacobbi wrote on 12/01/2016 09:37:
> But the fact is that it did NOT exist, so when you say "other than the 
> specific transition to PHP 7", well I think it is a big deal and 
> motivates the introduction of a new feature just for that.

I guess what I meant was, once everyone has been using PHP 7 for years, 
it would be a shame to have features lying around in the language that 
existed only to ease the transition, and aren't even "legacy" in the 
sense of matching PHP 5.


> 1) Check the handler's signature compatibility before calling it.
> Indeed there is no reason to fix a my_hnd(Exception $e) and not a 
> my_hnd(ExceptionA $e) where class ExceptionA extends Exception, so 
> this proposal would be to simply ignore the set_handler_exception()'s 
> callback if it is not compatible, or maybe even better issue an 
> `E_NOTICE` or `E_STRICT` in such case.

I like this idea. Although I can't think of many cases outside the 
transition where it would be deliberately used, generating a fatal error 
when calling an error handler is not particular helpful, so just falling 
through and acting as though no handler existed would seem like a more 
useful fallback. I don't know how simple it would be to implement in the 
engine though...


Just on a general point, there's actually surprisingly *few* BC breaks 
in PHP 7, in my opinion. I don't think it's fair to extrapolate from 
this a "general attitude to the community" - I think it's an unfortunate 
BC break which could have had more attention, either to ensure it was 
clearly signalled, or to look at mitigation ideas like we are in this 
thread. An honest mistake, rather than somebody not caring.

Regards,
-- 
Rowan Collins
[IMSoP]