Disabling error handling

[AndreKR]

This is more or less a continuation of #143.

Currently graphql-php catches exceptions and returns information about them together with the partial result data.

I see a number of problems with this approach:
If an exception happened, something has gone exceptionally wrong. The exception should bubble up and go to Sentry or logs or whatever you're using to monitor exceptions. Depending on the type of exception, the exception handler might do additional work, for example for an SqlException my exception handler usually extracts the SQL query and includes that in the report.
With graphql-php catching the exceptions, all this exception handling code needs to be written twice to be able to handle normal exceptions and the error output of GraphQL::executeQuery() at the same time.
Furthermore, in the client there needs to be additional code to parse the error data and find out which data is affected, because "an exception happened fetching this data" and "the data is null" are two very different things.

There is a flag Debug::RETHROW_INTERNAL_EXCEPTIONS that re-throws all caught exceptions. But even with this flag there are three problems:

• The fact that it is in the class Debug suggests that it is a debug tool and should be disabled in production. This leads to a situation where there was proper exception handling in development but exceptions happening in production are leaked to the client instead of being reported.
• I noticed that if an exception has a $previous exception set, only one of them seems to end up in Sentry.
• When using Xdebug, the stack traces are messed up:
  
  Granted, this is a problem of PHP/Xdebug as it shows the stack trace of the throw when it should probably show the stack trace of the new \Exception(), but nevertheless this issue would be nonexistent if graphql-php wouldn't catch the exception in the first place.

Therefore I suggest implementing a configuration flag that completely disables the exception handling in graphql-php (i.e. run the same code but not inside a try) and leaves that to the application.

[vladar]

The reality is that we keep following reference implementation (graphql-js) and deviations from their core logic are costly in terms of maintenance.

We do deviate when a feature is really important, but the only serious problem I see in your description is Xdebug stack traces (and it probably requires some investigation).

Everything else can be implemented in the app code. I.e. just don't use Debug::RETHROW_INTERNAL_EXCEPTIONS and throw manually after execution (if it is a part of your app logic):

if (isset($result->errors[0]) {
    if ($result->errors[0]->getPrevious()) {
        // Error caused by the app-resolver
        throw $result->errors[0]->getPrevious(); 
    } else {
        // GraphQL-specific error
        throw $result->errors[0]; 
    }
}

If you need all exceptions in Sentry, I am pretty sure it supports logging exceptions without throwing.

Furthermore, in the client there needs to be additional code to parse the error data and find out which data is affected, because "an exception happened fetching this data" and "the data is null" are two very different things.

I don't see how throwing one exception is more useful for the client than returning a partial result with the same exception included in the errors. With exception thrown, you won't even have any way to "find out which data is affected" because there is no data.

So if you don't need it - just look at errors[0] and if it exists - process the error the same way you would do with an exception thrown.

If some nodes of a result may end up in a domain error - they should be modeled as a union: DomainError | MyObjectType and resolved accordingly.

Therefore I suggest implementing a configuration flag that completely disables the exception handling in graphql-php (i.e. run the same code but not inside a try) and leaves that to the application.

Apart from maintenance costs, configuration options multiply possibility of bugs (i.e. feature affects execution path with try/catch, but missed the path without) and complicate bug reproduction since reporter must post all of their options as a part of a bug report (and very few people do it during the first iteration).

We've seen too many times how a bunch of configurations simplifies work at first but complicates it during other stages of the app lifecycle.

Sometimes configuration options are inevitable, but if they can be avoided, it is better to avoid them.

[vladar]

Closing due to inactivity