satispress icon indicating copy to clipboard operation
satispress copied to clipboard

Fatal error / WordPress crash when a 404 is expected.

Open rmpel opened this issue 2 years ago • 1 comments

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)

rmpel avatar Jan 29 '24 09:01 rmpel

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

BrianHenryIE avatar Jan 29 '24 16:01 BrianHenryIE

@rmpel As @BrianHenryIE mentioned, this is by design when WP_DEBUG is enabled. Let me know if it doesn't work as expected when WP_DEBUG is disabled or if there's a reason it should be changed.

bradyvercher avatar Oct 09 '24 20:10 bradyvercher

Sorry, I seem to have missed reply notifications, and due to workload haven't given this more thought, so I completely missed the reason.

I think I will try to find a way to either make SatisPress not know we are in WP_DEBUG (we keep loggin enabled to monitor site health) or live with the fact that a 404 results in a crash.

( And looking at the code, no filters in is_debug_mode, means I/we have to live with it ;P )

Thanks for the replies!

rmpel avatar Oct 10 '24 06:10 rmpel

@rmpel I went ahead and added a filter in 77b8a1f50c2ef8f0a6b8baadcc5764128cd064e5 to override debug mode for that request handler. That'll be included in the next release.

bradyvercher avatar Oct 10 '24 17:10 bradyvercher