-
Notifications
You must be signed in to change notification settings - Fork 7
HP-2823: modified Has Context Exception class to returns formatter message with context #15
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.