Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error handling still problematic #3287

Open
plasid opened this issue Sep 20, 2023 · 9 comments
Open

Error handling still problematic #3287

plasid opened this issue Sep 20, 2023 · 9 comments

Comments

@plasid
Copy link

plasid commented Sep 20, 2023

This issue had been raised before.
A lot of boilerplate code is needed to implement error handling, but the main problem is how Slim handles and overrides this internally, and then the order of defining middlewares and error handling in a complex application, you should not have to read the docs for this, cause if you miss it or do it wrong then your error handling will not work correctly.
Imagine a typical scenario where you need to handle and log ALL types of errors gracefully with your own views loggers etc, including fatal errors, this means in some cases your error handling needs to be "active" even before Slim boots, now reading the docs and tutorials it becomes an absolute verbose mess.

I've been working with Slim for a few years and error handling remains a pain point.
Can I suggest that you either remove error handling completely from the core and only throw, and the developer can wrap the application with error handlers any way he/she sees fit.
Or at least supply a configuration option to disable Slim's error handling completely.
Its really a trivial matter and should not be this complicated.

@odan
Copy link
Contributor

odan commented Sep 20, 2023

Thank you for your comprehensive feedback. It's clear that you have a deep understanding of Slim 4 and its error-handling mechanisms, and we value your perspective.

First, I wanted to add some clarification about PHP error types to our conversation. As you're likely aware, PHP has multiple types of "errors":

  • Traditional PHP Errors: These include E_NOTICE, E_WARNING, E_ERROR, etc.
  • Exception-based Errors: These involve try-catch blocks and are part of the Exception hierarchy.

In its current form, Slim's ErrorHandler is designed to catch and handle exception-based errors, but it doesn't natively handle traditional PHP errors. The core framework expects you to handle these kinds of errors separately, and you can use PHP's own register_shutdown_function() and set_error_handler() functions or similar mechanisms such as Symfony ErrorHandler.

use Symfony\Component\ErrorHandler\ErrorHandler;
// ...

// Throw an exception when a PHP error occurs
// and handled it with the Slim error handler.
ErrorHandler::register();

Regarding your concern about Slim's default error handling getting in the way, I wanted to point out that you can actually remove Slim's default ErrorHandler by simply not adding it in the first place or removing the line $app->addErrorMiddleware(...); from your code. This gives you full control to implement your own error-handling strategy without any interference from Slim's built-in mechanisms.

@plasid
Copy link
Author

plasid commented Sep 20, 2023

Thanks for the reply Odan,

Any type of error (even fatal) can occur inside a try-catch (try-catch throwable does not know the future), i.e. in a controller or middleware, as you said, Slim will not handle fatal errors and other "traditional" errors, and that is exactly the problem, if it cannot handle ALL errors then rather remove error handling, because.
Now you need to do gymnastics to implement supplementary error handlers, (I will handle these, Slim will handle those....) and configure it accordingly or maybe not... swtich addErrorMiddleware and setDefaultErrorHandler on/off and get different results (I've been using these methods for a long time).
Wouldn't it be nice to have just one, simple, clean, standard, unified way to handle errors in Slim?

@odan
Copy link
Contributor

odan commented Sep 20, 2023

You've raised some excellent points. It's clear that you've run into the complexities and limitations of Slim's current error-handling system. (I've had the same issues with it)

Note that that Slim's ErrorMiddleware operates within an HTTP-specific context. It's designed to work seamlessly with HTTP request and response objects, making it well-suited for errors that occur during the request-handling process.

However, "traditional" PHP errors, including fatal errors, can occur even before Slim starts running - perhaps during the bootstrap process or other early initialization steps. These errors don't necessarily have an HTTP context, and therefore using a middleware-based approach for handling them doesn't make much sense.

Your suggestion to have a single, unified system to handle all types of errors is very interesting, but due to the "nature" of PHP itself (and not Slim) this is quite "complex". I think it would make more sense to let the developers decide of and how they want to handle a "Throwable" and old-school Errors.

@plasid
Copy link
Author

plasid commented Sep 20, 2023

Yep, I'm using a decoupled error handler for all 4 major types, was hoping Slim 5 could address the above - but not a deal breaker, Slim is still amazing.

@akrabat
Copy link
Member

akrabat commented May 1, 2024

Error handling feels like the hardest part of Slim and we've had a few runs at it with different approaches in versions 2, 3 and 4. I'm confident that we can further improve.

I'm happy to look at a proposal for another take with a clear list of benefits, along with ideas on how someone would migrate from 4's error handling to the new one.

My preference would be for an iterative improvement for 5 to minimise the update pain.

@odan
Copy link
Contributor

odan commented Jun 15, 2024

I could imagine for Slim v5 the possibility of moving the ErrorMiddleware into a dedicated Slim package. This would help migration to v5 by allowing users to install this Middleware separately if required. For custom error-handling strategies, developers could then implement their own or install other Error- and Exception handler middleware(s).

Everyone would benefit by including only necessary middleware, potentially reducing the core framework's size and enhancing flexibility.

@fulldecent
Copy link

Sharing some practical workarounds. Here is our approach for setting up the app relying on our own out-of-band error reporting. Hopefully this anecdote may help any design decisions on Slim v5 with respect to error handling.

public/index.php

<?php
use Psr\Http\Message\ResponseInterface as Response;
use Psr\Http\Message\ServerRequestInterface as Request;
use Slim\Factory\AppFactory;
use App\Controllers;

require __DIR__ . '/../vendor/autoload.php';

// ℹ️ We use this container for useful things inside the route implementations
$container = require __DIR__ . '/../config/bootstrap.php';
AppFactory::setContainer($container);
$app = AppFactory::create();

// Route documentation: https://www.slimframework.com/docs/v4/objects/routing.html
$app->get('/', function (Request $request, Response $response, $args) {
    $response->getBody()->write("Hello world!");
    return $response;
});
// Route definitions...
$app->post('/api/xyz-webhook', Controllers\XYZWebhookController::class);

// Normal run
$app->run();

config/bootstrap.php

<?php

require __DIR__ . '/../vendor/autoload.php';

// Application configuration secrets ///////////////////////////////////////////////////////////////////////////////////
$dotenv = \Dotenv\Dotenv::createImmutable(__DIR__ . '/..');
$dotenv->load();

// Setup error handling (email to Will) ////////////////////////////////////////////////////////////////////////////////
error_reporting(-1);
ini_set('display_errors', TRUE);
ini_set('display_startup_errors', TRUE);
ini_set('log_errors', TRUE);
ini_set('html_errors', 'On');

function emailProductionIssueToWill($issueText)
{
  $mail = new \PHPMailer\PHPMailer\PHPMailer(true);
  $mail->isSMTP();                                      // Set mailer to use SMTP
  $mail->Host = $_ENV['SMTP_HOST'];                     // Specify main and backup SMTP servers
  $mail->SMTPAuth = true;                               // Enable SMTP authentication
  $mail->Username = $_ENV['SMTP_USER'];                 // SMTP username
  $mail->Password = $_ENV['SMTP_PASS'];                 // SMTP password
  $mail->SMTPSecure = 'tls';                            // Enable TLS encryption, `ssl` also accepted
  $mail->Port = $_ENV['SMTP_PORT'];                     // TCP port to connect to
  $mail->CharSet = 'utf-8';                             // https://stackoverflow.com/a/39365798/300224
  $mail->From = $_ENV['SMTP_FROM'];
  $mail->FromName = $_ENV['SMTP_FROM_NAME'];
  $mail->addAddress($_ENV['SMTP_TO'], 'Will');
  $mail->Subject = 'PRODUCTION ISSUE';
  $mail->Body = $issueText;
  $mail->addCustomHeader('X-MC-Tags', 'production_issue');
  $result = $mail->send();
  if (!$result) {
    echo 'Error mail error';
  }
}

set_error_handler(function ($number, $message, $file, $line, $vars = null) {
  emailProductionIssueToWill("$file:$line\n$message");
  // https://www.php.net/manual/en/function.set-error-handler.php
  // ... it is your responsibility to die() if necessary. If the error-handler function returns, script execution will continue with the next statement after the one that caused an error.
  echo 'PAGE ERROR, logged';
  exit(1);
}, E_ALL); 

set_exception_handler(function ($exception) {
  // If exception is type VisitorVisibleException, show the message to the visitor.
  if ($exception instanceof VisitorVisibleException) {
    echo '<p style="font-weight:bold;font-color:red">ERROR: ' . $exception->getMessage() . '</p>';
  }
  emailProductionIssueToWill($exception->getFile() . ':' . $exception->getLine() . "\n" . $exception->getMessage());
});

// Setup container /////////////////////////////////////////////////////////////////////////////////////////////////////
$container = new App\Container(
  getDatabase: function (App\Container $container): App\Database {
    return new App\Database($_ENV['DB_DSN'], $_ENV['DB_USER'], $_ENV['DB_PASS']);
  },
  getCache: function (App\Container $container): App\Cache {
    return new App\Cache($container->getDatabase());
  },
  getLogger: function (App\Container $container): App\Logger {
    return new App\Logger($container->getDatabase());
  },
  getShopify: function (App\Container $container): App\Shopify {
    return new App\Shopify($_ENV['SHOPIFY_SHOP'], $_ENV['SHOPIFY_ADMIN_API_ACCESS_TOKEN']);
  }
);

// Return globals //////////////////////////////////////////////////////////////////////////////////////////////////////
return $container;

Discussion

This approach works well to inject a container with lazy load dependencies/features. This is also how the application secrets are loaded into those features.

The error handling is done out-of-band in the bootstrap file. This means it directly uses the secrets. We have errors getting sent to email and a messaging service.

But this approach fails from missing useful phpslim integration. For example, if there is a 404 routing error, we won't know what is the URL.

<?php
try {
    $app->run();
} catch (HttpNotFoundException $e) {
    // Rethrow Exception with message: Not found: URL
    throw new Exception( /* HOW TO GET URL HERE??? */);
}

@odan
Copy link
Contributor

odan commented Jul 20, 2024

@fulldecent As long it's a Slim http specific exception, you can use the $exception->getRequest() method.

$uri = $exception->getRequest()->getUri();
use Slim\Exception\HttpException;
// ...

if ($exception instanceof HttpException) {
    $uri = $exception->getRequest()->getUri();
}

@plasid
Copy link
Author

plasid commented Jul 20, 2024

I'm just using Symfony Error Handler. I set it up on my public/index.php right before anything else Slim related.
Works well and also get the nice debug page. Only thing I added was a small piece of code to show errors as JSON when the request was XHR.
No need to wrap anything with try/catch, because the Error Handler was instantiated before the Slim app runs it will handle all errors and exceptions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants