Summary
Drupal core often calls PHP functions with the @ prefix to hide errors, checking only the return value to see if the function call succeeded.
[bfroehle@carmel drupal (8.x)]$ find . -type f -not -name "system.tar.inc" | xargs egrep "@[a-zA-Z_]+\(" --binary-files=without-match | wc -l
43
In some cases, the warning returned by the command would be useful to distinguish between a number of errors but the use of the @ warning suppression suppresses any useful information from the error. (cf. #1002048: Work around move_uploaded_file issues with safe_mode and open_basedir)
Converting these errors to exceptions, allowing for the use of a try/catch framework, could lead to more robust code and more useful debugging messages.
PHP Error Handling and ErrorException
PHP offers an ErrorException class which, with an appropriate error handler set, converts a PHP error into a usable PHP exception. Using the ErrorException class is straightforward:
function exception_error_handler($errno, $errstr, $errfile, $errline ) {
throw new ErrorException($errstr, 0, $errno, $errfile, $errline);
}
set_error_handler("exception_error_handler");
/* Trigger exception */
strpos();
ErrorException is fully supported in PHP 5.3. In PHP 5.2 the stack trace of the exception is broken and won't be fixed.
Using ErrorException within Drupal
Since Drupal already provides an error handler, the preferred pattern might be to only use a temporary error exception handler:
function drupal_move_uploaded_file($filename, $uri) {
set_error_handler("drupal_exception_error_handler");
try {
$result = move_uploaded_file($filename, $uri);
}
catch (ErrorException $e) {
// Try again using drupal_realpath()
// ...
}
restore_error_handler();
return $result;
}
This is relatively tedious, however the number of usages of @function() in core is modest (under 50).
Alternatives
Another option would be to globally replace _drupal_error_handler() with code that produces an ErrorException -- perhaps only for errors of a certain level or higher -- but given the large numbers of warnings which may exist but be hidden on a site this may not be prudent.
Recommendation
Investigate replacing some @function()
calls with corresponding try/catch blocks (and set_error_handler("drupal_exception_error_handler");
) to see if better error messages can be produced or code structure simplified.
Comment | File | Size | Author |
---|---|---|---|
#5 | drupal8.errorexception.5.patch | 27.37 KB | sun |
Comments
Comment #1
catchSubscribing, if we can do this right it'd be great.
Comment #2
Akaoni CreditAttribution: Akaoni commentedFor simplified usage, could we wrap this in a wrapper function?
Eg.
Edit: alternatively:
Comment #3
tim.plunkettSwitching something like #1283892: Let Render API fail in a tale-telling way on invalid $element to this would be great, once it exists.
Comment #4
mikeytown2 CreditAttribution: mikeytown2 commentedHere just to point out that you can capture error messages even if @ error suppression is used. Example of this is in the memcache module in dmemcache_get().
I'm totally for some sort of wrapper to make this easier.
Using the ErrorException Class sounds like a good way to make this happen. If set_error_handler() & restore_error_handler() doesn't work, we can fall back to using $php_errormsg.
Untested Example of using $php_errormsg
Comment #5
sunHumm... I really want(ed) this, because I do not want to put @error-suppressed calls into some new core component code.
However, after implementing it, I discovered that Symfony's
FlattenException
removes access to the original Exception, so you cannot work withErrorException
s and custom exceptions.Beyond that, I now consider the current
FlattenException
as a flawed idea, because anFlattenException
is not anException
— Somehow, I managed to get aFlattenException
to get re-thrown in my tests (even though PHP technically does not support that), and thus, the runtime code did not catch the exception, because it's not anException
... When I changed the code tocatch (\Symfony\Component\Debug\Exception\FlattenException $e)
, it was caught.I've presented an idea in aforementioned Symfony PR for how
FlattenException
could possibly be rewritten to be an actualException
and still support the primary idea of making itSerializable
. But of course, that's vapor-ware right now.FWIW, I did not understand why we're actually using
FlattenException
at all in our code.Lastly, the entire paradigm of
ErrorException
s fundamentally means that any kind of PHP Strict Notice, Notice, Warning, or Error will halt the currently executed code, unless theErrorException
is caught. Instead of one or more error messages on screen, your code will not run. — This could easily turn out to become very troublesome DX in case you're debugging/testing some long-running process and e.g. you merely forgot to declare a variable somewhere... :-/I did not test what the performance difference is between @error-suppression vs. (1) error handler callback + (2) throw new ErrorException + depending on context (3) FlattenException.
In short: First, I really wanted this. Now I'm rather undecided. Neither PHP core nor Symfony seem to provide an adequate architecture to cope with
ErrorException
s.Anyway, attached patch is a fully working implementation. It retains support for @error-suppression, so that we do not have to convert all code at once.
Comment #7
Crell CreditAttribution: Crell commentedE_RECOVERABLE_ERROR, at least, ought to be converted to ErrorException. We can certainly do that selectively (eg, not do that for E_NOTICE but do it for E_WARNING and E_RECOVERABLE_ERROR) if that ends up being cleaner. (See #2165589: Convert E_RECOVERABLE_ERROR to an Exception)
I don't think we're using FlattenException anywhere other than in the code we inherited directly from Symfony for error handling. Once you're in that pipeline you should just be setting a response and that's it.
Comment #8
Crell CreditAttribution: Crell commentedRelated, someone just brought this question up on FIG: https://groups.google.com/forum/#!msg/php-fig/T4mtQc6qyaE/q0Wq1leM_KAJ
Comment #12
jibranDo we need a policy here before the patch?