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
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
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]
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
Hi Adam,
Adam Harvey 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/
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:
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():
-
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 anE_NOTICEorE_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). -
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
ifset_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
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
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.
- 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_NOTICEorE_STRICTin 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]
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:
Then it was about if a set_throwable_handler should be introduced.
Regards //Björn Larsson