HP-2823: modified Has Context Exception class to returns formatter message with context#15
Conversation
…ssage with context
WalkthroughThe PR introduces exception formatting capabilities through a new Changes
Sequence DiagramsequenceDiagram
actor Client
participant ExceptionDebugFormatter
participant HasContext
participant HasContextInterface
Client->>ExceptionDebugFormatter: format(exception, skipContext)
ExceptionDebugFormatter->>ExceptionDebugFormatter: Extract class, message, location
alt Exception implements HasContextInterface
alt skipContext is false
ExceptionDebugFormatter->>HasContextInterface: getContext()
HasContextInterface-->>ExceptionDebugFormatter: context array
ExceptionDebugFormatter->>HasContext: getExceptionDebugInfo() [helper]
HasContext-->>ExceptionDebugFormatter: formatted exception data
end
end
alt Has previous exception
ExceptionDebugFormatter->>ExceptionDebugFormatter: format(previousException, skipContext)<br/>(recursive)
ExceptionDebugFormatter->>ExceptionDebugFormatter: Nest as parentException
end
ExceptionDebugFormatter-->>Client: array with debug info & chain
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/unit/exception/HasContextTest.php (1)
99-100: Consider verifying JSON structure instead of string matching.The current assertions check for specific JSON formatting (e.g.,
"a": 1with specific spacing), which tests implementation details rather than behavior. This could break if JSON formatting options change.Consider parsing the JSON and verifying the structure:
-// Pretty-printed JSON -$this->assertStringContainsString('"a": 1', $output); -$this->assertStringContainsString('"b": 2', $output); +// Verify JSON structure by parsing +$this->assertStringContainsString('data', $output); +// Extract and parse the JSON portion if needed for stricter validationsrc/Infrastructure/Exception/ExceptionDebugFormatter.php (1)
25-27: Clarify variable naming for the previous exception.The variable
$rootis misleading as it typically refers to the root/oldest exception in a chain, butgetPrevious()returns the immediate previous/parent exception.Apply this diff for clarity:
-if ($root = $e->getPrevious()) { - $debugInfo['parentException'] = $this->format($root, $skipContext); +if ($previous = $e->getPrevious()) { + $debugInfo['parentException'] = $this->format($previous, $skipContext); }src/exception/HasContext.php (2)
70-73: Add defensive error handling for JSON encoding.Using
JSON_THROW_ON_ERRORduring exception formatting (error handling) can throw aJsonExceptionand mask the original problem. This can happen with recursive data structures, resources, or non-serializable objects.Apply this diff to handle encoding errors gracefully:
private function jsonEncode($value): string { - return \json_encode($value, JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT); + try { + return \json_encode($value, JSON_THROW_ON_ERROR | JSON_PRETTY_PRINT); + } catch (\JsonException $e) { + // Fallback for non-serializable values + return \print_r($value, true); + } }
65-68: Consider reusing the formatter instance.Creating a new
ExceptionDebugFormatterinstance on every call is unnecessary. While the performance impact is minimal, a static instance would be more efficient.Consider this approach:
private function getExceptionDebugInfo(?Throwable $throwable): array { - return (new ExceptionDebugFormatter())->format($throwable); + static $formatter = null; + if ($formatter === null) { + $formatter = new ExceptionDebugFormatter(); + } + return $formatter->format($throwable); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Infrastructure/Exception/ExceptionDebugFormatter.php(1 hunks)src/exception/HasContext.php(2 hunks)src/exception/HasContextInterface.php(1 hunks)tests/unit/exception/HasContextTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/exception/HasContextInterface.php (1)
src/exception/HasContext.php (1)
getFormattedContext(42-63)
src/Infrastructure/Exception/ExceptionDebugFormatter.php (2)
src/exception/HasContext.php (2)
getContext(24-27)getPrevious(15-15)src/exception/HasContextInterface.php (1)
getContext(9-9)
src/exception/HasContext.php (2)
src/Infrastructure/Exception/ExceptionDebugFormatter.php (2)
ExceptionDebugFormatter(9-30)format(11-29)src/exception/HasContextInterface.php (2)
getFormattedContext(11-11)getContext(9-9)
tests/unit/exception/HasContextTest.php (3)
tests/unit/exception/stub/TestException.php (1)
TestException(11-14)src/exception/HasContext.php (2)
getFormattedContext(42-63)addContext(17-22)src/exception/HasContextInterface.php (2)
getFormattedContext(11-11)addContext(7-7)
🔇 Additional comments (4)
src/exception/HasContextInterface.php (1)
11-11: LGTM!The interface extension is clean and the return type is appropriate for formatted output.
tests/unit/exception/HasContextTest.php (1)
67-116: Good test coverage for the new formatting functionality.The tests cover empty context, simple values, complex values, and exception chaining appropriately.
src/Infrastructure/Exception/ExceptionDebugFormatter.php (1)
11-29: The formatting logic is correct and handles exception chaining well.The recursive processing of previous exceptions and conditional context inclusion work as intended.
src/exception/HasContext.php (1)
42-63: The formatting implementation is well-structured.The method correctly builds human-readable output by iterating over context and including previous exception details.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.