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.

CommentFileSizeAuthor
#5 drupal8.errorexception.5.patch27.37 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

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

Akaoni’s picture

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;
}?>
tim.plunkett’s picture

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

mikeytown2’s picture

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

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;
}
sun’s picture

Assigned: Unassigned » sun
Status: Active » Needs review
FileSize
27.37 KB

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.

Crell’s picture

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.

Crell’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

Assigned: sun » Unassigned
Status: Needs work » Active

Do we need a policy here before the patch?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.