Make reporting of client-safe errors configurable

[remipelhate]

• Added or updated tests
• Documented user facing changes
• Updated CHANGELOG.md

Changes

While it makes perfect sense to not pass client safe errors to Laravel's exception handler by default, in some cases, it can be useful to be able to do so.

Laravel allows to define a minimum log level at which each individual log channel is invoked. For example, sentry can be set to only report errors with a minimum level of warning, whereas slack only gets triggered as of critical:

// config/logging.php

return [
    'channels' => [
        'slack' => [
            'driver' => 'slack',
            'url' => env('LOG_SLACK_WEBHOOK_URL'),
            'username' => 'Laravel Log',
            'emoji' => ':boom:',
            'level' => env('LOG_LEVEL', Psr\Log\LogLevel::CRITICAL),
        ],
        'sentry' => [
            'driver' => 'sentry',
            'level' => env('LOG_LEVEL', Psr\Log\LogLevel::WARNING),
            'bubble' => true,
        ],
    ],
];

I've used this setup for a project where the team wanted to be able to track all authentication and authorization exceptions in Prometheus, as those can give insights on potential ACL issues or misuse. The problem is that Lighthouse marks these errors as client safe, causing them not to be passed to Laravel's exception handler.

This PR introduces a new Nuwave\Lighthouse\Execution\AlwaysReportingErrorHandler which can be used instead of Nuwave\Lighthouse\Execution\ReportingErrorHandler in the lighthouse.php config:

'error_handlers' => [
-    Nuwave\Lighthouse\Execution\ReportingErrorHandler::class,
+    Nuwave\Lighthouse\Execution\AlwaysReportingErrorHandler::class,
],

When using Nuwave\Lighthouse\Execution\AlwaysReportingErrorHandler, client-safe exceptions will be passed to the
default Laravel exception handler, allowing you to configure appropriate error reporting outside of Lighthouse.

Breaking changes

No

[spawnia]

I am not fully convinced that a configuration option at that level is the best option. The problem I see with this approach is that its value is only used from within ReportingErrorHandler, which is configurable in itself. Thus, for users that omit or replace ReportingErrorHandler, the option report_client_safe_errors has no effect.

[remipelhate]

I am not fully convinced that a configuration option at that level is the best option. The problem I see with this approach is that its value is only used from within ReportingErrorHandler, which is configurable in itself. Thus, for users that omit or replace ReportingErrorHandler, the option report_client_safe_errors has no effect.

That's a valid concern. Would you perhaps prefer to solve this on the ReportingErrorHandler itself? I noticed first-party Laravel packages are using static config callbacks more often lately, so perhaps that could be an option? Something along the lines of:

// app/Providers/GraphQLServiceProvider.php

public function boot(): void
{
    ReportingErrorHandler::reportClientSafeErrors();
}

I'm not always keen on static config, but it does fit Laravel's way of working.

Another option could be to bind it to the container and extend the instance when resolving it:

// app/Providers/GraphQLServiceProvider.php

public function boot(): void
{
    $this->app->extend(ReportingErrorHandler::class, fn (ReportingErrorHandler $errorHandler) => $errorHandler->reportClientSafeErrors());
}

This requires some more code, but removes the static context.

[remipelhate]

Or perhaps we could solve this through a named constructor on the ReportingErrorHandler which could be referenced in the config?

// config/lighthouse.php
return [
    'error_handlers' => [
        [Nuwave\Lighthouse\Execution\ReportingErrorHandler::class, 'includeClientSafeErrors'],
        // or...
        Nuwave\Lighthouse\Execution\ReportingErrorHandler::class . '@includeClientSafeErrors',
    ],
];

[spawnia]

Another option would be to add a new class to replace ReportingErrorHandler, perhaps called AlwaysReportingErrorHandler?

[remipelhate]

Another option would be to add a new class to replace ReportingErrorHandler, perhaps called AlwaysReportingErrorHandler?

Sure, that might work too. On it.

[spawnia]

Thank you @remipelhate, released with https://github.com/nuwave/lighthouse/releases/tag/v6.49.0.