Installer catches exceptions and manually re-prints them; does not use error/exception handler

Since this prevents people from solving their own problems (or at the very least from posting a bug report anyone else could have a hope of solving), escalating to major.

Could $e->getTraceAsString() be a better solution?

1. +++ b/core/includes/errors.inc
   @@ -215,14 +216,21 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +        $output = install_display_output($message, $GLOBALS['install_state']);
   +      }
   

   Last 2 lines of install_display_output() (not within any if or other condition):

     print theme('install_page', array('content' => $output));
     exit;
   

   So I'm afraid that this is not going to solve the related issue about the undefined theme() function, or that it will help at all. Also the exit prevents setting the status code to 500 and using the response object at all.

2. +++ b/core/includes/errors.inc
   @@ -215,14 +216,21 @@ function _drupal_log_error($error, $fatal = FALSE) {
          if ($fatal) {
            $response->setStatusCode(500, '500 Service unavailable (with message)');
   

   if ($fatal) within an if ($fatal)? OK, it is not code that is touched by this patch, but still. So shouldn't we just remove the inner if?

So, we accept that:
- when the installer is running we do not return status code 500, but try to display the error within the installer "wizard".
- #2091501: When install fails: Fatal error: Call to undefined function theme() in install.core.inc on line 954 will not be solved by this patch. A patch for that issue could just check if the theme() function exists before calling it and exiting. If it does not exist, just return and let _drupal_log_error() finish the work.

Personally, I'm fine with that. So RTBC for me.

Note: all exceptions that are thrown between the start of install_begin_request() and the call to drupal_maintenance_theme(); will not be themable. And there is a lot that can go wrong. (But this remark is more for the other issue).

I've manually tested this by throwing an exception in SystemListingInfo::profile() on the first call to it. The patch definitely improves the situation.

Without the patch

Additional uncaught exception thrown while handling exception.

Original

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "theme.negotiator" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 873 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Additional

Drupal\Core\Config\StorageException: Missing configuration file: system.theme.global in Drupal\Core\Config\InstallStorage->getFilePath() (line 54 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Config/InstallStorage.php).

With the patch

Additional uncaught exception thrown while handling exception.

Original

Exception: blah in Drupal\Core\SystemListingInfo->profiles() (line 25 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/SystemListingInfo.php).

Additional

Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "theme.negotiator" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 873 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

The new message now shows exactly where I threw the exception. In HEAD it's totally obscured.

Committed/pushed to 8.x, thanks!

1. +++ b/core/includes/errors.inc
   @@ -248,13 +253,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +  $container = \Drupal::getContainer();
   +  if (is_object($container) && $container->has('config.factory')) {
   +    return $container->get('config.factory')->get('system.logging')->get('error_level');
   

   What makes you think that this will never throw an exception. In other words: why not leave the try - catch intact but change the code to return _VERBOSE in the catch?

   Question: if ...get('error_level') returns, will it always return a proper value or should we check that as well? (During error handling I become even more paranoid than I already am in normal situations).

2. +++ b/core/includes/errors.inc
   @@ -119,11 +119,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +    // The installer initializes a maintenance theme at the earliest possible
   +    // point in time already.
   +    if (!$is_installer) {
   +      unset($GLOBALS['theme']);
   +    }
   

   I had a hard time understanding this comment. only after looking at the diff I got it a bit. The problem with the comment is that it talks about installer, but subsequently the condition is when not in the installer. So change to something like: The installer initializes a maintenance theme at the earliest possible point in time. Do not unset that.
   (Or describe why we would want to unset it when we are not in the installer)

3. +++ b/core/includes/install.core.inc
   @@ -953,6 +955,11 @@ function install_full_redirect_url($install_state) {
   +  // Ensure the maintenance theme is initialized.
   +  // The regular initialization call in install_begin_request() may not be
   +  // reached in case of an early installer error.
   +  drupal_maintenance_theme();
   +
   

   How sure can we be that this will not fail fatally?(undefined function ... and we have no useful error information at all anymore)
   What happens if it fails with an error/exception?

I can agree that the error handling as is, is complex, is spaghetti code, and has to many special cases, So my review was not about understanding the code but understanding (the implications of) the changes and if they could make sense. Therefore my remarks are more posed as questions than as things that must be changed.

What would be typical places in the code to insert throwing an exception to see the changes before and after?

With the latest patch

Additional uncaught exception thrown while handling exception.

Original

Exception: blah in Drupal\Core\SystemListingInfo->profiles() (line 25 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/SystemListingInfo.php).

Drupal\Core\SystemListingInfo->profiles('themes')
Drupal\Core\SystemListing->scan('/^[a-zA-Z_\x7f-\xff][a-zA-Z0-9_\x7f-\xff]*\.info.yml$/', 'themes', 'name', 1)
Drupal\Core\Extension\ThemeHandler->rebuildThemeData()
Drupal\Core\Extension\ThemeHandler->listInfo()
list_themes()
_drupal_maintenance_theme()
drupal_maintenance_theme()
install_begin_request(Array)
install_drupal()
Additional

Drupal\Core\Config\StorageException: Missing configuration file: system.theme.global in Drupal\Core\Config\InstallStorage->getFilePath() (line 54 of /Volumes/devdisk/dev/sites/drupal8alt.dev/core/lib/Drupal/Core/Config/InstallStorage.php).

Drupal\Core\Config\InstallStorage->getFilePath('system.theme.global')
Drupal\Core\Config\FileStorage->exists('system.theme.global')
Drupal\Core\Config\FileStorage->read('system.theme.global')
Drupal\Core\Config\FileStorage->readMultiple(Array)
Drupal\Core\Config\ConfigFactory->loadMultiple(Array)
Drupal\Core\Config\ConfigFactory->get('system.theme.global')
Drupal::config('system.theme.global')
theme_get_setting('features.favicon')
template_preprocess_maintenance_page(Array)
template_preprocess_install_page(Array, 'install_page', Array)
theme('install_page', Array)
install_display_output('The website has encountered an error. Please try again later.', Array)
_drupal_log_error(Array, 1)
_drupal_exception_handler(Object)

Review

1. +++ b/core/includes/errors.inc
   @@ -248,13 +253,16 @@ function _drupal_log_error($error, $fatal = FALSE) {
   -  try {
   -    return \Drupal::config('system.logging')->get('error_level');
   +  // Raise the error level to maximum for the early installer, so users are able
   +  // to file proper bug reports for early installer errors.
   +  if (drupal_installation_attempted() && empty($GLOBALS['install_state']['base_system_verified'])) {
   +    return ERROR_REPORTING_DISPLAY_VERBOSE;
      }
   -  catch (Exception $e) {
   -    // During very early install the cache_config table does not exist.
   -    return ERROR_REPORTING_DISPLAY_ALL;
   +  $container = \Drupal::getContainer();
   +  if (is_object($container) && $container->has('config.factory')) {
   +    return $container->get('config.factory')->get('system.logging')->get('error_level');
      }
   +  return ERROR_REPORTING_DISPLAY_VERBOSE;
   

   Another case for \Drupal::has() :) - but actually I don't understand why we don't just change the return in the catch.

2. +++ b/core/includes/install.core.inc
   @@ -494,7 +496,7 @@ function install_begin_request(&$install_state) {
   -  $module_handler->load('system');
   +  $module_handler->loadAll();
   

   Why is this change necessary?

3. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -89,9 +89,14 @@ public static function decodeException(\Exception $exception) {
      public static function renderExceptionSafe(\Exception $exception) {
        $decode = static::decodeException($exception);
   +    $backtrace = $decode['backtrace'];
        unset($decode['backtrace']);
   +    // Remove 'main()'.
   +    array_shift($backtrace);
    
   -    return String::format('%type: !message in %function (line %line of %file).', $decode);
   +    $output = String::format('%type: !message in %function (line %line of %file).', $decode);
   +    $output .= '<pre>' . static::formatBacktrace($backtrace) . '</pre>';
   +    return $output;
      }
   

   So this change ensures a verbose message if an secondary exception is thrown in _drupal_exception_handler or if an exception is thrown in _drupal_shutdown_function(). Seems like a good idea.

+++ b/core/includes/errors.inc
@@ -248,13 +256,23 @@ function _drupal_log_error($error, $fatal = FALSE) {
+  if (is_object($container) && $container->has('config.factory')) {
+    return $container->get('config.factory')->get('system.logging')->get('error_level');
   }
+  // If there is no container or if it has no config.factory service, we are

I now understand that this will never fail with an exception. So I tend to be fine with removing that try-catch. But OTOH, I still think that it would be cleaner and more readable to do use a try-catch. Many people after me will wonder (and loose time on) if this can fail?

I also had a situation in which it returned NULL. (Upgrading to HEAD more often seems to render my local site unusable than not. Is there something I can do to prevent that?). Should that situation be handled? and if so, how? Should returning NULL mean that we're in serious trouble thus return ERROR_REPORTING_DISPLAY_ALL?

All other parts of the patch are OK.

Thanks, I'm fine with the patch now.

Committed 643dab0 and pushed to 8.x. Thanks!