Throwable and Error exceptions break existing PHP 5.x code

php.internals

Giovanni Giacobbi

10 years ago
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

Markus Fischer

10 years ago
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 On 11.01.16 14:31, Giovanni Giacobbi wrote:

Rowan Collins

10 years ago
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]

Adam Harvey

10 years ago
On 11 January 2016 at 06:05, Rowan Collins <rowan.collins@gmail.com> wrote:
> 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

Andrew Faulds

10 years ago
Hi Adam, Adam Harvey wrote:
> On 11 January 2016 at 06:05, Rowan Collins <rowan.collins@gmail.com> wrote: >> 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.
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/

Giovanni Giacobbi

10 years ago
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. On 11 January 2016 at 15:05, Rowan Collins <rowan.collins@gmail.com> wrote:
> 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. > >
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

Umberto Salsi

10 years ago
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

Rowan Collins

10 years ago
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]

Björn Larsson

10 years ago
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