Improve closure rendering in stack traces (#172)#172
Improve closure rendering in stack traces (#172)#172WarLikeLaux wants to merge 5 commits intoyiisoft:masterfrom
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 86.92% 87.16% +0.23%
- Complexity 216 223 +7
============================================
Files 19 19
Lines 696 709 +13
============================================
+ Hits 605 618 +13
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.
| $this->assertSame('', $result); | ||
| $this->assertStringContainsString('not-exist', $result); | ||
| $this->assertStringContainsString('call-stack-item', $result); | ||
| $this->assertSame('file(not-exist): Failed to open stream: No such file or directory', $errorMessage); |
There was a problem hiding this comment.
This test asserts the full warning text from file('not-exist') exactly. That message can vary across OS/PHP builds/locales (e.g., Windows vs Linux), making the test brittle. Prefer asserting key substrings (or simply that an error was captured) rather than an exact full message.
| $this->assertSame('file(not-exist): Failed to open stream: No such file or directory', $errorMessage); | |
| $this->assertNotNull($errorMessage); |
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
There was a problem hiding this comment.
file_level_closure_exception.php uses RuntimeException multiple times but doesn't import it, while other test support fixtures consistently use RuntimeException;. For consistency (and to match the project's explicit-import convention), add use RuntimeException; after the declare(strict_types=1); line.
| use RuntimeException; |
| use function dirname; | ||
| use function file_exists; | ||
| use function file_put_contents; | ||
| use function fopen; | ||
| use function Yiisoft\ErrorHandler\Tests\Support\loadFileLevelClosureException; | ||
| use function unlink; | ||
| use function sprintf; | ||
| use function count; | ||
|
|
||
| use const DIRECTORY_SEPARATOR; | ||
| use const PHP_VERSION_ID; | ||
|
|
||
| require_once dirname(__DIR__) . '/Support/FileLevelClosureLoader.php'; |
There was a problem hiding this comment.
This file follows an explicit use function ... import style, but the newly added code uses additional global functions (file, tempnam, sys_get_temp_dir, preg_quote, array_map, etc.) without importing them. Please add the missing use function imports (or otherwise make the usage consistent) to avoid mixing styles within the same file.
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?
| $lines = []; | ||
| } else { | ||
| $lineCount = count($lines); | ||
| if ($line < $lineCount) { |
There was a problem hiding this comment.
| if ($line < $lineCount) { | |
| if ($line <= $lineCount) { |
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.