Skip to content

fix: clean up buffer on error #12

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.

Sign up for GitHub

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

Merged
dkaser merged 1 commit into from
Oct 21, 2025
Merged

fix: clean up buffer on error #12

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/usr/local/emhttp/plugins/plugin-diagnostics/include/page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,13 @@ function includePage(string $filename, array $params = array()): string

if (is_file($filename)) {
ob_start();
include $filename;
return ob_get_clean() ?: "";
try {
include $filename;
return ob_get_clean() ?: "";
} catch (\Throwable $e) {
ob_end_clean();
Comment on lines +58 to +60
Copy link

@codeant-ai codeant-ai bot Oct 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Guard calls to output-buffer functions with an ob_get_level() check so ob_get_clean() / ob_end_clean() are only called when a buffer actually exists, avoiding warnings or false failures if the buffer was closed inside the included file. [best practice]

Severity Level: Minor ⚠️

Suggested change
return ob_get_clean() ?: "";
} catch (\Throwable $e) {
ob_end_clean();
if (ob_get_level() > 0) {
return ob_get_clean() ?: "";
}
return "";
} catch (\Throwable $e) {
if (ob_get_level() > 0) {
ob_end_clean();
}
Why it matters? ⭐

The suggestion is valid: the PR starts an output buffer then includes arbitrary page code that may itself manipulate or close buffers. Calling ob_get_clean()/ob_end_clean() unguarded can emit warnings or silently fail if the included file already cleaned the buffer. Wrapping those calls with ob_get_level() > 0 prevents warnings and makes the behavior deterministic. This fixes a real robustness issue rather than being merely cosmetic.

Prompt for AI Agent 🤖 
<code>This is a comment left during a code review.

**Path:** src/usr/local/emhttp/plugins/plugin-diagnostics/include/page.php
**Line:** 58:60
**Comment:**
	*Best Practice: Guard calls to output-buffer functions with an `ob_get_level()` check so `ob_get_clean()` / `ob_end_clean()` are only called when a buffer actually exists, avoiding warnings or false failures if the buffer was closed inside the included file.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, please make it concise.

throw $e;
}
}
return "";
}