feat(1314): add unclosed ob_start check

[faisalahammad]

What?

Closes #1314

Adds a new performance check to detect ob_start() calls that are not paired with a corresponding buffer-closing function within the same scope. Prevents unpredictable behaviour caused by misaligned output buffers in shared WordPress environments.

Why?

Every ob_start() must be paired with a closing call (ob_get_clean(), ob_end_clean(), ob_get_flush(), or ob_end_flush()) in the same scope. Unclosed buffers break headers, lose output, and cause redirect issues in shared hosting. WordPress core, themes, and other plugins all share the buffer stack.

How?

Implemented as a PHPCS sniff (PluginCheck.CodeAnalysis.UnclosedObStart) following @westonruter's suggestion:

• Extends Abstract_PHP_CodeSniffer_Check like other sniff-based checks
• Uses PHPCS conditions data for scope tracking instead of hand-rolled brace-depth
• Resolves fully-qualified \ob_start() calls (PHP 7.4 two-token form + PHP 8+ T_NAME_FULLY_QUALIFIED)
• Flags conditional closes (e.g. ob_end_clean() only inside an if) as warnings
• Conservative hook-paired heuristic: suppresses warning when ob_start() is immediately followed by add_action()/add_filter()
• Registered in Default_Check_Repository under both CATEGORY_PERFORMANCE and CATEGORY_PLUGIN_REPO

Testing Instructions

1. Install the plugin from this branch
2. Run wp plugin check <plugin-slug> or use the Plugin Check admin UI
3. Verify the "unclosed_ob_start" check runs by default in the Plugin Directory view (CATEGORY_PLUGIN_REPO ensures it is selected automatically)
4. Test with a plugin that has an unpaired ob_start() — confirm a warning is reported on the exact line
5. Test with a plugin that has paired ob_start() / ob_end_clean() in the same function — confirm no warning
6. Test with \ob_start() in namespaced code — confirm detection works
7. Test the hook-paired case (ob_start() + add_action()) — confirm no warning

AI Usage Disclosure

• This PR was created without the help of AI tools
• This PR includes AI-assisted code or content

If AI tools were used, please describe how they were used:

• Claude Code assisted with PHPCS sniff architecture research (reviewing existing AbstractFunctionParameterSniff / AbstractFunctionRestrictionsSniff patterns in phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/)
• Drafted the scope-tracking logic (function/closure walk via $tokens[ $stackPtr ]['conditions'])
• Generated the PHP 7.4 vs 8.0 fully-qualified name handling (T_STRING + T_NS_SEPARATOR vs T_NAME_FULLY_QUALIFIED)
• Generated the PHPUnit fixture scenarios (paired, multiple, closure, conditional, FQN, hook-paired)
• Drafted the PR body and testing instructions

Screenshots or screencast

Before	After
CLI / no UI change	CLI / no UI change

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: faisalahammad <faisalahammad@git.wordpress.org>
Co-authored-by: davidperezgar <davidperez@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: ernilambar <nilambar@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[davidperezgar]

Thanks for working on this. The new check is a good direction, but I found a few issues that should be addressed before merge:

1. Fully-qualified \ob_start() calls are currently missed. On PHP 8, \ob_start() tokenizes as T_NAME_FULLY_QUALIFIED, but the check only handles T_STRING, so namespaced plugins can bypass the check.

2. The implementation flags hook-paired buffers even when the open/close callbacks are immediately adjacent and statically obvious. Issue Unclosed ob_start() usage #1314 says those should not be considered issues, so this needs either support or a clarified scope.

3. The warning docs URL points to the old WordPress 5.7 template roadmap post, while the message recommends the WordPress 6.9 template enhancement output buffer. Please update the link to the 6.9 field-guide resource referenced in the issue.

[westonruter]

Could this be implemented as a PHPCS Sniff instead? Since it's walking over the tokens in the source file, this would seem more appropriate.

[faisalahammad]

Hi @davidperezgar @westonruter, thanks for the feedback. I reworked the check based on it.

Main change: following @westonruter's suggestion, I moved the logic out of the custom token_get_all() walker and into a real PHPCS sniff (PluginCheck.CodeAnalysis.UnclosedObStart). The check class now extends Abstract_PHP_CodeSniffer_Check, like the other sniff-based checks. This drops the hand-rolled brace-depth tracking and lets PHPCS handle scope via its conditions data.

The three issues from @davidperezgar:

1. Fully-qualified \ob_start() — the sniff now resolves the function name from both the T_NS_SEPARATOR + T_STRING form (PHP 7.4) and the single T_NAME_FULLY_QUALIFIED token (PHP 8+), guarded with defined() so 7.4 stays happy. Namespaced \ob_start() is no longer skipped.

2. Hook-paired buffers — per issue Unclosed ob_start() usage #1314, an ob_start() directly followed by add_action()/add_filter() is no longer flagged. This is intentionally narrow: only an immediately adjacent hook registration suppresses the warning, so we don't mask genuinely unclosed buffers. I kept it conservative because statically proving the callback closes the buffer is out of reach. If you'd prefer a wider or different signal here, happy to adjust.

3. Docs URL — updated to the WordPress 6.9 Frontend Performance Field Guide (the template enhancement output buffer), in both get_documentation_url() and the per-message link.

Pairing logic: a close only pairs with a start when they share the same nesting context, so a buffer closed only inside an if is still flagged. The sniff unit test covers the five cases from #1314 plus the FQN and hook-paired ones.

One thing I want your opinion on: the hook-paired heuristic is the softest part. I went with "next statement is add_action/add_filter" as the statically obvious signal, but I can see arguments for leaving it out entirely (flag everything, let the author // phpcs:ignore) or making it smarter. What do you think fits the project best?

If you'd rather keep the original token_get_all() approach, that's fine too — I can revert to it and just apply the three fixes there. Just let me know which direction you prefer.

[davidperezgar]

@faisalahammad The check is missing CATEGORY_PLUGIN_REPO in get_categories(), which means it won't run in the Plugin Directory UI by default (only when explicitly requested via --checks=unclosed_ob_start).

Other Performance checks that also apply to the plugin directory follow the same pattern of returning both categories. For example, Enqueued_Resources_Check:

public function get_categories() {
    return array(
        Check_Categories::CATEGORY_PLUGIN_REPO,
        Check_Categories::CATEGORY_PERFORMANCE,
    );
}

Please update Unclosed_Ob_Start_Check::get_categories() to include CATEGORY_PLUGIN_REPO alongside CATEGORY_PERFORMANCE.

[faisalahammad]

Hi @davidperezgar, pushed changes addressing your feedback:

1. CATEGORY_PLUGIN_REPO — added to get_categories() so the check runs by default in Plugin Directory UI. Added test_get_categories() to verify both categories.

2. Docs URL — DRYed add_result_message_for_file to use $this->get_documentation_url() (WordPress 6.9 field guide) when no docs URL is passed, so the link is consistent across all messages.

3. @SInCE tags — bumped from 1.4.0 to 2.1.0 across the check class and sniff to match the next release version.

Quality gates (PHPCS + PHPStan) pass clean. PR description updated to reflect the PHPCS sniff implementation. Ready for re-review when you have time.

[westonruter]

• This PR was created without the help of AI tools

Is this accurate?

[faisalahammad]

• This PR was created without the help of AI tools

Is this accurate?

Sorry, I unchecked the checkbox. Is there anything else I should fix before merging? Thank you.

[westonruter]

@faisalahammad then it seems you need to check this checkbox instead:

This PR includes AI-assisted code or content

Right? If so then explain how the tools were used, per the description:

If AI tools were used, please describe how they were used:

[faisalahammad]

Done. Checked the AI-assisted box and described Claude Code's role: PHPCS sniff architecture research, scope-tracking logic, PHP 7.4/8.0 FQN handling, PHPUnit fixture scenarios, PR body draft.

Could you please check again and share your feedback, @westonruter?

[davidperezgar]

LGTM

[davidperezgar]

@mukeshpanchal27 ?

[ernilambar]

@faisalahammad Thanks for working on the PR. Please resolve the merge conflicts and PR will be ready to ship.

[faisalahammad]

Merge conflicts resolved. Merged trunk into this branch and fixed the one conflict in includes/Checker/Default_Check_Repository.php (array key collision with the newly-added ai_provider check, kept both entries). ruleset.xml merged clean.

Tests pass after merge: PHPCS lint (0 errors), PHPStan (0 errors), PHPUnit (482 tests, 1407 assertions, all green). Branch is now mergeable with trunk.

[Copilot]

Pull request overview

Adds a new static performance check to Plugin Check to detect ob_start() calls that are not reliably closed within the same logical scope, implemented via a dedicated PHPCS sniff and surfaced through the existing Abstract_PHP_CodeSniffer_Check infrastructure.

Changes:

• Introduces PluginCheck.CodeAnalysis.UnclosedObStart PHPCS sniff to track ob_start() and matching close calls per scope (with a conservative hook-adjacent suppression heuristic).
• Registers a new unclosed_ob_start check so the sniff runs by default and is categorized under both Performance and Plugin Repo checks.
• Adds PHPUnit coverage (plugin-check check runner) and PHPCS sniff unit tests with fixtures for paired/unpaired/conditional/FQN/hook-adjacent scenarios.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
includes/Checker/Checks/Performance/Unclosed_Ob_Start_Check.php	New PHPCS-backed check wrapper exposing the sniff through Plugin Check.
includes/Checker/Default_Check_Repository.php	Registers the new unclosed_ob_start check in the default repository.
phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/UnclosedObStartSniff.php	Implements the new sniff logic for detecting unclosed output buffers by scope.
phpcs-sniffs/PluginCheck/ruleset.xml	Enables the new sniff in the PluginCheck coding standard ruleset.
phpcs-sniffs/PluginCheck/Tests/CodeAnalysis/UnclosedObStartUnitTest.php	Adds PHPCS unit test expectations for the new sniff.
phpcs-sniffs/PluginCheck/Tests/CodeAnalysis/UnclosedObStartUnitTest.inc	Adds PHPCS fixture code covering the key detection scenarios.
tests/phpunit/tests/Checker/Checks/Unclosed_Ob_Start_Check_Tests.php	Adds PHPUnit test validating Plugin Check surfaces warnings from the sniff with correct locations.
tests/phpunit/testdata/plugins/test-plugin-unclosed-ob-start/load.php	Adds plugin fixture used by the PHPUnit test for real-world scenarios.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[faisalahammad]

@mukeshpanchal27 T_FN scope gap verified in commit a8a673b. get_scope_pointer() now treats T_FN (arrow functions) as a scope alongside T_FUNCTION and T_CLOSURE.

Test cases added:

Sniff unit test (UnclosedObStartUnitTest.inc):

• Line 45: $arrow_unpaired = fn() => ob_start(); — flagged
• Line 48: $arrow_paired = static fn() => ob_start() && ob_get_clean(); — not flagged

PHPUnit integration test (load.php):

• Line 56: arrow function with unpaired ob_start() — flagged
• Line 59: arrow function with paired ob_start() / ob_get_clean() — not flagged

Manual sniff run on the fixture produces 4 warnings on lines 14, 17, 29, 45 (T_FN included). Sniff unit test passes 16/16.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

[faisalahammad]

CI Fix — PHP (Sniffs) resolved

The PHP (Sniffs) check failed on a self-lint of the new sniff file. Two Generic.Formatting.MultipleStatementAlignment.NotSameWarning warnings on the assignment block, and check-cs exits non-zero on warnings.

File	Rule	Fix
phpcs-sniffs/.../UnclosedObStartSniff.php L141, L143	MultipleStatementAlignment	Align = to longest LHS (PHPCBF)

Whitespace only, no logic change.

check-cs pass (exit 0) - sniff tests 16/16 - PHP 8.0 compatible.