From 7e70d2cca0c8ebc467399f0bb95d1271c0f7de84 Mon Sep 17 00:00:00 2001 From: Arman <407448+armanist@users.noreply.github.com> Date: Fri, 1 May 2026 16:23:19 +0400 Subject: [PATCH 1/5] [#490] Refactor error handler into focused tracer components --- docs/error-handler-refactor-ticket-draft.md | 76 +++++++ docs/unify-errorhandler-web-flow.md | 185 ++++++++++++++++ src/Tracer/ErrorHandler.php | 154 +++----------- src/Tracer/ExceptionSeverityResolver.php | 62 ++++++ src/Tracer/StackTraceFormatter.php | 83 ++++++++ src/Tracer/WebExceptionRenderer.php | 50 +++++ tests/Unit/Tracer/ErrorHandlerTest.php | 199 +++++------------- .../Tracer/ExceptionSeverityResolverTest.php | 42 ++++ tests/Unit/Tracer/StackTraceFormatterTest.php | 137 ++++++++++++ .../Unit/Tracer/WebExceptionRendererTest.php | 31 +++ 10 files changed, 744 insertions(+), 275 deletions(-) create mode 100644 docs/error-handler-refactor-ticket-draft.md create mode 100644 docs/unify-errorhandler-web-flow.md create mode 100644 src/Tracer/ExceptionSeverityResolver.php create mode 100644 src/Tracer/StackTraceFormatter.php create mode 100644 src/Tracer/WebExceptionRenderer.php create mode 100644 tests/Unit/Tracer/ExceptionSeverityResolverTest.php create mode 100644 tests/Unit/Tracer/StackTraceFormatterTest.php create mode 100644 tests/Unit/Tracer/WebExceptionRendererTest.php diff --git a/docs/error-handler-refactor-ticket-draft.md b/docs/error-handler-refactor-ticket-draft.md new file mode 100644 index 000000000..b37884ee1 --- /dev/null +++ b/docs/error-handler-refactor-ticket-draft.md @@ -0,0 +1,76 @@ +## Title +Refactor `Tracer\ErrorHandler` for separation of concerns and safer fallback behavior + +## Background +`src/Tracer/ErrorHandler.php` currently handles multiple concerns in one class: +- CLI/web branching +- error page rendering +- stack trace source extraction/formatting +- severity mapping +- logging decisions +- direct web response send + +The current behavior works, but the class is hard to test and harder to evolve safely. + +## Goal +Refactor `ErrorHandler` to improve maintainability and testability without changing external behavior. + +## Scope +- Keep global handler registration flow unchanged (`SetupErrorHandlerStage`). +- Keep current user-visible behavior for CLI and web errors. +- Internally split logic into focused collaborators. + +## Proposed Changes +1. Extract `ExceptionSeverityResolver` +- Responsibility: map `Throwable` to error/log severity. +- Moves logic from `getErrorType()`. + +2. Extract `StackTraceFormatter` (or `StackTraceComposer`) +- Responsibility: build trace payload + source snippets for debug views. +- Moves logic from `composeStackTrace()`, `getSourceCode()`, `formatLineItem()`. + +3. Extract `WebExceptionRenderer` +- Responsibility: produce web error content for debug/production. +- Uses view rendering and formatter. +- Handles fallback minimal output if rendering fails. + +4. Keep `ErrorHandler` as orchestrator +- Responsibility: +- route CLI vs web path +- invoke renderer/resolver +- perform logging +- send final web response + +5. Add robust fallback behavior +- If web rendering fails inside error handling, fallback to plain text 500 response. +- Prevent recursive/secondary failure loops. + +## Non-Goals +- No change to routing/middleware/web adapter flow. +- No switch to adapter-level exception handling in this ticket. +- No behavioral change to current error templates/content (except emergency fallback path). + +## Acceptance Criteria +- Existing `ErrorHandler` behavior remains equivalent for: +- CLI exception output +- debug-mode web exception page +- production-mode web 500 page + logging +- New unit tests cover extracted components and fallback path. +- `ErrorHandler` unit tests are simplified and focused on orchestration. +- No regression in existing test suite. + +## Suggested Test Plan +- Unit: severity resolver mappings (`ErrorException`, `ParseError`, `ReflectionException`, generic `Throwable`). +- Unit: stack trace formatter with local filesystem adapter and non-local adapter. +- Unit: renderer in debug and production modes. +- Unit: fallback output when renderer throws. +- Integration-like unit: `ErrorHandler` orchestrates CLI/web and logging calls correctly. + +## Risks / Notes +- Refactor touches exception path; fallback behavior must be carefully tested. +- Keep constructor and wiring compatible with current DI/bootstrap usage. + +## Estimated Change Set +- `src/Tracer/ErrorHandler.php` (reduced orchestrator) +- new classes under `src/Tracer/` (or `src/Tracer/*`) +- test updates/additions under `tests/Unit/Tracer/` diff --git a/docs/unify-errorhandler-web-flow.md b/docs/unify-errorhandler-web-flow.md new file mode 100644 index 000000000..f5091d69b --- /dev/null +++ b/docs/unify-errorhandler-web-flow.md @@ -0,0 +1,185 @@ +# Unify ErrorHandler With Web Response Flow + +## Background +The web request/response pipeline now expects handlers to return a `Response`, with a shared send path that applies cross-cutting behavior (for example, CORS headers) before `send()`. + +Current exception handling in `Tracer\ErrorHandler` bypasses that pipeline by rendering and sending directly. This creates a second response-send path with duplicated behavior and a risk of drift. + +## Current Gap +- Normal flow: route/controller returns `Response` -> web adapter sends through shared path. +- Error flow: global exception handler builds output and calls `response()->send()` directly. + +Result: exception responses are not guaranteed to go through the same emit path as normal web responses. + +## Goal +Adopt one shared response emission path for: +- success responses +- 404/not-found responses +- uncaught exception responses + +while preserving global exception handling as a safety net. + +## What Is An Emitter? +An emitter is a very small class whose only job is to send a prepared `Response` to the client. + +In this codebase, "send a response" is not just `->send()`. We also need shared behavior (like CORS headers) right before send. That means response emission is a policy, not a one-liner. + +So an emitter gives us one place for that policy. + +## Why We Need It Here +Right now we have two web send paths: + +1. Normal request path: +- `WebAppAdapter` -> `WebAppTrait::sendResponse()` -> apply CORS -> `send()` + +2. Exception path: +- `ErrorHandler::handleWebException()` -> `response()->html(...)` -> `response()->send()` + +Issue: +- exception responses skip shared web send behavior (CORS and future headers/middleware-like concerns) +- behavior can drift over time because logic is duplicated + +Emitter fixes this by making both paths call the same send component. + +## Minimal Design + +### 1) New shared emitter +```php +has('cors')) { + config()->import(new Setup('config', 'cors')); + } + + foreach (config()->get('cors') as $key => $value) { + $response->setHeader($key, (string) $value); + } + + $response->send(); + } +} +``` + +### 2) Use emitter in normal flow (WebAppTrait) +Before: +```php +private function sendResponse(Response $response): void +{ + $this->handleCors($response); + $response->send(); +} +``` + +After: +```php +private function sendResponse(Response $response): void +{ + $emitter = Di::get(WebResponseEmitter::class); + $emitter->emit($response); +} +``` + +### 3) Use emitter in ErrorHandler web exception path +Before: +```php +response()->html($errorPage); +response()->send(); +``` + +After: +```php +$response = response()->html($errorPage); +$emitter = Di::get(WebResponseEmitter::class); +$emitter->emit($response); +``` + +## Important Clarification +Unifying flow does not mean "run exception responses through `WebAppAdapter::start()` again". + +When `set_exception_handler` runs, the original flow has already failed. The correct unification point is response emission policy (headers + send), not full request dispatch. + +## Proposed Approach + +### Phase 1: Extract exception-to-response mapping +Create a dedicated service (example: `ExceptionResponseFactory`) that converts `Throwable` -> `Response`. + +Responsibilities moved from `ErrorHandler`: +- debug/production error-page selection +- stack trace payload composition for debug view +- error type/severity mapping +- production logging decision + +### Phase 2: Centralize response emission +Create a reusable `ResponseEmitter` (or equivalent) used by all web response exits. + +Responsibilities: +- apply shared response concerns (existing CORS behavior) +- call `send()` exactly once + +Refactor callers: +- `WebAppAdapter` (normal path) uses emitter +- `ErrorHandler` web exception path uses emitter + +### Phase 3 (Optional): True single control-flow entry +Wrap top-level web app startup in `try/catch` and route caught exceptions through: +- `ExceptionResponseFactory` +- `ResponseEmitter` + +Keep `set_exception_handler` as fallback for fatal/unexpected contexts. + +## Scope +In-scope: +- `src/Tracer/ErrorHandler.php` +- web response emission path used by `WebAppAdapter` +- new service(s) for exception-response mapping and emission +- related unit tests + +Out-of-scope (for initial ticket): +- changing HTTP contract of controllers/routes +- broad boot pipeline redesign + +## Testing Plan +- Update `ErrorHandler` tests to validate delegation to new shared services. +- Add focused unit tests for `ExceptionResponseFactory`: + - debug vs production rendering + - severity mapping + - logging behavior +- Add unit tests for `ResponseEmitter`: + - shared headers applied + - single send behavior +- Keep/extend adapter tests to confirm exception responses still render and send correctly. + +## Risks and Mitigations +- Risk: double-send/partial output in exceptional paths. + - Mitigation: guard emitter usage and add explicit tests for send behavior. +- Risk: behavior changes in testing/CLI environment. + - Mitigation: isolate CLI exception path, keep existing behavior intact. +- Risk: hidden coupling from helper globals. + - Mitigation: move logic into injectable services and test boundaries directly. + +## Acceptance Criteria +- Uncaught web exceptions no longer call `response()->send()` via custom inline flow in `ErrorHandler`. +- Exception responses pass through the same shared emitter path as normal web responses. +- Existing behavior remains functionally equivalent (debug trace page, production 500 page, logging). +- Validation passes: + - `vendor/bin/phpunit --coverage-clover coverage.xml` + - `composer phpstan` + - `composer cs:check` + +## Suggested Ticket Title +Unify `Tracer\\ErrorHandler` exception response handling with shared web response pipeline diff --git a/src/Tracer/ErrorHandler.php b/src/Tracer/ErrorHandler.php index 08fdf590f..6cc884922 100644 --- a/src/Tracer/ErrorHandler.php +++ b/src/Tracer/ErrorHandler.php @@ -16,19 +16,15 @@ namespace Quantum\Tracer; -use Quantum\Storage\Contracts\LocalFilesystemAdapterInterface; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Output\ConsoleOutput; use Quantum\Renderer\Exceptions\RendererException; use Quantum\Config\Exceptions\ConfigException; use Quantum\App\Exceptions\BaseException; -use Quantum\View\Factories\ViewFactory; use Quantum\Di\Exceptions\DiException; use Quantum\Logger\Logger; use ReflectionException; -use Psr\Log\LogLevel; use ErrorException; -use ParseError; use Throwable; use Exception; @@ -38,32 +34,20 @@ */ class ErrorHandler { - /** - * Number of lines to be returned - */ - public const NUM_LINES = 10; - - /** - * @var array - */ - public const ERROR_TYPES = [ - E_ERROR => 'error', - E_WARNING => 'warning', - E_PARSE => 'error', - E_NOTICE => 'notice', - E_CORE_ERROR => 'error', - E_CORE_WARNING => 'warning', - E_COMPILE_ERROR => 'error', - E_COMPILE_WARNING => 'warning', - E_USER_ERROR => 'error', - E_USER_WARNING => 'warning', - E_USER_NOTICE => 'notice', - E_STRICT => 'notice', - E_RECOVERABLE_ERROR => 'error', - ]; - private ?Logger $logger = null; private ?OutputInterface $cliOutput = null; + private ExceptionSeverityResolver $severityResolver; + private WebExceptionRenderer $webExceptionRenderer; + + public function __construct( + ?ExceptionSeverityResolver $severityResolver = null, + ?StackTraceFormatter $stackTraceFormatter = null, + ?WebExceptionRenderer $webExceptionRenderer = null + ) { + $this->severityResolver = $severityResolver ?? new ExceptionSeverityResolver(); + $formatter = $stackTraceFormatter ?? new StackTraceFormatter(); + $this->webExceptionRenderer = $webExceptionRenderer ?? new WebExceptionRenderer($formatter); + } private function __clone() { @@ -110,25 +94,31 @@ public function handleException(Throwable $throwable): void private function handleCliException(Throwable $throwable): void { $output = $this->cliOutput ?? new ConsoleOutput(); - $output->writeln('' . $throwable->getMessage() . ''); + + if (!is_debug_mode()) { + $output->writeln('' . $throwable->getMessage() . ''); + return; + } + + $output->writeln('' . get_class($throwable) . ': ' . $throwable->getMessage() . ''); + $output->writeln('In ' . $throwable->getFile() . ':' . $throwable->getLine()); + $output->writeln($throwable->getTraceAsString()); } /** - * @throws ConfigException|RendererException|DiException|BaseException|ReflectionException|Exception + * @throws DiException|BaseException|ReflectionException|Exception */ private function handleWebException(Throwable $throwable): void { - $view = ViewFactory::get(); $errorType = $this->getErrorType($throwable); - if (is_debug_mode()) { - $errorPage = $view->renderPartial('errors' . DS . 'trace', [ - 'stackTrace' => $this->composeStackTrace($throwable), - 'errorMessage' => $throwable->getMessage(), - 'severity' => ucfirst($errorType), - ]); - } else { - $errorPage = $view->renderPartial('errors' . DS . '500'); + try { + $errorPage = $this->webExceptionRenderer->render($throwable, $errorType); + } catch (Throwable $e) { + $errorPage = 'Internal Server Error'; + } + + if (!is_debug_mode()) { $this->logError($throwable, $errorType); } @@ -147,95 +137,11 @@ private function logError(Throwable $e, string $errorType): void $this->logger->$logMethod($e->getMessage(), ['trace' => $e->getTraceAsString()]); } - /** - * Composes the stack trace - * @return array - * @throws ConfigException|RendererException|DiException|BaseException|ReflectionException - */ - private function composeStackTrace(Throwable $e): array - { - $trace[] = [ - 'file' => $e->getFile(), - 'code' => $this->getSourceCode($e->getFile(), $e->getLine(), 'error-line'), - ]; - - foreach ($e->getTrace() as $item) { - if (($item['class'] ?? null) === self::class) { - continue; - } - - if (isset($item['file'])) { - $trace[] = [ - 'file' => $item['file'], - 'code' => $this->getSourceCode($item['file'], $item['line'] ?? 1, 'switch-line'), - ]; - } - } - - return $trace; - } - - /** - * Gets the source code where the error happens - * @throws ConfigException|RendererException|DiException|BaseException|ReflectionException - */ - private function getSourceCode(string $filename, int $lineNumber, string $className): string - { - $lineNumber--; - - $halfLines = intdiv(self::NUM_LINES, 2); - $start = max($lineNumber - $halfLines, 1); - - $adapter = fs()->getAdapter(); - - if (!$adapter instanceof LocalFilesystemAdapterInterface) { - return ''; - } - - $lines = $adapter->getLines($filename, $start, self::NUM_LINES); - - $code = '
    '; - - foreach ($lines as $currentLineNumber => $line) { - $code .= $this->formatLineItem($currentLineNumber, $line, $lineNumber, $className); - } - - return $code . '
'; - } - - /** - * Formats the line item - */ - private function formatLineItem(int $currentLineNumber, string $line, int $lineNumber, string $className): string - { - $highlightClass = $currentLineNumber === $lineNumber ? " class=\"{$className}\"" : ''; - - $encodedLine = htmlspecialchars($line, ENT_QUOTES); - - return sprintf( - '
%s
', - $highlightClass, - $encodedLine - ); - } - /** * Gets the error type based on the exception class */ private function getErrorType(Throwable $e): string { - if ($e instanceof ErrorException) { - return self::ERROR_TYPES[$e->getSeverity()] ?? LogLevel::ERROR; - } - - if ($e instanceof ParseError) { - return LogLevel::CRITICAL; - } - - if ($e instanceof ReflectionException) { - return LogLevel::WARNING; - } - - return LogLevel::ERROR; + return $this->severityResolver->resolve($e); } } diff --git a/src/Tracer/ExceptionSeverityResolver.php b/src/Tracer/ExceptionSeverityResolver.php new file mode 100644 index 000000000..1090272b0 --- /dev/null +++ b/src/Tracer/ExceptionSeverityResolver.php @@ -0,0 +1,62 @@ + + * @copyright Copyright (c) 2018 Softberg LLC (https://softberg.org) + * @link http://quantum.softberg.org/ + * @since 3.0.0 + */ + +namespace Quantum\Tracer; + +use ReflectionException; +use Psr\Log\LogLevel; +use ErrorException; +use ParseError; +use Throwable; + +final class ExceptionSeverityResolver +{ + /** + * @var array + */ + private const ERROR_TYPES = [ + E_ERROR => 'error', + E_WARNING => 'warning', + E_PARSE => 'error', + E_NOTICE => 'notice', + E_CORE_ERROR => 'error', + E_CORE_WARNING => 'warning', + E_COMPILE_ERROR => 'error', + E_COMPILE_WARNING => 'warning', + E_USER_ERROR => 'error', + E_USER_WARNING => 'warning', + E_USER_NOTICE => 'notice', + E_STRICT => 'notice', + E_RECOVERABLE_ERROR => 'error', + ]; + + public function resolve(Throwable $throwable): string + { + if ($throwable instanceof ErrorException) { + return self::ERROR_TYPES[$throwable->getSeverity()] ?? LogLevel::ERROR; + } + + if ($throwable instanceof ParseError) { + return LogLevel::CRITICAL; + } + + if ($throwable instanceof ReflectionException) { + return LogLevel::WARNING; + } + + return LogLevel::ERROR; + } +} diff --git a/src/Tracer/StackTraceFormatter.php b/src/Tracer/StackTraceFormatter.php new file mode 100644 index 000000000..e142e17c1 --- /dev/null +++ b/src/Tracer/StackTraceFormatter.php @@ -0,0 +1,83 @@ + + * @copyright Copyright (c) 2018 Softberg LLC (https://softberg.org) + * @link http://quantum.softberg.org/ + * @since 3.0.0 + */ + +namespace Quantum\Tracer; + +use Quantum\Storage\Contracts\LocalFilesystemAdapterInterface; +use Throwable; + +final class StackTraceFormatter +{ + private const NUM_LINES = 10; + + /** + * @return array + */ + public function compose(Throwable $throwable): array + { + $trace[] = [ + 'file' => $throwable->getFile(), + 'code' => $this->getSourceCode($throwable->getFile(), $throwable->getLine(), 'error-line'), + ]; + + foreach ($throwable->getTrace() as $item) { + if (($item['class'] ?? null) === ErrorHandler::class) { + continue; + } + + if (isset($item['file'])) { + $trace[] = [ + 'file' => $item['file'], + 'code' => $this->getSourceCode($item['file'], $item['line'] ?? 1, 'switch-line'), + ]; + } + } + + return $trace; + } + + public function getSourceCode(string $filename, int $lineNumber, string $className): string + { + $lineNumber--; + + $halfLines = intdiv(self::NUM_LINES, 2); + $start = max($lineNumber - $halfLines, 1); + + $adapter = fs()->getAdapter(); + + if (!$adapter instanceof LocalFilesystemAdapterInterface) { + return ''; + } + + $lines = $adapter->getLines($filename, $start, self::NUM_LINES); + + $code = '
    '; + + foreach ($lines as $currentLineNumber => $line) { + $code .= $this->formatLineItem($currentLineNumber, $line, $lineNumber, $className); + } + + return $code . '
'; + } + + public function formatLineItem(int $currentLineNumber, string $line, int $lineNumber, string $className): string + { + $highlightClass = $currentLineNumber === $lineNumber ? " class=\"{$className}\"" : ''; + $encodedLine = htmlspecialchars($line, ENT_QUOTES); + + return sprintf('
%s
', $highlightClass, $encodedLine); + } +} diff --git a/src/Tracer/WebExceptionRenderer.php b/src/Tracer/WebExceptionRenderer.php new file mode 100644 index 000000000..e04f2c026 --- /dev/null +++ b/src/Tracer/WebExceptionRenderer.php @@ -0,0 +1,50 @@ + + * @copyright Copyright (c) 2018 Softberg LLC (https://softberg.org) + * @link http://quantum.softberg.org/ + * @since 3.0.0 + */ + +namespace Quantum\Tracer; + +use Quantum\Renderer\Exceptions\RendererException; +use Quantum\Config\Exceptions\ConfigException; +use Quantum\App\Exceptions\BaseException; +use Quantum\Di\Exceptions\DiException; +use ReflectionException; +use Throwable; + +class WebExceptionRenderer +{ + private StackTraceFormatter $stackTraceFormatter; + + public function __construct(?StackTraceFormatter $stackTraceFormatter = null) + { + $this->stackTraceFormatter = $stackTraceFormatter ?? new StackTraceFormatter(); + } + + /** + * @throws ConfigException|RendererException|DiException|BaseException|ReflectionException + */ + public function render(Throwable $throwable, string $errorType): string + { + if (is_debug_mode()) { + return view()->renderPartial('errors' . DS . 'trace', [ + 'stackTrace' => $this->stackTraceFormatter->compose($throwable), + 'errorMessage' => $throwable->getMessage(), + 'severity' => ucfirst($errorType), + ]); + } + + return view()->renderPartial('errors' . DS . '500'); + } +} diff --git a/tests/Unit/Tracer/ErrorHandlerTest.php b/tests/Unit/Tracer/ErrorHandlerTest.php index bd03fe966..257e2ec06 100644 --- a/tests/Unit/Tracer/ErrorHandlerTest.php +++ b/tests/Unit/Tracer/ErrorHandlerTest.php @@ -2,12 +2,9 @@ namespace Quantum\Tests\Unit\Tracer; -use Quantum\Storage\Factories\FileSystemFactory; -use Quantum\Storage\Contracts\FilesystemAdapterInterface; -use Quantum\Storage\FileSystem; use Quantum\Tests\Unit\AppTestCase; use Quantum\Tracer\ErrorHandler; -use Quantum\Di\Di; +use Quantum\Tracer\WebExceptionRenderer; use Quantum\Logger\Logger; use Symfony\Component\Console\Output\BufferedOutput; use ReflectionException; @@ -94,16 +91,10 @@ public function testHandleErrorReturnsFalseForSuppressedErrors(): void } } - public function testErrorTypesConstant(): void - { - $this->assertEquals('error', ErrorHandler::ERROR_TYPES[E_ERROR]); - $this->assertEquals('warning', ErrorHandler::ERROR_TYPES[E_WARNING]); - $this->assertEquals('notice', ErrorHandler::ERROR_TYPES[E_NOTICE]); - $this->assertEquals('error', ErrorHandler::ERROR_TYPES[E_PARSE]); - } - public function testHandleExceptionCliWritesExceptionMessage(): void { + config()->set('app.debug', false); + $output = new BufferedOutput(); $this->errorHandler->setCliOutput($output); @@ -112,6 +103,21 @@ public function testHandleExceptionCliWritesExceptionMessage(): void $this->assertStringContainsString('CLI failure', $output->fetch()); } + public function testHandleExceptionCliInDebugModeWritesVerboseDiagnostics(): void + { + config()->set('app.debug', true); + + $output = new BufferedOutput(); + $this->errorHandler->setCliOutput($output); + + $this->errorHandler->handleException(new Exception('CLI debug failure')); + + $display = $output->fetch(); + $this->assertStringContainsString(Exception::class . ': CLI debug failure', $display); + $this->assertStringContainsString('In ', $display); + $this->assertStringContainsString(__FILE__, $display); + } + public function testHandleWebExceptionInDebugModeRendersTraceView(): void { config()->set('app.debug', true); @@ -150,6 +156,24 @@ public function testHandleWebExceptionInProductionModeLogsAndRenders500(): void $this->assertStringContainsString('ERROR 500 VIEW', $content); } + public function testHandleWebExceptionFallsBackWhenRendererThrows(): void + { + config()->set('app.debug', true); + + $renderer = Mockery::mock(WebExceptionRenderer::class); + $renderer->shouldReceive('render') + ->once() + ->andThrow(new Exception('render failed')); + + $handler = new ErrorHandler(null, null, $renderer); + + ob_start(); + $this->invokePrivateMethodOn($handler, 'handleWebException', [new Exception('boom')]); + ob_get_clean(); + + $this->assertSame('Internal Server Error', response()->getContent()); + } + public function testLogErrorReturnsEarlyWhenLoggerIsNull(): void { $this->setPrivateProperty($this->errorHandler, 'logger', null); @@ -173,145 +197,6 @@ public function testLogErrorFallsBackToErrorMethodForUnknownLevel(): void $this->assertTrue(true); } - public function testComposeStackTraceSkipsHandlerClassEntriesAndBuildsCodeSnippets(): void - { - $sourceFile = PROJECT_ROOT . DS . 'shared' . DS . 'trace-source.php'; - $this->createFile($sourceFile, "setAccessible(true); - $throwableProperty->setValue($throwable, $sourceFile); - - $lineProperty = new \ReflectionProperty(Exception::class, 'line'); - $lineProperty->setAccessible(true); - $lineProperty->setValue($throwable, 3); - - $traceProperty = new \ReflectionProperty(Exception::class, 'trace'); - $traceProperty->setAccessible(true); - $traceProperty->setValue($throwable, [ - ['class' => ErrorHandler::class, 'file' => $sourceFile, 'line' => 2], - ['class' => 'ExternalClass', 'file' => $sourceFile, 'line' => 4], - ['class' => 'NoFileClass'], - ]); - - $trace = $this->invokePrivateMethod('composeStackTrace', [$throwable]); - - $this->assertCount(2, $trace); - $this->assertSame($sourceFile, $trace[0]['file']); - $this->assertStringContainsString('
    ', $trace[0]['code']); - $this->assertStringContainsString('class="error-line"', $trace[0]['code']); - $this->assertSame($sourceFile, $trace[1]['file']); - $this->assertStringContainsString('class="switch-line"', $trace[1]['code']); - } finally { - $this->removeFile($sourceFile); - } - } - - public function testGetSourceCodeReturnsEmptyStringForNonLocalAdapter(): void - { - $adapter = new class () implements FilesystemAdapterInterface { - public function makeDirectory(string $dirname, ?string $parentId = null): bool - { - return false; - } - public function removeDirectory(string $dirname): bool - { - return false; - } - public function get(string $filename) - { - return false; - } - public function put(string $filename, $content, ?string $parentId = null) - { - return false; - } - public function append(string $filename, $content) - { - return false; - } - public function rename(string $oldName, string $newName): bool - { - return false; - } - public function copy(string $source, string $dest): bool - { - return false; - } - public function exists(string $filename): bool - { - return false; - } - public function size(string $filename) - { - return false; - } - public function lastModified(string $filename) - { - return false; - } - public function remove(string $filename): bool - { - return false; - } - public function isFile(string $filename): bool - { - return false; - } - public function isDirectory(string $dirname): bool - { - return false; - } - public function listDirectory(string $dirname) - { - return false; - } - }; - - $factory = Di::get(FileSystemFactory::class); - $instancesProperty = new \ReflectionProperty(FileSystemFactory::class, 'instances'); - $instancesProperty->setAccessible(true); - $originalInstances = $instancesProperty->getValue($factory); - - try { - $instancesProperty->setValue($factory, ['local' => new FileSystem($adapter)]); - - $code = $this->invokePrivateMethod('getSourceCode', [__FILE__, 10, 'error-line']); - - $this->assertSame('', $code); - } finally { - $instancesProperty->setValue($factory, $originalInstances); - } - } - - public function testGetSourceCodeBuildsOrderedListForLocalAdapter(): void - { - $file = PROJECT_ROOT . DS . 'shared' . DS . 'source-code.php'; - $this->createFile($file, "invokePrivateMethod('getSourceCode', [$file, 5, 'error-line']); - - $this->assertStringContainsString('
      ', $code); - $this->assertStringContainsString('class="error-line"', $code); - $this->assertStringContainsString('
      C
      ', $code); - } finally { - $this->removeFile($file); - } - } - - public function testFormatLineItemEscapesHtmlAndAppliesHighlightClass(): void - { - $html = $this->invokePrivateMethod('formatLineItem', [5, '
      A&B
      ', 5, 'switch-line']); - $plain = $this->invokePrivateMethod('formatLineItem', [4, 'plain', 5, 'switch-line']); - - $this->assertStringContainsString('class="switch-line"', $html); - $this->assertStringContainsString('<div class="x">A&B</div>', $html); - $this->assertStringNotContainsString('class="switch-line"', $plain); - } - /** * @dataProvider errorTypeProvider */ @@ -347,4 +232,16 @@ private function invokePrivateMethod(string $method, array $args = []) return $reflectionMethod->invokeArgs($this->errorHandler, $args); } + + /** + * @param array $args + * @return mixed + */ + private function invokePrivateMethodOn(object $object, string $method, array $args = []) + { + $reflectionMethod = new ReflectionMethod($object, $method); + $reflectionMethod->setAccessible(true); + + return $reflectionMethod->invokeArgs($object, $args); + } } diff --git a/tests/Unit/Tracer/ExceptionSeverityResolverTest.php b/tests/Unit/Tracer/ExceptionSeverityResolverTest.php new file mode 100644 index 000000000..c60981885 --- /dev/null +++ b/tests/Unit/Tracer/ExceptionSeverityResolverTest.php @@ -0,0 +1,42 @@ +resolver = new ExceptionSeverityResolver(); + } + + /** + * @dataProvider mappingProvider + */ + public function testResolveMappings(\Throwable $throwable, string $expected): void + { + $this->assertSame($expected, $this->resolver->resolve($throwable)); + } + + /** + * @return array + */ + public function mappingProvider(): array + { + return [ + 'mapped-error-exception' => [new ErrorException('x', 0, E_WARNING), 'warning'], + 'unmapped-error-exception' => [new ErrorException('x', 0, E_USER_DEPRECATED), 'error'], + 'parse-error' => [new ParseError('bad syntax'), 'critical'], + 'reflection-exception' => [new ReflectionException('reflection failed'), 'warning'], + 'generic-exception' => [new Exception('unknown'), 'error'], + ]; + } +} diff --git a/tests/Unit/Tracer/StackTraceFormatterTest.php b/tests/Unit/Tracer/StackTraceFormatterTest.php new file mode 100644 index 000000000..7e0789cd5 --- /dev/null +++ b/tests/Unit/Tracer/StackTraceFormatterTest.php @@ -0,0 +1,137 @@ +formatter = new StackTraceFormatter(); + } + + public function testComposeSkipsErrorHandlerClassEntriesAndBuildsCodeSnippets(): void + { + $sourceFile = PROJECT_ROOT . DS . 'shared' . DS . 'trace-source-formatter.php'; + $this->createFile($sourceFile, "setAccessible(true); + $throwableProperty->setValue($throwable, $sourceFile); + + $lineProperty = new \ReflectionProperty(Exception::class, 'line'); + $lineProperty->setAccessible(true); + $lineProperty->setValue($throwable, 3); + + $traceProperty = new \ReflectionProperty(Exception::class, 'trace'); + $traceProperty->setAccessible(true); + $traceProperty->setValue($throwable, [ + ['class' => ErrorHandler::class, 'file' => $sourceFile, 'line' => 2], + ['class' => 'ExternalClass', 'file' => $sourceFile, 'line' => 4], + ['class' => 'NoFileClass'], + ]); + + $trace = $this->formatter->compose($throwable); + + $this->assertCount(2, $trace); + $this->assertSame($sourceFile, $trace[0]['file']); + $this->assertStringContainsString('
        ', $trace[0]['code']); + $this->assertStringContainsString('class="error-line"', $trace[0]['code']); + $this->assertSame($sourceFile, $trace[1]['file']); + $this->assertStringContainsString('class="switch-line"', $trace[1]['code']); + } finally { + $this->removeFile($sourceFile); + } + } + + public function testGetSourceCodeReturnsEmptyStringForNonLocalAdapter(): void + { + $adapter = new class () implements FilesystemAdapterInterface { + public function makeDirectory(string $dirname, ?string $parentId = null): bool + { + return false; + } + public function removeDirectory(string $dirname): bool + { + return false; + } + public function get(string $filename) + { + return false; + } + public function put(string $filename, $content, ?string $parentId = null) + { + return false; + } + public function append(string $filename, $content) + { + return false; + } + public function rename(string $oldName, string $newName): bool + { + return false; + } + public function copy(string $source, string $dest): bool + { + return false; + } + public function exists(string $filename): bool + { + return false; + } + public function size(string $filename) + { + return false; + } + public function lastModified(string $filename) + { + return false; + } + public function remove(string $filename): bool + { + return false; + } + public function isFile(string $filename): bool + { + return false; + } + public function isDirectory(string $dirname): bool + { + return false; + } + public function listDirectory(string $dirname) + { + return false; + } + }; + + $factory = Di::get(FileSystemFactory::class); + $instancesProperty = new \ReflectionProperty(FileSystemFactory::class, 'instances'); + $instancesProperty->setAccessible(true); + $originalInstances = $instancesProperty->getValue($factory); + + try { + $instancesProperty->setValue($factory, ['local' => new FileSystem($adapter)]); + + $code = $this->formatter->getSourceCode(__FILE__, 10, 'error-line'); + + $this->assertSame('', $code); + } finally { + $instancesProperty->setValue($factory, $originalInstances); + } + } +} diff --git a/tests/Unit/Tracer/WebExceptionRendererTest.php b/tests/Unit/Tracer/WebExceptionRendererTest.php new file mode 100644 index 000000000..4b4ac1806 --- /dev/null +++ b/tests/Unit/Tracer/WebExceptionRendererTest.php @@ -0,0 +1,31 @@ +set('app.debug', true); + + $renderer = new WebExceptionRenderer(); + $html = $renderer->render(new Exception('Debug renderer failure'), 'warning'); + + $this->assertStringContainsString('TRACE VIEW', $html); + $this->assertStringContainsString('Warning :: Debug renderer failure', $html); + } + + public function testRenderProductionModeUses500Template(): void + { + config()->set('app.debug', false); + + $renderer = new WebExceptionRenderer(); + $html = $renderer->render(new Exception('Prod renderer failure'), 'error'); + + $this->assertStringContainsString('ERROR 500 VIEW', $html); + } +} From d0d1bb1919d98a34b760e7597b3048a394da89ac Mon Sep 17 00:00:00 2001 From: Arman <407448+armanist@users.noreply.github.com> Date: Fri, 1 May 2026 16:27:34 +0400 Subject: [PATCH 2/5] [#490] Remove temporary error-handler planning docs --- docs/error-handler-refactor-ticket-draft.md | 76 -------- docs/unify-errorhandler-web-flow.md | 185 -------------------- 2 files changed, 261 deletions(-) delete mode 100644 docs/error-handler-refactor-ticket-draft.md delete mode 100644 docs/unify-errorhandler-web-flow.md diff --git a/docs/error-handler-refactor-ticket-draft.md b/docs/error-handler-refactor-ticket-draft.md deleted file mode 100644 index b37884ee1..000000000 --- a/docs/error-handler-refactor-ticket-draft.md +++ /dev/null @@ -1,76 +0,0 @@ -## Title -Refactor `Tracer\ErrorHandler` for separation of concerns and safer fallback behavior - -## Background -`src/Tracer/ErrorHandler.php` currently handles multiple concerns in one class: -- CLI/web branching -- error page rendering -- stack trace source extraction/formatting -- severity mapping -- logging decisions -- direct web response send - -The current behavior works, but the class is hard to test and harder to evolve safely. - -## Goal -Refactor `ErrorHandler` to improve maintainability and testability without changing external behavior. - -## Scope -- Keep global handler registration flow unchanged (`SetupErrorHandlerStage`). -- Keep current user-visible behavior for CLI and web errors. -- Internally split logic into focused collaborators. - -## Proposed Changes -1. Extract `ExceptionSeverityResolver` -- Responsibility: map `Throwable` to error/log severity. -- Moves logic from `getErrorType()`. - -2. Extract `StackTraceFormatter` (or `StackTraceComposer`) -- Responsibility: build trace payload + source snippets for debug views. -- Moves logic from `composeStackTrace()`, `getSourceCode()`, `formatLineItem()`. - -3. Extract `WebExceptionRenderer` -- Responsibility: produce web error content for debug/production. -- Uses view rendering and formatter. -- Handles fallback minimal output if rendering fails. - -4. Keep `ErrorHandler` as orchestrator -- Responsibility: -- route CLI vs web path -- invoke renderer/resolver -- perform logging -- send final web response - -5. Add robust fallback behavior -- If web rendering fails inside error handling, fallback to plain text 500 response. -- Prevent recursive/secondary failure loops. - -## Non-Goals -- No change to routing/middleware/web adapter flow. -- No switch to adapter-level exception handling in this ticket. -- No behavioral change to current error templates/content (except emergency fallback path). - -## Acceptance Criteria -- Existing `ErrorHandler` behavior remains equivalent for: -- CLI exception output -- debug-mode web exception page -- production-mode web 500 page + logging -- New unit tests cover extracted components and fallback path. -- `ErrorHandler` unit tests are simplified and focused on orchestration. -- No regression in existing test suite. - -## Suggested Test Plan -- Unit: severity resolver mappings (`ErrorException`, `ParseError`, `ReflectionException`, generic `Throwable`). -- Unit: stack trace formatter with local filesystem adapter and non-local adapter. -- Unit: renderer in debug and production modes. -- Unit: fallback output when renderer throws. -- Integration-like unit: `ErrorHandler` orchestrates CLI/web and logging calls correctly. - -## Risks / Notes -- Refactor touches exception path; fallback behavior must be carefully tested. -- Keep constructor and wiring compatible with current DI/bootstrap usage. - -## Estimated Change Set -- `src/Tracer/ErrorHandler.php` (reduced orchestrator) -- new classes under `src/Tracer/` (or `src/Tracer/*`) -- test updates/additions under `tests/Unit/Tracer/` diff --git a/docs/unify-errorhandler-web-flow.md b/docs/unify-errorhandler-web-flow.md deleted file mode 100644 index f5091d69b..000000000 --- a/docs/unify-errorhandler-web-flow.md +++ /dev/null @@ -1,185 +0,0 @@ -# Unify ErrorHandler With Web Response Flow - -## Background -The web request/response pipeline now expects handlers to return a `Response`, with a shared send path that applies cross-cutting behavior (for example, CORS headers) before `send()`. - -Current exception handling in `Tracer\ErrorHandler` bypasses that pipeline by rendering and sending directly. This creates a second response-send path with duplicated behavior and a risk of drift. - -## Current Gap -- Normal flow: route/controller returns `Response` -> web adapter sends through shared path. -- Error flow: global exception handler builds output and calls `response()->send()` directly. - -Result: exception responses are not guaranteed to go through the same emit path as normal web responses. - -## Goal -Adopt one shared response emission path for: -- success responses -- 404/not-found responses -- uncaught exception responses - -while preserving global exception handling as a safety net. - -## What Is An Emitter? -An emitter is a very small class whose only job is to send a prepared `Response` to the client. - -In this codebase, "send a response" is not just `->send()`. We also need shared behavior (like CORS headers) right before send. That means response emission is a policy, not a one-liner. - -So an emitter gives us one place for that policy. - -## Why We Need It Here -Right now we have two web send paths: - -1. Normal request path: -- `WebAppAdapter` -> `WebAppTrait::sendResponse()` -> apply CORS -> `send()` - -2. Exception path: -- `ErrorHandler::handleWebException()` -> `response()->html(...)` -> `response()->send()` - -Issue: -- exception responses skip shared web send behavior (CORS and future headers/middleware-like concerns) -- behavior can drift over time because logic is duplicated - -Emitter fixes this by making both paths call the same send component. - -## Minimal Design - -### 1) New shared emitter -```php -has('cors')) { - config()->import(new Setup('config', 'cors')); - } - - foreach (config()->get('cors') as $key => $value) { - $response->setHeader($key, (string) $value); - } - - $response->send(); - } -} -``` - -### 2) Use emitter in normal flow (WebAppTrait) -Before: -```php -private function sendResponse(Response $response): void -{ - $this->handleCors($response); - $response->send(); -} -``` - -After: -```php -private function sendResponse(Response $response): void -{ - $emitter = Di::get(WebResponseEmitter::class); - $emitter->emit($response); -} -``` - -### 3) Use emitter in ErrorHandler web exception path -Before: -```php -response()->html($errorPage); -response()->send(); -``` - -After: -```php -$response = response()->html($errorPage); -$emitter = Di::get(WebResponseEmitter::class); -$emitter->emit($response); -``` - -## Important Clarification -Unifying flow does not mean "run exception responses through `WebAppAdapter::start()` again". - -When `set_exception_handler` runs, the original flow has already failed. The correct unification point is response emission policy (headers + send), not full request dispatch. - -## Proposed Approach - -### Phase 1: Extract exception-to-response mapping -Create a dedicated service (example: `ExceptionResponseFactory`) that converts `Throwable` -> `Response`. - -Responsibilities moved from `ErrorHandler`: -- debug/production error-page selection -- stack trace payload composition for debug view -- error type/severity mapping -- production logging decision - -### Phase 2: Centralize response emission -Create a reusable `ResponseEmitter` (or equivalent) used by all web response exits. - -Responsibilities: -- apply shared response concerns (existing CORS behavior) -- call `send()` exactly once - -Refactor callers: -- `WebAppAdapter` (normal path) uses emitter -- `ErrorHandler` web exception path uses emitter - -### Phase 3 (Optional): True single control-flow entry -Wrap top-level web app startup in `try/catch` and route caught exceptions through: -- `ExceptionResponseFactory` -- `ResponseEmitter` - -Keep `set_exception_handler` as fallback for fatal/unexpected contexts. - -## Scope -In-scope: -- `src/Tracer/ErrorHandler.php` -- web response emission path used by `WebAppAdapter` -- new service(s) for exception-response mapping and emission -- related unit tests - -Out-of-scope (for initial ticket): -- changing HTTP contract of controllers/routes -- broad boot pipeline redesign - -## Testing Plan -- Update `ErrorHandler` tests to validate delegation to new shared services. -- Add focused unit tests for `ExceptionResponseFactory`: - - debug vs production rendering - - severity mapping - - logging behavior -- Add unit tests for `ResponseEmitter`: - - shared headers applied - - single send behavior -- Keep/extend adapter tests to confirm exception responses still render and send correctly. - -## Risks and Mitigations -- Risk: double-send/partial output in exceptional paths. - - Mitigation: guard emitter usage and add explicit tests for send behavior. -- Risk: behavior changes in testing/CLI environment. - - Mitigation: isolate CLI exception path, keep existing behavior intact. -- Risk: hidden coupling from helper globals. - - Mitigation: move logic into injectable services and test boundaries directly. - -## Acceptance Criteria -- Uncaught web exceptions no longer call `response()->send()` via custom inline flow in `ErrorHandler`. -- Exception responses pass through the same shared emitter path as normal web responses. -- Existing behavior remains functionally equivalent (debug trace page, production 500 page, logging). -- Validation passes: - - `vendor/bin/phpunit --coverage-clover coverage.xml` - - `composer phpstan` - - `composer cs:check` - -## Suggested Ticket Title -Unify `Tracer\\ErrorHandler` exception response handling with shared web response pipeline From c81c160c38df336296f44d7bec7f1241a07ce3c3 Mon Sep 17 00:00:00 2001 From: Arman <407448+armanist@users.noreply.github.com> Date: Fri, 1 May 2026 16:58:51 +0400 Subject: [PATCH 3/5] [#490] Simplify web exception handling path and tests --- src/Tracer/ErrorHandler.php | 11 ++++------- tests/Unit/Tracer/ErrorHandlerTest.php | 14 +++++++++----- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/Tracer/ErrorHandler.php b/src/Tracer/ErrorHandler.php index 6cc884922..1f6e8f733 100644 --- a/src/Tracer/ErrorHandler.php +++ b/src/Tracer/ErrorHandler.php @@ -22,6 +22,7 @@ use Quantum\Config\Exceptions\ConfigException; use Quantum\App\Exceptions\BaseException; use Quantum\Di\Exceptions\DiException; +use Quantum\Http\Enums\StatusCode; use Quantum\Logger\Logger; use ReflectionException; use ErrorException; @@ -111,18 +112,14 @@ private function handleCliException(Throwable $throwable): void private function handleWebException(Throwable $throwable): void { $errorType = $this->getErrorType($throwable); - - try { - $errorPage = $this->webExceptionRenderer->render($throwable, $errorType); - } catch (Throwable $e) { - $errorPage = 'Internal Server Error'; - } + $statusCode = StatusCode::INTERNAL_SERVER_ERROR; + $errorPage = $this->webExceptionRenderer->render($throwable, $errorType); if (!is_debug_mode()) { $this->logError($throwable, $errorType); } - response()->html($errorPage); + response()->html($errorPage, $statusCode); response()->send(); } diff --git a/tests/Unit/Tracer/ErrorHandlerTest.php b/tests/Unit/Tracer/ErrorHandlerTest.php index 257e2ec06..e5377b5a6 100644 --- a/tests/Unit/Tracer/ErrorHandlerTest.php +++ b/tests/Unit/Tracer/ErrorHandlerTest.php @@ -156,7 +156,7 @@ public function testHandleWebExceptionInProductionModeLogsAndRenders500(): void $this->assertStringContainsString('ERROR 500 VIEW', $content); } - public function testHandleWebExceptionFallsBackWhenRendererThrows(): void + public function testHandleWebExceptionPropagatesRendererFailure(): void { config()->set('app.debug', true); @@ -167,11 +167,15 @@ public function testHandleWebExceptionFallsBackWhenRendererThrows(): void $handler = new ErrorHandler(null, null, $renderer); - ob_start(); - $this->invokePrivateMethodOn($handler, 'handleWebException', [new Exception('boom')]); - ob_get_clean(); + $this->expectException(Exception::class); + $this->expectExceptionMessage('render failed'); - $this->assertSame('Internal Server Error', response()->getContent()); + ob_start(); + try { + $this->invokePrivateMethodOn($handler, 'handleWebException', [new Exception('boom')]); + } finally { + ob_get_clean(); + } } public function testLogErrorReturnsEarlyWhenLoggerIsNull(): void From b12c13bd256aa2e004e0096ade051d6efdb56255 Mon Sep 17 00:00:00 2001 From: Arman <407448+armanist@users.noreply.github.com> Date: Fri, 1 May 2026 17:39:24 +0400 Subject: [PATCH 4/5] [#490] Unify tracer test helpers and clean formatter tests --- tests/Unit/AppTestCase.php | 9 ++ tests/Unit/Tracer/ErrorHandlerTest.php | 40 ++------- tests/Unit/Tracer/StackTraceFormatterTest.php | 83 +++---------------- .../Unit/Tracer/WebExceptionRendererTest.php | 2 +- 4 files changed, 28 insertions(+), 106 deletions(-) diff --git a/tests/Unit/AppTestCase.php b/tests/Unit/AppTestCase.php index 0d4cc8201..40f9f4b61 100644 --- a/tests/Unit/AppTestCase.php +++ b/tests/Unit/AppTestCase.php @@ -15,6 +15,7 @@ use Quantum\Router\Route; use Quantum\Http\Response; use ReflectionClass; +use ReflectionMethod; use Quantum\App\App; use Quantum\Di\Di; use ReflectionProperty; @@ -92,6 +93,14 @@ protected function getPrivateProperty($object, $property) return $property->getValue($object); } + protected function invokePrivateMethod(object $object, string $method, array $args = []) + { + $reflectionMethod = new ReflectionMethod($object, $method); + $reflectionMethod->setAccessible(true); + + return $reflectionMethod->invokeArgs($object, $args); + } + protected function createFile(string $filePath, string $content) { $this->fs->put($filePath, $content); diff --git a/tests/Unit/Tracer/ErrorHandlerTest.php b/tests/Unit/Tracer/ErrorHandlerTest.php index e5377b5a6..990870580 100644 --- a/tests/Unit/Tracer/ErrorHandlerTest.php +++ b/tests/Unit/Tracer/ErrorHandlerTest.php @@ -2,13 +2,12 @@ namespace Quantum\Tests\Unit\Tracer; +use Symfony\Component\Console\Output\BufferedOutput; +use Quantum\Tracer\WebExceptionRenderer; use Quantum\Tests\Unit\AppTestCase; use Quantum\Tracer\ErrorHandler; -use Quantum\Tracer\WebExceptionRenderer; use Quantum\Logger\Logger; -use Symfony\Component\Console\Output\BufferedOutput; use ReflectionException; -use ReflectionMethod; use ErrorException; use ParseError; use Exception; @@ -125,7 +124,7 @@ public function testHandleWebExceptionInDebugModeRendersTraceView(): void $exception = new Exception('Web debug failure'); ob_start(); - $this->invokePrivateMethod('handleWebException', [$exception]); + $this->invokePrivateMethod($this->errorHandler, 'handleWebException', [$exception]); ob_get_clean(); $content = response()->getContent(); @@ -149,7 +148,7 @@ public function testHandleWebExceptionInProductionModeLogsAndRenders500(): void $exception = new ErrorException('Web production warning', 0, E_WARNING, __FILE__, __LINE__); ob_start(); - $this->invokePrivateMethod('handleWebException', [$exception]); + $this->invokePrivateMethod($this->errorHandler, 'handleWebException', [$exception]); ob_get_clean(); $content = response()->getContent(); @@ -172,7 +171,7 @@ public function testHandleWebExceptionPropagatesRendererFailure(): void ob_start(); try { - $this->invokePrivateMethodOn($handler, 'handleWebException', [new Exception('boom')]); + $this->invokePrivateMethod($handler, 'handleWebException', [new Exception('boom')]); } finally { ob_get_clean(); } @@ -182,7 +181,7 @@ public function testLogErrorReturnsEarlyWhenLoggerIsNull(): void { $this->setPrivateProperty($this->errorHandler, 'logger', null); - $this->invokePrivateMethod('logError', [new Exception('No logger'), 'error']); + $this->invokePrivateMethod($this->errorHandler, 'logError', [new Exception('No logger'), 'error']); $this->assertTrue(true); } @@ -196,7 +195,7 @@ public function testLogErrorFallsBackToErrorMethodForUnknownLevel(): void $this->setPrivateProperty($this->errorHandler, 'logger', $logger); - $this->invokePrivateMethod('logError', [new Exception('Unknown level message'), 'not_existing_level']); + $this->invokePrivateMethod($this->errorHandler, 'logError', [new Exception('Unknown level message'), 'not_existing_level']); $this->assertTrue(true); } @@ -206,7 +205,7 @@ public function testLogErrorFallsBackToErrorMethodForUnknownLevel(): void */ public function testGetErrorTypeMappings(\Throwable $throwable, string $expected): void { - $type = $this->invokePrivateMethod('getErrorType', [$throwable]); + $type = $this->invokePrivateMethod($this->errorHandler, 'getErrorType', [$throwable]); $this->assertSame($expected, $type); } @@ -225,27 +224,4 @@ public function errorTypeProvider(): array ]; } - /** - * @param array $args - * @return mixed - */ - private function invokePrivateMethod(string $method, array $args = []) - { - $reflectionMethod = new ReflectionMethod($this->errorHandler, $method); - $reflectionMethod->setAccessible(true); - - return $reflectionMethod->invokeArgs($this->errorHandler, $args); - } - - /** - * @param array $args - * @return mixed - */ - private function invokePrivateMethodOn(object $object, string $method, array $args = []) - { - $reflectionMethod = new ReflectionMethod($object, $method); - $reflectionMethod->setAccessible(true); - - return $reflectionMethod->invokeArgs($object, $args); - } } diff --git a/tests/Unit/Tracer/StackTraceFormatterTest.php b/tests/Unit/Tracer/StackTraceFormatterTest.php index 7e0789cd5..538595bc5 100644 --- a/tests/Unit/Tracer/StackTraceFormatterTest.php +++ b/tests/Unit/Tracer/StackTraceFormatterTest.php @@ -2,14 +2,16 @@ namespace Quantum\Tests\Unit\Tracer; -use Quantum\Storage\Factories\FileSystemFactory; use Quantum\Storage\Contracts\FilesystemAdapterInterface; -use Quantum\Storage\FileSystem; +use Quantum\Storage\Factories\FileSystemFactory; +use Quantum\Tracer\StackTraceFormatter; use Quantum\Tests\Unit\AppTestCase; use Quantum\Tracer\ErrorHandler; -use Quantum\Tracer\StackTraceFormatter; +use Quantum\Storage\FileSystem; use Quantum\Di\Di; use Exception; +use Mockery; +use ReflectionProperty; class StackTraceFormatterTest extends AppTestCase { @@ -29,17 +31,9 @@ public function testComposeSkipsErrorHandlerClassEntriesAndBuildsCodeSnippets(): try { $throwable = new Exception('trace'); - $throwableProperty = new \ReflectionProperty(Exception::class, 'file'); - $throwableProperty->setAccessible(true); - $throwableProperty->setValue($throwable, $sourceFile); - - $lineProperty = new \ReflectionProperty(Exception::class, 'line'); - $lineProperty->setAccessible(true); - $lineProperty->setValue($throwable, 3); - - $traceProperty = new \ReflectionProperty(Exception::class, 'trace'); - $traceProperty->setAccessible(true); - $traceProperty->setValue($throwable, [ + $this->setPrivateProperty($throwable, 'file', $sourceFile); + $this->setPrivateProperty($throwable, 'line', 3); + $this->setPrivateProperty($throwable, 'trace', [ ['class' => ErrorHandler::class, 'file' => $sourceFile, 'line' => 2], ['class' => 'ExternalClass', 'file' => $sourceFile, 'line' => 4], ['class' => 'NoFileClass'], @@ -60,67 +54,10 @@ public function testComposeSkipsErrorHandlerClassEntriesAndBuildsCodeSnippets(): public function testGetSourceCodeReturnsEmptyStringForNonLocalAdapter(): void { - $adapter = new class () implements FilesystemAdapterInterface { - public function makeDirectory(string $dirname, ?string $parentId = null): bool - { - return false; - } - public function removeDirectory(string $dirname): bool - { - return false; - } - public function get(string $filename) - { - return false; - } - public function put(string $filename, $content, ?string $parentId = null) - { - return false; - } - public function append(string $filename, $content) - { - return false; - } - public function rename(string $oldName, string $newName): bool - { - return false; - } - public function copy(string $source, string $dest): bool - { - return false; - } - public function exists(string $filename): bool - { - return false; - } - public function size(string $filename) - { - return false; - } - public function lastModified(string $filename) - { - return false; - } - public function remove(string $filename): bool - { - return false; - } - public function isFile(string $filename): bool - { - return false; - } - public function isDirectory(string $dirname): bool - { - return false; - } - public function listDirectory(string $dirname) - { - return false; - } - }; + $adapter = Mockery::mock(FilesystemAdapterInterface::class); $factory = Di::get(FileSystemFactory::class); - $instancesProperty = new \ReflectionProperty(FileSystemFactory::class, 'instances'); + $instancesProperty = new ReflectionProperty(FileSystemFactory::class, 'instances'); $instancesProperty->setAccessible(true); $originalInstances = $instancesProperty->getValue($factory); diff --git a/tests/Unit/Tracer/WebExceptionRendererTest.php b/tests/Unit/Tracer/WebExceptionRendererTest.php index 4b4ac1806..d19f1d4f5 100644 --- a/tests/Unit/Tracer/WebExceptionRendererTest.php +++ b/tests/Unit/Tracer/WebExceptionRendererTest.php @@ -2,8 +2,8 @@ namespace Quantum\Tests\Unit\Tracer; -use Quantum\Tests\Unit\AppTestCase; use Quantum\Tracer\WebExceptionRenderer; +use Quantum\Tests\Unit\AppTestCase; use Exception; class WebExceptionRendererTest extends AppTestCase From 3786a21c8c92ddd5c26da43097e5fbe618f20287 Mon Sep 17 00:00:00 2001 From: Arman <407448+armanist@users.noreply.github.com> Date: Fri, 1 May 2026 17:56:48 +0400 Subject: [PATCH 5/5] [#490] Harden web exception fallback and tracer tests --- src/Tracer/ErrorHandler.php | 12 ++++++++---- tests/Unit/Tracer/ErrorHandlerTest.php | 16 +++++++--------- tests/Unit/Tracer/StackTraceFormatterTest.php | 2 +- 3 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/Tracer/ErrorHandler.php b/src/Tracer/ErrorHandler.php index 1f6e8f733..82bf92874 100644 --- a/src/Tracer/ErrorHandler.php +++ b/src/Tracer/ErrorHandler.php @@ -112,15 +112,19 @@ private function handleCliException(Throwable $throwable): void private function handleWebException(Throwable $throwable): void { $errorType = $this->getErrorType($throwable); - $statusCode = StatusCode::INTERNAL_SERVER_ERROR; - $errorPage = $this->webExceptionRenderer->render($throwable, $errorType); if (!is_debug_mode()) { $this->logError($throwable, $errorType); } - response()->html($errorPage, $statusCode); - response()->send(); + try { + $errorPage = $this->webExceptionRenderer->render($throwable, $errorType); + response()->html($errorPage, StatusCode::INTERNAL_SERVER_ERROR); + response()->send(); + } catch (Throwable $e) { + response()->html('Internal Server Error', StatusCode::INTERNAL_SERVER_ERROR); + response()->send(); + } } private function logError(Throwable $e, string $errorType): void diff --git a/tests/Unit/Tracer/ErrorHandlerTest.php b/tests/Unit/Tracer/ErrorHandlerTest.php index 990870580..3c12df59a 100644 --- a/tests/Unit/Tracer/ErrorHandlerTest.php +++ b/tests/Unit/Tracer/ErrorHandlerTest.php @@ -155,7 +155,7 @@ public function testHandleWebExceptionInProductionModeLogsAndRenders500(): void $this->assertStringContainsString('ERROR 500 VIEW', $content); } - public function testHandleWebExceptionPropagatesRendererFailure(): void + public function testHandleWebExceptionFallsBackWhenRendererThrows(): void { config()->set('app.debug', true); @@ -166,15 +166,13 @@ public function testHandleWebExceptionPropagatesRendererFailure(): void $handler = new ErrorHandler(null, null, $renderer); - $this->expectException(Exception::class); - $this->expectExceptionMessage('render failed'); - ob_start(); - try { - $this->invokePrivateMethod($handler, 'handleWebException', [new Exception('boom')]); - } finally { - ob_get_clean(); - } + $this->invokePrivateMethod($handler, 'handleWebException', [new Exception('boom')]); + ob_get_clean(); + + $this->assertSame('Internal Server Error', response()->getContent()); + $this->assertSame(500, response()->getStatusCode()); + $this->assertStringStartsWith('text/html', (string) response()->getHeader('Content-Type')); } public function testLogErrorReturnsEarlyWhenLoggerIsNull(): void diff --git a/tests/Unit/Tracer/StackTraceFormatterTest.php b/tests/Unit/Tracer/StackTraceFormatterTest.php index 538595bc5..4b6af87f5 100644 --- a/tests/Unit/Tracer/StackTraceFormatterTest.php +++ b/tests/Unit/Tracer/StackTraceFormatterTest.php @@ -8,10 +8,10 @@ use Quantum\Tests\Unit\AppTestCase; use Quantum\Tracer\ErrorHandler; use Quantum\Storage\FileSystem; +use ReflectionProperty; use Quantum\Di\Di; use Exception; use Mockery; -use ReflectionProperty; class StackTraceFormatterTest extends AppTestCase {