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:

<?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.

Files: 
CommentFileSizeAuthor
#5 drupal8.errorexception.5.patch27.37 KBsun
FAILED: [[SimpleTest]]: [MySQL] 64,386 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Comments

Subscribing, if we can do this right it'd be great.

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;
}
?>

Switching something like #1283892: Let Render API fail in a tale-telling way on invalid $element to this would be great, once it exists.

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;
}
?>

Assigned:Unassigned» sun
Status:Active» Needs review
StatusFileSize
new27.37 KB
FAILED: [[SimpleTest]]: [MySQL] 64,386 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

Humm... 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 with ErrorExceptions and custom exceptions.

Beyond that, I now consider the current FlattenException as a flawed idea, because an FlattenException is not an Exception — Somehow, I managed to get a FlattenException 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 an Exception... When I changed the code to catch (\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 actual Exception and still support the primary idea of making it Serializable. 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 ErrorExceptions fundamentally means that any kind of PHP Strict Notice, Notice, Warning, or Error will halt the currently executed code, unless the ErrorException 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 ErrorExceptions.

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.

Status:Needs review» Needs work

The last submitted patch, 5: drupal8.errorexception.5.patch, failed testing.

E_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.

Related, someone just brought this question up on FIG: https://groups.google.com/forum/#!msg/php-fig/T4mtQc6qyaE/q0Wq1leM_KAJ