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

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

Closed
rmpel opened this issue Jan 29, 2024 · 4 comments
Closed

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

rmpel opened this issue Jan 29, 2024 · 4 comments

Comments

@rmpel
Copy link

rmpel commented Jan 29, 2024

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
Copy link
Contributor

BrianHenryIE commented Jan 29, 2024

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

if ( $this->is_debug_mode() && ! $e instanceof AuthenticationException ) {
throw $e;
}

@bradyvercher
Copy link
Member

@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.

@rmpel
Copy link
Author

rmpel commented Oct 10, 2024

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!

@bradyvercher
Copy link
Member

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

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

No branches or pull requests

3 participants