cedaro / satispress

Expose installed WordPress plugins and themes as Composer packages.
500 stars 48 forks source link

Fatal error / WordPress crash when a 404 is expected. #211

Open rmpel opened 5 months ago

rmpel commented 5 months ago

The code says:

// Send a 404 response if the package doesn't exist.
if ( ! $package instanceof Package ) {
    throw HttpException::forUnknownPackage( $slug );
}
public static function forUnknownPackage(
    string $slug,
    int $code = 0,
    Throwable $previous = null
    ): HttpException {
    $message = "Package does not exist; Package: {$slug}";
    return new static( $message, HTTP::NOT_FOUND, $code, $previous );
}

Which suggest a 404 should be sent, however, it is an uncaught exception, resulting in a 500 Internal Server Error

[29-Jan-2024 09:38:34 UTC] PHP Fatal error:  Uncaught SatisPress\Exception\HttpException: Package does not exist; Package: composer.json in .../plugins/satispress/src/Exception/HttpException.php:91
Stack trace:
#0 .../plugins/satispress/src/Route/Download.php(110): SatisPress\Exception\HttpException::forUnknownPackage()
#1 .../plugins/satispress/src/Provider/RequestHandler.php(90): SatisPress\Route\Download->handle()
...
#10 {main}
  thrown in .../plugins/satispress/src/Exception/HttpException.php on line 91

Other than a total refactor, I have no solution for this. (I never used throwable exceptions to generate an HTTP status, so I wouldn't know where to start. I would simply do a header("HTTP/1.0 404 Not Found", true, 404); exit; but as said, that would require a total refactor, not just this instance)

BrianHenryIE commented 5 months ago

I think this is by design – in RequestHandler::dispatch(), when WP_DEBUG is enabled exceptions are thrown rather than handled:

https://github.com/cedaro/satispress/blob/33eaa99a54fa326af00bfcb932c1a55efb7ded49/src/Provider/RequestHandler.php#L95-L97