Refactored error formatting (debugging part)

src/Error/Debug.php

@@ -0,0 +1,10 @@
+<?php
+namespace GraphQL\Error;
+
+
+class Debug
+{
+    const INCLUDE_DEBUG_MESSAGE = 1;
+    const INCLUDE_TRACE = 2;
+    const RETHROW_INTERNAL_EXCEPTIONS = 4;
+}

src/Error/FormattedError.php

@@ -13,10 +13,6 @@ use GraphQL\Utils\Utils;
  */
 class FormattedError
 {
-    const INCLUDE_DEBUG_MESSAGE = 1;
-    const INCLUDE_TRACE = 2;
-    const RETHROW_RESOLVER_EXCEPTIONS = 4;
-
     private static $internalErrorMessage = 'Internal server error';
 
     public static function setInternalErrorMessage($msg)
@@ -32,8 +28,6 @@ class FormattedError
      * @param bool|int $debug
      * @param string $internalErrorMessage
      * @return array
-     * @throws Error
-     *
      * @throws \Throwable
      */
     public static function createFromException($e, $debug = false, $internalErrorMessage = null)
@@ -44,36 +38,19 @@ class FormattedError
             Utils::getVariableType($e)
         );
 
-        if ($debug & self::RETHROW_RESOLVER_EXCEPTIONS > 0) {
-            if (!$e instanceof Error || $e->getPrevious()) {
-                throw $e;
-            }
-        }
-
-        $debug = (int) $debug;
         $internalErrorMessage = $internalErrorMessage ?: self::$internalErrorMessage;
 
         if ($e instanceof ClientAware) {
-            if ($e->isClientSafe()) {
+            $formattedError = [
-                $result = [
+                'message' => $e->isClientSafe() ? $e->getMessage() : $internalErrorMessage,
-                    'message' => $e->getMessage(),
+                'category' => $e->getCategory()
-                    'category' => $e->getCategory()
+            ];
-                ];
-            } else {
-                $result = [
-                    'message' => $internalErrorMessage,
-                    'category' => $e->getCategory()
-                ];
-            }
         } else {
-            $result = [
+            $formattedError = [
                 'message' => $internalErrorMessage,
                 'category' => Error::CATEGORY_INTERNAL
             ];
         }
-        if (($debug & self::INCLUDE_DEBUG_MESSAGE > 0) && $result['message'] === $internalErrorMessage) {
-            $result['debugMessage'] = $e->getMessage();
-        }
 
         if ($e instanceof Error) {
             $locations = Utils::map($e->getLocations(), function(SourceLocation $loc) {
@@ -81,48 +58,105 @@ class FormattedError
             });
 
             if (!empty($locations)) {
-                $result['locations'] = $locations;
+                $formattedError['locations'] = $locations;
             }
             if (!empty($e->path)) {
-                $result['path'] = $e->path;
+                $formattedError['path'] = $e->path;
-            }
-        } else if ($e instanceof \ErrorException) {
-            if ($debug) {
-                $result += [
-                    'file' => $e->getFile(),
-                    'line' => $e->getLine(),
-                    'severity' => $e->getSeverity()
-                ];
-            }
-        } else if ($e instanceof \Error) {
-            if ($debug) {
-                $result += [
-                    'file' => $e->getFile(),
-                    'line' => $e->getLine(),
-                ];
             }
         }
 
-        if ($debug & self::INCLUDE_TRACE > 0) {
+        if ($debug) {
+            $formattedError = self::addDebugEntries($formattedError, $e, $debug);
+        }
+
+        return $formattedError;
+    }
+
+    /**
+     * @param array $formattedError
+     * @param \Throwable $e
+     * @param bool $debug
+     * @return array
+     * @throws \Throwable
+     */
+    public static function addDebugEntries(array $formattedError, $e, $debug)
+    {
+        if (!$debug) {
+            return $formattedError;
+        }
+
+        Utils::invariant(
+            $e instanceof \Exception || $e instanceof \Throwable,
+            "Expected exception, got %s",
+            Utils::getVariableType($e)
+        );
+
+        $debug = (int) $debug;
+
+        if ($debug & Debug::RETHROW_INTERNAL_EXCEPTIONS) {
+            if (!$e instanceof Error) {
+                throw $e;
+            } else if ($e->getPrevious()) {
+                throw $e->getPrevious();
+            }
+        }
+
+        $isInternal = !$e instanceof ClientAware || !$e->isClientSafe();
+
+        if (($debug & Debug::INCLUDE_DEBUG_MESSAGE) && $isInternal) {
+            // Displaying debugMessage as a first entry:
+            $formattedError = ['debugMessage' => $e->getMessage()] + $formattedError;
+        }
+
+        if ($debug & Debug::INCLUDE_TRACE) {
+            if ($e instanceof \ErrorException || $e instanceof \Error) {
+                $formattedError += [
+                    'file' => $e->getFile(),
+                    'line' => $e->getLine(),
+                ];
+            }
+
             $isTrivial = $e instanceof Error && !$e->getPrevious();
 
             if (!$isTrivial) {
                 $debugging = $e->getPrevious() ?: $e;
-                $result['trace'] = static::toSafeTrace($debugging->getTrace());
+                $formattedError['trace'] = static::toSafeTrace($debugging);
             }
         }
+        return $formattedError;
+    }
 
-        return $result;
+    /**
+     * Prepares final error formatter taking in account $debug flags.
+     * If initial formatter is not set, FormattedError::createFromException is used
+     *
+     * @param callable|null $formatter
+     * @param $debug
+     * @return callable|\Closure
+     */
+    public static function prepareFormatter(callable $formatter = null, $debug)
+    {
+        $formatter = $formatter ?: function($e) {
+            return FormattedError::createFromException($e);
+        };
+        if ($debug) {
+            $formatter = function($e) use ($formatter, $debug) {
+                return FormattedError::addDebugEntries($formatter($e), $e, $debug);
+            };
+        }
+        return $formatter;
     }
 
     /**
      * Converts error trace to serializable array
      *
-     * @param array $trace
+     * @param \Throwable $error
      * @return array
      */
-    private static function toSafeTrace(array $trace)
+    public static function toSafeTrace($error)
     {
+        $trace = $error->getTrace();
+
         // Remove invariant entries as they don't provide much value:
         if (
             isset($trace[0]['function']) && isset($trace[0]['class']) &&
@@ -221,7 +255,7 @@ class FormattedError
         return [
             'message' => $e->getMessage(),
             'severity' => $e->getSeverity(),
-            'trace' => self::toSafeTrace($e->getTrace())
+            'trace' => self::toSafeTrace($e)
         ];
     }
 }

src/Executor/ExecutionResult.php

@@ -98,23 +98,13 @@ class ExecutionResult implements \JsonSerializable
         $result = [];
 
         if (!empty($this->errors)) {
-            if ($debug) {
-                $errorFormatter = function($e) use ($debug) {
-                    return FormattedError::createFromException($e, $debug);
-                };
-            } else if (!$this->errorFormatter) {
-                $errorFormatter = function($e) {
-                    return FormattedError::createFromException($e, false);
-                };
-            } else {
-                $errorFormatter = $this->errorFormatter;
-            }
-
             $errorsHandler = $this->errorsHandler ?: function(array $errors, callable $formatter) {
                 return array_map($formatter, $errors);
             };
-
+            $result['errors'] = $errorsHandler(
-            $result['errors'] = $errorsHandler($this->errors, $errorFormatter);
+                $this->errors,
+                FormattedError::prepareFormatter($this->errorFormatter, $debug)
+            );
         }
 
         if (null !== $this->data) {

src/Server/Helper.php

@@ -141,21 +141,20 @@ class Helper
             );
         }
 
-        $applyErrorFormatting = function (ExecutionResult $result) use ($config) {
+        $applyErrorHandling = function (ExecutionResult $result) use ($config) {
-            if ($config->getDebug()) {
+            if ($config->getErrorsHandler()) {
-                $errorFormatter = function($e) {
+                $result->setErrorsHandler($config->getErrorsHandler());
-                    return FormattedError::createFromException($e, true);
+            }
-                };
+            if ($config->getErrorFormatter() || $config->getDebug()) {
-            } else {
+                $result->setErrorFormatter(
-                $errorFormatter = $config->getErrorFormatter() ?: function($e) {
+                    FormattedError::prepareFormatter($config->getErrorFormatter(),
-                    return FormattedError::createFromException($e, false);
+                    $config->getDebug())
-                };
+                );
             }
-            $result->setErrorFormatter($errorFormatter);
             return $result;
         };
 
-        return $result->then($applyErrorFormatting);
+        return $result->then($applyErrorHandling);
     }
 
     /**

src/Server/ServerConfig.php

@@ -40,10 +40,15 @@ class ServerConfig
     private $rootValue;
 
     /**
-     * @var callable
+     * @var callable|null
      */
     private $errorFormatter;
 
+    /**
+     * @var callable|null
+     */
+    private $errorsHandler;
+
     /**
      * @var bool
      */
@@ -131,7 +136,7 @@ class ServerConfig
     }
 
     /**
-     * @return callable
+     * @return callable|null
      */
     public function getErrorFormatter()
     {
@@ -150,6 +155,26 @@ class ServerConfig
         return $this;
     }
 
+    /**
+     * Expects function(array $errors, callable $formatter) : array
+     *
+     * @param callable $handler
+     * @return $this
+     */
+    public function setErrorsHandler(callable $handler)
+    {
+        $this->errorsHandler = $handler;
+        return $this;
+    }
+
+    /**
+     * @return callable|null
+     */
+    public function getErrorsHandler()
+    {
+        return $this->errorsHandler;
+    }
+
     /**
      * @return PromiseAdapter
      */
@@ -243,27 +268,14 @@ class ServerConfig
     }
 
     /**
-     * Settings this option has two effects:
+     * Set response debug flags, see GraphQL\Error\Debug class for a list of available flags
      *
-     * 1. Replaces current error formatter with the one for debugging (has precedence over `setErrorFormatter()`).
+     * @param bool|int $set
-     *    This error formatter adds `trace` entry for all errors in ExecutionResult when it is converted to array.
-     *
-     * 2. All PHP errors are intercepted during query execution (including warnings, notices and deprecations).
-     *
-     *    These PHP errors are converted to arrays with `message`, `file`, `line`, `trace` keys and then added to
-     *    `extensions` section of ExecutionResult under key `phpErrors`.
-     *
-     *    After query execution error handler will be removed from stack,
-     *    so any errors occurring after execution will not be caught.
-     *
-     * Use this feature for development and debugging only.
-     *
-     * @param bool $set
      * @return $this
      */
     public function setDebug($set = true)
     {
-        $this->debug = (bool) $set;
+        $this->debug = $set;
         return $this;
     }
 

tests/Server/QueryExecutionTest.php

@@ -2,6 +2,7 @@
 namespace GraphQL\Tests\Server;
 
 use GraphQL\Deferred;
+use GraphQL\Error\Debug;
 use GraphQL\Error\Error;
 use GraphQL\Error\InvariantViolation;
 use GraphQL\Error\UserError;
@@ -136,7 +137,8 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase
 
     public function testDebugExceptions()
     {
-        $this->config->setDebug(true);
+        $debug = Debug::INCLUDE_DEBUG_MESSAGE | Debug::INCLUDE_TRACE;
+        $this->config->setDebug($debug);
 
         $query = '
         {
@@ -649,6 +651,72 @@ class QueryExecutionTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals('query', $operationType);
     }
 
+    public function testAppliesErrorFormatter()
+    {
+        $called = false;
+        $error = null;
+        $this->config->setErrorFormatter(function($e) use (&$called, &$error) {
+            $called = true;
+            $error = $e;
+            return ['test' => 'formatted'];
+        });
+
+        $result = $this->executeQuery('{fieldWithException}');
+        $this->assertFalse($called);
+        $formatted = $result->toArray();
+        $expected = [
+            'errors' => [
+                ['test' => 'formatted']
+            ]
+        ];
+        $this->assertTrue($called);
+        $this->assertArraySubset($expected, $formatted);
+        $this->assertInstanceOf(Error::class, $error);
+
+        // Assert debugging still works even with custom formatter
+        $formatted = $result->toArray(Debug::INCLUDE_TRACE);
+        $expected = [
+            'errors' => [
+                [
+                    'test' => 'formatted',
+                    'trace' => []
+                ]
+            ]
+        ];
+        $this->assertArraySubset($expected, $formatted);
+    }
+
+    public function testAppliesErrorsHandler()
+    {
+        $called = false;
+        $errors = null;
+        $formatter = null;
+        $this->config->setErrorsHandler(function($e, $f) use (&$called, &$errors, &$formatter) {
+            $called = true;
+            $errors = $e;
+            $formatter = $f;
+            return [
+                ['test' => 'handled']
+            ];
+        });
+
+        $result = $this->executeQuery('{fieldWithException,test: fieldWithException}');
+
+        $this->assertFalse($called);
+        $formatted = $result->toArray();
+        $expected = [
+            'errors' => [
+                ['test' => 'handled']
+            ]
+        ];
+        $this->assertTrue($called);
+        $this->assertArraySubset($expected, $formatted);
+        $this->assertInternalType('array', $errors);
+        $this->assertCount(2, $errors);
+        $this->assertInternalType('callable', $formatter);
+        $this->assertArraySubset($expected, $formatted);
+    }
+
     private function executePersistedQuery($queryId, $variables = null)
     {
         $op = OperationParams::create(['queryId' => $queryId, 'variables' => $variables]);

tests/Server/ServerConfigTest.php

@@ -17,6 +17,7 @@ class ServerConfigTest extends \PHPUnit_Framework_TestCase
         $this->assertEquals(null, $config->getContext());
         $this->assertEquals(null, $config->getRootValue());
         $this->assertEquals(null, $config->getErrorFormatter());
+        $this->assertEquals(null, $config->getErrorsHandler());
         $this->assertEquals(null, $config->getPromiseAdapter());
         $this->assertEquals(null, $config->getValidationRules());
         $this->assertEquals(null, $config->getDefaultFieldResolver());
@@ -77,6 +78,19 @@ class ServerConfigTest extends \PHPUnit_Framework_TestCase
         $this->assertSame($formatter, $config->getErrorFormatter());
     }
 
+    public function testAllowsSettingErrorsHandler()
+    {
+        $config = ServerConfig::create();
+
+        $handler = function() {};
+        $config->setErrorsHandler($handler);
+        $this->assertSame($handler, $config->getErrorsHandler());
+
+        $handler = 'date'; // test for callable
+        $config->setErrorsHandler($handler);
+        $this->assertSame($handler, $config->getErrorsHandler());
+    }
+
     public function testAllowsSettingPromiseAdapter()
     {
         $config = ServerConfig::create();

tests/ServerTest.php

@@ -1,6 +1,7 @@
 <?php
 namespace GraphQL\Tests;
 
+use GraphQL\Error\Debug;
 use GraphQL\Error\FormattedError;
 use GraphQL\Error\InvariantViolation;
 use GraphQL\Error\UserError;
@@ -494,7 +495,8 @@ class ServerTest extends \PHPUnit_Framework_TestCase
 
         $server->setDebug(Server::DEBUG_EXCEPTIONS);
         $server->setExceptionFormatter(function($e) {
-            return FormattedError::createFromException($e, true);
+            $debug = Debug::INCLUDE_TRACE;
+            return FormattedError::createFromException($e, $debug);
         });
         $result = $server->executeQuery('{withException}');