So testing out PHP 7 I triggered an \EngineException() in Drupal 8. For
various reasons that don't really matter here Drupal uses
set_exception_handler() to provide its own exception handler. What is
important is that the methods used by that handler type hint \Exception for
their arguments. The immediately fatals.
By design, \EngineException does not extend \Exception so code doesn't
accidentally catch this special type of exception. Unfortunately they still
always make their way into this exception handler. Unfortunately this means
we have to remove the type hinting to support forward compatibility which
is a bit akward and unfortunate.
I don't know if this is all acceptable and/or by design but it is awkward
so I wanted to bring it to the list to discuss.
Simplified test case. Sorry for not putting it on 3v4l but its currently
unreachable.
https://gist.github.com/neclimdul/ab29ddb223cfc197e635
Hi James,
By design, \EngineException does not extend \Exception so code doesn't
accidentally catch this special type of exception. ...I don't know if this is all acceptable and/or by design but it is awkward
so I wanted to bring it to the list to discuss.
Let me try to explain why the BC break at the top level is the right choice.
The change in the Exception hierarchy has to cause a BC break to exist
'somewhere', as errors in code that were previously not throwing an
exception, but in effect just doing exit() are now throwing an
exception. This is a change in behaviour that cannot be handled
without some sort of BC break.
There are two types of places where \Exception are currently being caught:
i) in the middle of applications where some library is being called
where all exceptions need to be caught and changed to a more specific
exception.
function foo() {
try {
someOtherLibrary();
}
catch (\Exception $e) {
throw new OtherLibraryException(
$e->getMessage(),
$e->getCode,
$e
);
}
}
ii) At the top level of the application where all exceptions are being
caught, so that they can be logged and a more graceful error message
than a stack trace can be shown to the user.
If the BC break was for (i) by making all of the new exceptions be
under the hierarchy of \Exception, it would result in a lot of BC
break spread out across applications.
Having the BC break in (ii) is pretty strongly preferable; it means
that there are just a few (or one), easy to find places in an
application where the code now needs to be changed from catching
\Exception to catching \BaseException (or whatever it will be after
the exception tidy up).
Drupal (or any other application) can handle this BC break reasonably
easily by either dropping the type from the parameter of
_default_exception_handler or by adding a version check around it's
declaration like:
if (PHP_VERSION_ID >= 700000) {
function _default_exception_handler(\BaseException $e) {
}
}
else {
function _default_exception_handler(\Exception $e) {
}
}
Yeah, this isn't the most awesome thing ever to have to do, but imo it
sure beats having to change code in the middle of applications.
cheers
Dan
That is a stellar response Dan, Thanks for being so clear. I had a feeling
that this might be the case so we'll have to work around it.
Just for clarity and posterity I provided the simplest test case in the
original email not the actual problem. In Drupal this isn't just one place
though becase, we have a library(and by library I mean class) for parsing
information out of exceptions. So we would have to do quite a bit more
hackery then that to to keep type hinting.
Hi James,
By design, \EngineException does not extend \Exception so code doesn't
accidentally catch this special type of exception. ...I don't know if this is all acceptable and/or by design but it is awkward
so I wanted to bring it to the list to discuss.Let me try to explain why the BC break at the top level is the right
choice.The change in the Exception hierarchy has to cause a BC break to exist
'somewhere', as errors in code that were previously not throwing an
exception, but in effect just doing exit() are now throwing an
exception. This is a change in behaviour that cannot be handled
without some sort of BC break.There are two types of places where \Exception are currently being caught:
i) in the middle of applications where some library is being called
where all exceptions need to be caught and changed to a more specific
exception.function foo() {
try {
someOtherLibrary();
}
catch (\Exception $e) {
throw new OtherLibraryException(
$e->getMessage(),
$e->getCode,
$e
);
}
}ii) At the top level of the application where all exceptions are being
caught, so that they can be logged and a more graceful error message
than a stack trace can be shown to the user.If the BC break was for (i) by making all of the new exceptions be
under the hierarchy of \Exception, it would result in a lot of BC
break spread out across applications.Having the BC break in (ii) is pretty strongly preferable; it means
that there are just a few (or one), easy to find places in an
application where the code now needs to be changed from catching
\Exception to catching \BaseException (or whatever it will be after
the exception tidy up).Drupal (or any other application) can handle this BC break reasonably
easily by either dropping the type from the parameter of
_default_exception_handler or by adding a version check around it's
declaration like:if (PHP_VERSION_ID >= 700000) {
function _default_exception_handler(\BaseException $e) {
}
}
else {
function _default_exception_handler(\Exception $e) {
}
}Yeah, this isn't the most awesome thing ever to have to do, but imo it
sure beats having to change code in the middle of applications.cheers
Dan