Fix #106: Improve closure rendering in stack traces#172
Conversation
WarLikeLaux
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #172 +/- ##
============================================
+ Coverage 87.18% 87.44% +0.26%
- Complexity 225 233 +8
============================================
Files 19 19
Lines 710 725 +15
============================================
+ Hits 619 634 +15
Misses 91 91 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes HTML stack trace rendering for closure frames (including PHP 8.4+ {closure:...} format) and ensures stack frames are not dropped when source snippets can’t be read, addressing #106.
Changes:
- Keep rendering call stack items even when the source file can’t be read or the requested line is out of range (skip only the source excerpt).
- Introduce
HtmlRenderer::formatTraceFunctionName()and use it in the call-stack item template to normalize closure names (PHP < 8.4 and PHP 8.4+). - Add/adjust test fixtures and PHPUnit coverage for closure rendering scenarios (method closures, bound closures, internal-function closures, and file-level closures).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/Renderer/HtmlRenderer.php |
Adds trace function name formatter, recognizes {closure:...} frames, and keeps frames when source can’t be loaded. |
templates/_call-stack-item.php |
Uses the new formatter to display cleaner function/closure names. |
tests/Renderer/HtmlRendererTest.php |
Expands test coverage for closure formatting and for rendering frames without source excerpts. |
tests/Support/NamespacedClosureTraceFixture.php |
Fixture for PHP < 8.4 namespaced closure trace behavior. |
tests/Support/FileLevelClosureLoader.php |
Helper to load a file-level closure fixture for PHP 8.4+ behavior. |
tests/Support/file_level_closure_exception.php |
File-level closure exception generator used by tests. |
CHANGELOG.md |
Records the bugfix in the unreleased section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
vjik
left a comment
vjik
left a comment
There was a problem hiding this comment.
The edits are similar to LLM usage without review. Am I right?
# Conflicts: # tests/Renderer/HtmlRendererTest.php
Co-authored-by: Sergei Predvoditelev <sergey.predvoditelev@gmail.com>
WarLikeLaux
changed the title
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe PR refactors stack-trace closure rendering and source extraction. ChangesClosure Rendering and Source Handling
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Renderer/HtmlRenderer.php`:
- Around line 776-777: The branch that handles invalid snippet lines currently
sets $lines = [] but leaves $line unchanged so templates can render an
impossible non-positive value; modify the branch that checks "if ($line < 0 ||
$lines === false)" to also set $line = null (in addition to $lines = []) so the
frame is preserved but templates receive a null line instead of 0 or negative;
locate the variables $line and $lines in the HtmlRenderer rendering code path
(around the snippet extraction logic) and update that branch to assign $line =
null.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca807532-a3a2-41b3-9c6c-50a79856dde7
📒 Files selected for processing (7)
CHANGELOG.mdsrc/Renderer/HtmlRenderer.phptemplates/_call-stack-item.phptests/Renderer/HtmlRendererTest.phptests/Support/FileLevelClosureLoader.phptests/Support/NamespacedClosureTraceFixture.phptests/Support/file_level_closure_exception.php
Yeah, fair. I do use Claude Code, but I go through the diff and run the tests myself rather than just pasting output. Doesn't mean I catch everything though - I didn't here and that's on me. I'll keep a closer eye on it. |
What does this PR do?
This fixes how closures are shown in the HTML stack trace and stops some trace items from disappearing.
Keep trace items when the source snippet is unavailable.
renderCallStackItem()used to return an empty string when the source file could not be read. That removed the whole item from the error page. Now the item still renders with the available file and function data. The source viewer is skipped.Remove duplicated namespace parts in closure names. On PHP < 8.4, a namespaced closure like
Yiisoft\Yii\Gii\{closure}was combined with the class name and ended up asYiisoft\Yii\Gii\Gii::Yiisoft\Yii\Gii\{closure}. It now renders asYiisoft\Yii\Gii\Gii::{closure}. Bound closures (class=Closure) also stop showing the extraClosure::prefix.Format PHP 8.4+ closure names. PHP 8.4+ uses closure names like
{closure:Foo::bar():4}that include the definition location. The HTML template now formats that value as{closure} Foo::bar():4.renderCallStack()also treats both{closure}and{closure:...}as closure frames.Before / After
Ns\Gii::Ns\{closure}Ns\Gii::{closure}Closure::{closure}{closure}Foo::{closure:Foo::bar():4}{closure} Foo::bar():4{closure:/app/index.php:12}{closure} /app/index.php:12Foo::{closure}Foo::{closure}(unchanged)Note on missing file/line
Some closures called through internal PHP functions such as
array_mapdo not havefileorlinein the trace data. In that case the page shows the function name and arguments only, without a source viewer for that frame. This matches Symfony's error handler, which also renders source excerpts only for frames that have a file path. On PHP 8.4+, the formatted closure name still shows where the closure was defined.BC
No BC break:
formatTraceFunctionName()is a new public helper used by the HTML template.P.S. Existing tests that use custom templates now create temporary template files per test. This avoids random-order interference between tests and does not change runtime behavior.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests