feat: custom exception handler

[kenjis]

Description
Supersedes #5675, #6710

Allows developers to easily create and integrate their own Exception handler classes,
and limiting their usage to only the exception or HTTP status code desired.

• add ExceptionHandlerInterface, BaseExceptionHandler, and ExceptionHandler
• add Config\Exceptions::handler() to define Exception handler

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[iRedds]

debug is debug. A custom exception handler is not a debug.
No need to wrap it in this namespace. It's about something else.

[kenjis]

That may indeed be true.

I have rebased the previous PR to make it mergeable. However, I don't remember what's in it anymore and need to remember what I was trying to do before.

[kenjis]

Reworked.

[iRedds]

In addition to what has already been said, I will criticize the implementation.

1. HTTP 404 should not be handled in exceptions. The handler already exists in the CodeIgniter class.
   If the route is defined, but no controller or method is found, it's not a 404, but a 503. As in any other case, when a class or method is not found.

2. I don't think it's correct that the Config\Exceptions::handler() method returns an ExceptionHandler instance by default.
   It's an easy way to shoot yourself in the foot. In my opinion, an instance of ExceptionHandler should be created if the Config\Exceptions::handler method is not defined or returns nothing.
   It will also save 100500 changes to the application configs just because of adding functionality.

3. Custom exceptions are needed to form the correct response. That is, to return, for example, HTTP 200 instead of debugging data or a stub on the production server.
   I want to say that in this case the methods from BaseExceptionHandler (by the way, maybe we will stop calling the classes Base) are not needed in the principle.

4. BaseExceptionHandler::render() method. According to the name, it should render the result, but not output it. And in general it can be replaced with view('template');

5. The current implementation ignores logging.

6. It seems to me that it would be more correct for exception handlers to return a Responce object.

[iRedds]

And by the way, when I suggested my implementation of a custom handler, @lonnieezell said that it's not good to override the behavior of framework exceptions, such as PageNotFoundException.

[kenjis]

If the route is defined, but no controller or method is found, it's not a 404, but a 503. As in any other case, when a class or method is not found.

Why 503? Isn't it 501?
And is it relevant to this PR?

[kenjis]

In my opinion, an instance of ExceptionHandler should be created if the Config\Exceptions::handler method is not defined or returns nothing.

I don't see why the ExceptionHandler should be instantiated if the Config\Exceptions::handler() is defined.

In this PR if the Config\Exceptions::handler() is not defined, the ExceptionHandler will not be instantiated.
So the existing apps do not need to define Config\Exceptions::handler().

[kenjis]

Custom exceptions are needed to form the correct response. That is, to return, for example, HTTP 200 instead of debugging data or a stub on the production server.

A 200 Response is not exceptional. So Exception Handler is not related.

[kenjis]

I want to say that in this case the methods from BaseExceptionHandler (by the way, maybe we will stop calling the classes Base) are not needed in the principle.

You don't need to use BaseExceptionHandler. It just provides some methods.

[kenjis]

The current implementation ignores logging.

You are correct. Thank you.
Fixed to log before calling ExceptionHandler::handle().

[kenjis]

It seems to me that it would be more correct for exception handlers to return a Responce object.

I'm not sure. But exception handlers might not return (HTTP) Response.

[iRedds]

If the route is defined, but no controller or method is found, it's not a 404, but a 503. As in any other case, when a class or method is not found.

Why 503? Isn't it 501? And is it relevant to this PR?

This was for an example. That is, in my opinion, if there is no controller or method, it should not return 404.

This does not apply to PR. I just had to say it, because for some reason PageNotFoundException is first caught in its handler, and then thrown out again, but processed already in debug.

[iRedds]

In my opinion, an instance of ExceptionHandler should be created if the Config\Exceptions::handler method is not defined or returns nothing.

I don't see why the ExceptionHandler should be instantiated if the Config\Exceptions::handler() is defined.

In this PR if the Config\Exceptions::handler() is not defined, the ExceptionHandler will not be instantiated. So the existing apps do not need to define Config\Exceptions::handler().

Then can you explain why you need ExceptionHandler, if everything should work without it?
I assumed this would be the default exception handler when no custom one is set.
Especially since you added it to the config.

public function handler(int $statusCode, Throwable $exception): ExceptionHandlerInterface
{
    return new ExceptionHandler($this);
}

[iRedds]

Custom exceptions are needed to form the correct response. That is, to return, for example, HTTP 200 instead of debugging data or a stub on the production server.

A 200 Response is not exceptional. So Exception Handler is not related.

You need to look deeper into a tool like exceptions.
Redirect and HTTP 404 are not exceptions either, but the framework handles exceptions that generate responses with the appropriate codes.

For example, I want to do a centralized processing of my exception and I want the answer to be exactly 200. Or 201 or some other response code for me.

Let me take Laravel as an example.
If I'm not mistaken, the Model::findOrFail() method, if the entry is not found, will throw an exception, which will be handled as 404 ending the program execution.

That is, you do not need checks in the controller.

[iRedds]

I want to say that in this case the methods from BaseExceptionHandler (by the way, maybe we will stop calling the classes Base) are not needed in the principle.

You don't need to use BaseExceptionHandler. It just provides some methods.

Then I have doubts about the need for this class.
It might be better to implement it as a trait.

[iRedds]

I'm not sure. But exception handlers might not return (HTTP) Response.

This is to avoid avoiding behavior like the CodeIgniter class, where $response->send() is called in many places instead of just one.
If we send the response from one place, it will be easier to control the behavior.

For example, this code is duplicated in CodeIgniter\Debug\Exceptions and CodeIgniter\Debug\ExceptionHandler

CodeIgniter4/system/Debug/Exceptions.php

Lines 154 to 170 in d2d65cd

	try {
	$this->response->setStatusCode($statusCode);
	} catch (HTTPException $e) {
	// Workaround for invalid HTTP status code.
	$statusCode = 500;
	$this->response->setStatusCode($statusCode);
	}
	if (! headers_sent()) {
	header(sprintf('HTTP/%s %s %s', $this->request->getProtocolVersion(), $this->response->getStatusCode(), $this->response->getReasonPhrase()), true, $statusCode);
	}
	if (strpos($this->request->getHeaderLine('accept'), 'text/html') === false) {
	$this->respond(ENVIRONMENT === 'development' ? $this->collectVars($exception, $statusCode) : '', $statusCode)->send();
	exit($exitCode);
	}

[iRedds]

Also I would like this PR to (if possible) add the ability to use exceptions without registration.

As I suggested on the forum. https://forum.codeigniter.com/showthread.php?tid=86567

[kenjis]

Then can you explain why you need ExceptionHandler, if everything should work without it?

Everything works without it, because we keep BC.
The current code (which should be removed in the future) does everything as before.

I assumed this would be the default exception handler when no custom one is set.
Especially since you added it to the config.

Yes.

[kenjis]

You need to look deeper into a tool like exceptions.
Redirect and HTTP 404 are not exceptions either, but the framework handles exceptions that generate responses with the appropriate codes.

No, they are not real exceptions. But the framework already takes care of them.
What does this have to do with this PR?

For example, I want to do a centralized processing of my exception and I want the answer to be exactly 200. Or 201 or some other response code for me.

2xx is normal response, not an exception.

[kenjis]

If we send the response from one place, it will be easier to control the behavior.

I agree, but it seems not the scope of this PR.

We cannot solve many problems at once while keeping backward compatibility.
Please send PRs one at a time if necessary.
This PR allows for the definition of custom exception handlers.

[iRedds]

Everything works without it, because we keep BC. The current code (which should be removed in the future) does everything as before.

Then I do not understand what prevents us from using the default exception handler right now.

Something like this

if () {  // trying to get a custom handler.
// $handler = 
} else {
$handler = new ExceptionHandler();
}

Given that the ExceptionHandler class implements the current functionality, nothing will change.
BC will not be here, since the result will not change.

[kenjis]

The following implementation is more breaking than this PR.
Because ExceptionHandler is a new class, and not the same as Exceptions.

if () {  // trying to get a custom handler.
// $handler = 
} else {
$handler = new ExceptionHandler();
}

If a dev overrides Exceptions::render(), it breaks.

[iRedds]

Yes, redirects are normal responses, not exceptional. Most cases of 4xx are also expected, not exceptional.

If 3xx and 4xx are the same normal responses as 2xx, then why can the first ones be processed through exceptions (and 4xx is processed twice), but 2xx cannot?

[kenjis]

If 3xx and 4xx are the same normal responses as 2xx, then why can the first ones be processed through exceptions (and 4xx is processed twice), but 2xx cannot?

It is not an exceptional process, so we should not use exceptions. It would be cleaner to remove them all.
However, deleting what is already there is not possible because it is a braking change.

[MGatner]

@kenjis can you summarize what changed from Lonnie's PR? Does this need a full re-review?

[kenjis]

I honestly don't remember exactly, but I think #6710 was just an update of Lonnie's PR.
This PR is basically the same, but keeps more compatibility than that.

Changes:

• add an interface
• use the current code in exceptionHandler() when custom handler is not used
  • 6710: https://github.com/codeigniter4/CodeIgniter4/pull/6710/files#diff-560b4518cdc530a79ebd6846a0c5e694d567418be0db700a3ea533a0d3960c5aL125
  • This: https://github.com/codeigniter4/CodeIgniter4/pull/7087/files#diff-560b4518cdc530a79ebd6846a0c5e694d567418be0db700a3ea533a0d3960c5aR152

The difference between #6710 and this (including the changes in 4.4 branch):

$ git diff feat-ExceptionHandler..HEAD system/Debug/
diff --git a/system/Debug/ExceptionHandler.php b/system/Debug/ExceptionHandler.php
index 5428f38994..1eaa05acee 100644
--- a/system/Debug/ExceptionHandler.php
+++ b/system/Debug/ExceptionHandler.php
@@ -20,7 +20,7 @@ use CodeIgniter\HTTP\ResponseInterface;
 use Config\Paths;
 use Throwable;
 
-final class ExceptionHandler extends BaseExceptionHandler
+final class ExceptionHandler extends BaseExceptionHandler implements ExceptionHandlerInterface
 {
     use ResponseTrait;
 
@@ -36,8 +36,6 @@ final class ExceptionHandler extends BaseExceptionHandler
 
     /**
      * Determines the correct way to display the error.
-     *
-     * @return void
      */
     public function handle(
         Throwable $exception,
@@ -45,7 +43,7 @@ final class ExceptionHandler extends BaseExceptionHandler
         ResponseInterface $response,
         int $statusCode,
         int $exitCode
-    ) {
+    ): void {
         // ResponseTrait needs these properties.
         $this->request  = $request;
         $this->response = $response;
diff --git a/system/Debug/ExceptionHandlerInterface.php b/system/Debug/ExceptionHandlerInterface.php
new file mode 100644
index 0000000000..bbfcb6ba70
--- /dev/null
+++ b/system/Debug/ExceptionHandlerInterface.php
@@ -0,0 +1,30 @@
+<?php
+
+/**
+ * This file is part of CodeIgniter 4 framework.
+ *
+ * (c) CodeIgniter Foundation <admin@codeigniter.com>
+ *
+ * For the full copyright and license information, please view
+ * the LICENSE file that was distributed with this source code.
+ */
+
+namespace CodeIgniter\Debug;
+
+use CodeIgniter\HTTP\RequestInterface;
+use CodeIgniter\HTTP\ResponseInterface;
+use Throwable;
+
+interface ExceptionHandlerInterface
+{
+    /**
+     * Determines the correct way to display the error.
+     */
+    public function handle(
+        Throwable $exception,
+        RequestInterface $request,
+        ResponseInterface $response,
+        int $statusCode,
+        int $exitCode
+    ): void;
+}
diff --git a/system/Debug/Exceptions.php b/system/Debug/Exceptions.php
index 0479e64154..f77e313e4d 100644
--- a/system/Debug/Exceptions.php
+++ b/system/Debug/Exceptions.php
@@ -15,10 +15,12 @@ use CodeIgniter\API\ResponseTrait;
 use CodeIgniter\Exceptions\HasExitCodeInterface;
 use CodeIgniter\Exceptions\HTTPExceptionInterface;
 use CodeIgniter\Exceptions\PageNotFoundException;
+use CodeIgniter\HTTP\Exceptions\HTTPException;
 use CodeIgniter\HTTP\RequestInterface;
 use CodeIgniter\HTTP\ResponseInterface;
 use Config\Exceptions as ExceptionsConfig;
 use Config\Paths;
+use Config\Services;
 use ErrorException;
 use Psr\Log\LogLevel;
 use Throwable;
@@ -59,7 +61,7 @@ class Exceptions
     /**
      * The request.
      *
-     * @var RequestInterface
+     * @var RequestInterface|null
      */
     protected $request;
 
@@ -70,25 +72,29 @@ class Exceptions
      */
     protected $response;
 
+    private ?Throwable $exceptionCaughtByExceptionHandler = null;
+
     /**
-     * @param RequestInterface $request
+     * @param RequestInterface|null $request
+     *
+     * @deprecated The parameter $request and $response are deprecated. No longer used.
      */
-    public function __construct(ExceptionsConfig $config, $request, ResponseInterface $response)
+    public function __construct(ExceptionsConfig $config, $request, ResponseInterface $response) /** @phpstan-ignore-line */
     {
         // For backward compatibility
         $this->ob_level = ob_get_level();
         $this->viewPath = rtrim($config->errorViewPath, '\\/ ') . DIRECTORY_SEPARATOR;
 
-        $this->config   = $config;
-        $this->request  = $request;
-        $this->response = $response;
+        $this->config = $config;
 
         // workaround for upgraded users
+        // This causes "Deprecated: Creation of dynamic property" in PHP 8.2.
+        // @TODO remove this after dropping PHP 8.1 support.
         if (! isset($this->config->sensitiveDataInTrace)) {
             $this->config->sensitiveDataInTrace = [];
         }
-        if (! isset($this->config->logDeprecationsOnly, $this->config->deprecationLogLevel)) {
-            $this->config->logDeprecationsOnly = false;
+        if (! isset($this->config->logDeprecations, $this->config->deprecationLogLevel)) {
+            $this->config->logDeprecations     = false;
             $this->config->deprecationLogLevel = LogLevel::WARNING;
         }
     }
@@ -113,6 +119,8 @@ class Exceptions
      */
     public function exceptionHandler(Throwable $exception)
     {
+        $this->exceptionCaughtByExceptionHandler = $exception;
+
         [$statusCode, $exitCode] = $this->determineCodes($exception);
 
         if ($this->config->log === true && ! in_array($statusCode, $this->config->ignoreCodes, true)) {
@@ -124,14 +132,47 @@ class Exceptions
             ]);
         }
 
-        // For upgraded users who did not update the config file.
-        if (! method_exists($this->config, 'handler')) {
-            $handler = new ExceptionHandler($this->config);
-        } else {
+        $this->request  = Services::request();
+        $this->response = Services::response();
+
+        if (method_exists($this->config, 'handler')) {
+            // Use new ExceptionHandler
             $handler = $this->config->handler($statusCode, $exception);
+            $handler->handle(
+                $exception,
+                $this->request,
+                $this->response,
+                $statusCode,
+                $exitCode
+            );
+
+            return;
         }
 
-        $handler->handle($exception, $this->request, $this->response, $statusCode, $exitCode);
+        // For backward compatibility
+        if (! is_cli()) {
+            try {
+                $this->response->setStatusCode($statusCode);
+            } catch (HTTPException $e) {
+                // Workaround for invalid HTTP status code.
+                $statusCode = 500;
+                $this->response->setStatusCode($statusCode);
+            }
+
+            if (! headers_sent()) {
+                header(sprintf('HTTP/%s %s %s', $this->request->getProtocolVersion(), $this->response->getStatusCode(), $this->response->getReasonPhrase()), true, $statusCode);
+            }
+
+            if (strpos($this->request->getHeaderLine('accept'), 'text/html') === false) {
+                $this->respond(ENVIRONMENT === 'development' ? $this->collectVars($exception, $statusCode) : '', $statusCode)->send();
+
+                exit($exitCode);
+            }
+        }
+
+        $this->render($exception, $statusCode);
+
+        exit($exitCode);
     }
 
     /**
@@ -145,7 +186,11 @@ class Exceptions
      */
     public function errorHandler(int $severity, string $message, ?string $file = null, ?int $line = null)
     {
-        if ($this->isDeprecationError($severity) && $this->config->logDeprecationsOnly) {
+        if ($this->isDeprecationError($severity)) {
+            if (! $this->config->logDeprecations || (bool) env('CODEIGNITER_SCREAM_DEPRECATIONS')) {
+                throw new ErrorException($message, 0, $severity, $file, $line);
+            }
+
             return $this->handleDeprecationError($message, $file, $line);
         }
 
@@ -172,6 +217,13 @@ class Exceptions
 
         ['type' => $type, 'message' => $message, 'file' => $file, 'line' => $line] = $error;
 
+        if ($this->exceptionCaughtByExceptionHandler) {
+            $message .= "\n【Previous Exception】\n"
+                . get_class($this->exceptionCaughtByExceptionHandler) . "\n"
+                . $this->exceptionCaughtByExceptionHandler->getMessage() . "\n"
+                . $this->exceptionCaughtByExceptionHandler->getTraceAsString();
+        }
+
         if (in_array($type, [E_ERROR, E_CORE_ERROR, E_COMPILE_ERROR, E_PARSE], true)) {
             $this->exceptionHandler(new ErrorException($message, 0, $type, $file, $line));
         }
@@ -338,8 +390,6 @@ class Exceptions
     }
 
     /**
-     * @noRector \Rector\DeadCode\Rector\ClassMethod\RemoveUselessReturnTagRector
-     *
      * @return true
      */
     private function handleDeprecationError(string $message, ?string $file = null, ?int $line = null): bool
@@ -515,9 +565,6 @@ class Exceptions
                     case is_resource($value):
                         return sprintf('resource (%s)', get_resource_type($value));
 
-                    case is_string($value):
-                        return var_export(clean_path($value), true);
-
                     default:
                         return var_export($value, true);
                 }

[kenjis]

Does this need a full re-review?

No. The code structure is not changed since the Lonnie's PR.
The review point is the upgrade guide is sufficient or not.
In other words, whether there are any breaking changes other than documented.

[kenjis]

Rebased to resolve conflict.

[kenjis]

I think this PR is ready to merge.
@codeigniter4/core-team Can someone review?

[MGatner]

I'll have to come back to this, it's a big one.

[kenjis]

Rebased to resolve conflict.
Can someone review?