Jump to:
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | base system |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Issue Summary
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
43In 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:
<?php
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:
<?php
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.
Comments
#1
Subscribing, if we can do this right it'd be great.
#2
For simplified usage, could we wrap this in a wrapper function?
Eg.
<?php
function drupal_move_uploaded_file($filename, $uri) {
try {
$result = drupal_throw_warnings('move_uploaded_file', $filename, $uri);
}
catch (ErrorException $e) {
// Try again using drupal_realpath()
// ...
}
return $result;
}?>
Edit: alternatively:
<?php
function drupal_move_uploaded_file($filename, $uri) {
drupal_throw_warnings(TRUE);
try {
$result = move_uploaded_file($filename, $uri);
}
catch (ErrorException $e) {
// Try again using drupal_realpath()
// ...
}
drupal_throw_warnings(FALSE);
return $result;
}?>
#3
Switching something like #1283892: Let Render API fail in a tale-telling way on invalid $element to this would be great, once it exists.
#4
Here 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
<?php
function drupal_move_uploaded_file($filename, $uri) {
try {
$result = drupal_run_and_throw('move_uploaded_file', array($filename, $uri));
}
catch (ErrorException $e) {
// Try again using drupal_realpath()
// ...
}
return $result;
}
function drupal_run_and_throw($callback, $parameters) {
// Setup error capturing.
$track_errors = ini_set('track_errors', '1');
$php_errormsg = '';
// Run the callback.
$return = @call_user_func_array($callback, $parameters);
// Check for errors.
if (!empty($php_errormsg)) {
throw new ErrorException($php_errormsg, 0, 0, __FILE__, __LINE__);
$php_errormsg = '';
}
ini_set('track_errors', $track_errors);
// Return result from callback.
return $return;
}
?>