diff --git a/includes/Checker/Checks/Performance/Unclosed_Ob_Start_Check.php b/includes/Checker/Checks/Performance/Unclosed_Ob_Start_Check.php new file mode 100644 index 000000000..7215375e6 --- /dev/null +++ b/includes/Checker/Checks/Performance/Unclosed_Ob_Start_Check.php @@ -0,0 +1,105 @@ + 'php', + 'standard' => 'PluginCheck', + 'sniffs' => 'PluginCheck.CodeAnalysis.UnclosedObStart', + ); + } + + /** + * Gets the description for the check. + * + * Every check must have a short description explaining what the check does. + * + * @since 2.1.0 + * + * @return string Description. + */ + public function get_description(): string { + return __( 'Detects calls to ob_start() that are not paired with a corresponding buffer-closing function within the same logical scope.', 'plugin-check' ); + } + + /** + * Gets the documentation URL for the check. + * + * Every check must have a URL with further information about the check. + * + * @since 2.1.0 + * + * @return string The documentation URL. + */ + public function get_documentation_url(): string { + return __( 'https://make.wordpress.org/core/2025/11/18/wordpress-6-9-frontend-performance-field-guide/#introduce-the-template-enhancement-output-buffer', 'plugin-check' ); + } + + /** + * Amends the given result with a message for the specified file, including the + * documentation URL for this check. + * + * @since 2.1.0 + * + * @param Check_Result $result The check result to amend, including the plugin context to check. + * @param bool $error Whether it is an error or notice. + * @param string $message Error message. + * @param string $code Error code. + * @param string $file Absolute path to the file where the issue was found. + * @param int $line The line on which the message occurred. Default is 0 (unknown line). + * @param int $column The column on which the message occurred. Default is 0 (unknown column). + * @param string $docs URL for further information about the message. + * @param int $severity Severity level. Default is 5. + */ + protected function add_result_message_for_file( Check_Result $result, $error, $message, $code, $file, $line = 0, $column = 0, string $docs = '', $severity = 5 ) { + if ( '' === $docs ) { + $docs = $this->get_documentation_url(); + } + + parent::add_result_message_for_file( $result, $error, $message, $code, $file, $line, $column, $docs, $severity ); + } +} diff --git a/includes/Checker/Default_Check_Repository.php b/includes/Checker/Default_Check_Repository.php index c1b7d420c..c12f579e3 100644 --- a/includes/Checker/Default_Check_Repository.php +++ b/includes/Checker/Default_Check_Repository.php @@ -103,6 +103,7 @@ private function register_default_checks() { 'direct_file_access' => new Checks\Plugin_Repo\Direct_File_Access_Check(), 'external_admin_menu_links' => new Checks\Plugin_Repo\External_Admin_Menu_Links_Check(), 'wp_functions_compatibility' => new Checks\Plugin_Repo\WP_Functions_Compatibility_Check(), + 'unclosed_ob_start' => new Checks\Performance\Unclosed_Ob_Start_Check(), ) ); diff --git a/phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/UnclosedObStartSniff.php b/phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/UnclosedObStartSniff.php new file mode 100644 index 000000000..8f704b14a --- /dev/null +++ b/phpcs-sniffs/PluginCheck/Sniffs/CodeAnalysis/UnclosedObStartSniff.php @@ -0,0 +1,402 @@ + + */ + private $closing_functions = array( + 'ob_get_clean' => true, + 'ob_end_clean' => true, + 'ob_get_flush' => true, + 'ob_end_flush' => true, + ); + + /** + * Scope-keyed collection of ob_start() and closing calls for the current file. + * + * Populated during the token walk and evaluated once the file has been + * fully processed. + * + * @since 2.1.0 + * + * @var array, closes: array}> + */ + private $scopes = array(); + + /** + * Ordered list of scope start pointers, used to process scopes in source order. + * + * @since 2.1.0 + * + * @var array + */ + private $scope_order = array(); + + /** + * Whether the current file has been finalized (scopes evaluated). + * + * @since 2.1.0 + * + * @var bool + */ + private $finalized = false; + + /** + * Returns an array of tokens this test wants to listen for. + * + * @since 2.1.0 + * + * @return array + */ + public function register() { + $tokens = array( T_STRING ); + + // T_NAME_FULLY_QUALIFIED was introduced in PHP 8.0 for namespaced names like \ob_start(). + if ( defined( 'T_NAME_FULLY_QUALIFIED' ) ) { + $tokens[] = constant( 'T_NAME_FULLY_QUALIFIED' ); + } + + return $tokens; + } + + /** + * Processes this test, when one of its tokens is encountered. + * + * @since 2.1.0 + * + * @param int $stackPtr The position of the current token in the stack. + * @return int|void Integer stack pointer to skip forward or void to continue normal file processing. + */ + public function process_token( $stackPtr ) { + $name = $this->get_global_function_name( $stackPtr ); + if ( null === $name ) { + $this->maybe_finalize( $stackPtr ); + return; + } + + $lower_name = strtolower( $name ); + + if ( 'ob_start' === $lower_name ) { + $this->record_call( $stackPtr, true ); + } elseif ( isset( $this->closing_functions[ $lower_name ] ) ) { + $this->record_call( $stackPtr, false ); + } + + $this->maybe_finalize( $stackPtr ); + } + + /** + * Finalizes the file once the last registered token has been processed. + * + * PHPCS only invokes process_token() for tokens returned by register(), so the + * final file token (often trailing whitespace) is never visited. This finalizes as + * soon as no further registered tokens remain ahead, ensuring all collected calls + * are available before pairing. + * + * @since 2.1.0 + * + * @param int $stackPtr The position of the current token in the stack. + */ + private function maybe_finalize( $stackPtr ) { + if ( $this->finalized ) { + return; + } + + $next = $this->phpcsFile->findNext( $this->register(), ( $stackPtr + 1 ) ); + if ( false !== $next ) { + return; + } + + $this->finalize_file(); + } + + /** + * Resolves the global function name being called at the given pointer, or null if + * the token is not a global call to a tracked function. + * + * Handles both the legacy T_STRING + T_NS_SEPARATOR sequence (PHP < 8.0) and the + * single T_NAME_FULLY_QUALIFIED token (PHP >= 8.0) so that \ob_start() in namespaced + * code is detected consistently. + * + * @since 2.1.0 + * + * @param int $stackPtr The position of the current token in the stack. + * @return string|null The lowercased function name, or null when not a global call. + */ + private function get_global_function_name( $stackPtr ) { + $token_code = $this->tokens[ $stackPtr ]['code']; + + $name = null; + + if ( T_STRING === $token_code ) { + $previous = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $stackPtr - 1 ), null, true ); + if ( false === $previous ) { + return null; + } + + $previous_code = $this->tokens[ $previous ]['code']; + + $disallowed_previous = array( T_FUNCTION, T_NEW, T_OBJECT_OPERATOR, T_DOUBLE_COLON ); + + // T_NULLSAFE_OBJECT_OPERATOR (?->) was introduced in PHP 8.0. + if ( defined( 'T_NULLSAFE_OBJECT_OPERATOR' ) ) { + $disallowed_previous[] = constant( 'T_NULLSAFE_OBJECT_OPERATOR' ); + } + + // Skip method calls, constructors and function declarations. + if ( in_array( $previous_code, $disallowed_previous, true ) ) { + return null; + } + + // Handle the legacy \Namespace\func() two-token form. + if ( T_NS_SEPARATOR === $previous_code ) { + $before_ns = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, ( $previous - 1 ), null, true ); + if ( false !== $before_ns ) { + $before_code = $this->tokens[ $before_ns ]['code']; + if ( in_array( $before_code, array( T_STRING, T_NAMESPACE ), true ) ) { + return null; // Namespaced call, not a global one. + } + } + } + + $name = $this->tokens[ $stackPtr ]['content']; + } elseif ( defined( 'T_NAME_FULLY_QUALIFIED' ) && constant( 'T_NAME_FULLY_QUALIFIED' ) === $token_code ) { + // PHP >= 8.0: \ob_start() tokenizes as a single T_NAME_FULLY_QUALIFIED token. + $name = ltrim( $this->tokens[ $stackPtr ]['content'], '\\' ); + } + + if ( null === $name ) { + return null; + } + + // Must be followed by an opening parenthesis to be a function call. + $next = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $stackPtr + 1 ), null, true ); + if ( false === $next || T_OPEN_PARENTHESIS !== $this->tokens[ $next ]['code'] ) { + return null; + } + + return $name; + } + + /** + * Records an ob_start() or closing call against its enclosing scope. + * + * The file scope is represented by a pointer of -1. Function, method and closure + * scopes use the pointer of their opening condition token. + * + * @since 2.1.0 + * + * @param int $stackPtr The position of the call. + * @param bool $is_start Whether this is an ob_start() (true) or a closing call (false). + */ + private function record_call( $stackPtr, $is_start ) { + $scope_ptr = $this->get_scope_pointer( $stackPtr ); + + if ( ! isset( $this->scopes[ $scope_ptr ] ) ) { + $this->scopes[ $scope_ptr ] = array( + 'starts' => array(), + 'closes' => array(), + ); + $this->scope_order[] = $scope_ptr; + } + + $line = $this->tokens[ $stackPtr ]['line']; + $conditions = isset( $this->tokens[ $stackPtr ]['conditions'] ) ? $this->tokens[ $stackPtr ]['conditions'] : array(); + + if ( $is_start ) { + $this->scopes[ $scope_ptr ]['starts'][] = array( + 'ptr' => $stackPtr, + 'line' => $line, + 'conditions' => $conditions, + ); + } else { + $this->scopes[ $scope_ptr ]['closes'][] = array( + 'ptr' => $stackPtr, + 'line' => $line, + 'conditions' => $conditions, + ); + } + } + + /** + * Gets the scope pointer that owns the token at the given position. + * + * Returns the pointer of the nearest enclosing function, method or closure, or -1 + * for the file (top-level) scope. + * + * @since 2.1.0 + * + * @param int $stackPtr The position of the token. + * @return int The scope pointer, or -1 for the file scope. + */ + private function get_scope_pointer( $stackPtr ) { + if ( empty( $this->tokens[ $stackPtr ]['conditions'] ) ) { + return -1; + } + + $conditions = array_reverse( $this->tokens[ $stackPtr ]['conditions'], true ); + foreach ( $conditions as $condition_ptr => $condition_code ) { + if ( in_array( $condition_code, array( T_FUNCTION, T_CLOSURE ), true ) ) { + return $condition_ptr; + } + } + + return -1; + } + + /** + * Evaluates collected scopes and reports unpaired ob_start() calls. + * + * Called once when the end of the file is reached so that the full scope contents + * are available for pairing. + * + * @since 2.1.0 + */ + private function finalize_file() { + $this->finalized = true; + + foreach ( $this->scope_order as $scope_ptr ) { + $scope = $this->scopes[ $scope_ptr ]; + if ( empty( $scope['starts'] ) ) { + continue; + } + + $unpaired = $this->pair_starts_and_closes( $scope['starts'], $scope['closes'] ); + + foreach ( $unpaired as $start ) { + if ( $this->is_hook_paired( $start['ptr'] ) ) { + continue; + } + + $this->phpcsFile->addWarning( + 'ob_start() was found without a corresponding closing call (ob_get_clean(), ob_end_clean(), ob_get_flush() or ob_end_flush()) in the same scope. Output buffering is a valid technique, but a buffer must not be left open. WordPress is a shared environment where core, themes and other plugins may also open or close buffers, and a misaligned buffer stack causes unpredictable behaviour (headers already sent, lost output, broken redirects, etc.). Please ensure every ob_start() is paired with a closing function within the same function scope, and that nothing (including hooks or early returns) can bypass that closing logic. If you need to modify the full response output, use the new template enhancement output buffer available since WordPress 6.9.', + $start['ptr'], + 'UnclosedObStart' + ); + } + } + + // Reset state so the sniff can be reused across multiple files. + $this->scopes = array(); + $this->scope_order = array(); + $this->finalized = false; + } + + /** + * Pairs ob_start() calls with closing calls within the same scope. + * + * A closing call pairs with the most recent unpaired ob_start() that appears before + * it in source order and that sits at the same nesting level (identical set of + * enclosing conditions). This ensures a buffer that is only closed conditionally, + * for example inside an if block that may not execute, is treated as unpaired and + * flagged. + * + * ob_start() calls that remain after all closes have been consumed are returned as + * unpaired. + * + * @since 2.1.0 + * + * @param array $starts The ob_start() calls. + * @param array $closes The closing calls. + * @return array The unpaired ob_start() calls. + */ + private function pair_starts_and_closes( array $starts, array $closes ) { + $unpaired = $starts; + + foreach ( $closes as $close ) { + for ( $i = ( count( $unpaired ) - 1 ); $i >= 0; $i-- ) { + $start = $unpaired[ $i ]; + + if ( $close['ptr'] <= $start['ptr'] ) { + continue; + } + + // Only pair when both calls share the exact same nesting context, so a + // close inside a conditional block does not silently pair with a start + // at the enclosing level. + if ( $this->same_nesting( $start['conditions'], $close['conditions'] ) ) { + array_splice( $unpaired, $i, 1 ); + break; + } + } + } + + return $unpaired; + } + + /** + * Determines whether two condition sets represent the same nesting level. + * + * Compares the full set of enclosing condition pointers and their token codes, so + * that two calls in different conditional branches (same depth, different blocks) + * are not treated as nested identically. + * + * @since 2.1.0 + * + * @param array $a The first set of conditions (token pointer => token code). + * @param array $b The second set of conditions (token pointer => token code). + * @return bool True when both calls sit at the same nesting level. + */ + private function same_nesting( array $a, array $b ) { + return $a === $b; + } + + /** + * Best-effort heuristic for hook-paired buffers that should not be flagged. + * + * Per issue #1314, an ob_start() that is intentionally closed via a hook callback + * registered immediately next to it, in a way that is statically obvious, should + * not be considered an issue. This checks whether the statement immediately + * following the ob_start() is a call to add_action() or add_filter(), which is the + * statically obvious signal that the buffer is managed by a hooked callback. + * + * This is intentionally conservative: it only suppresses the warning when the hook + * registration directly follows the ob_start() call, to avoid masking genuinely + * unclosed buffers. Resolving the registered callback and verifying it closes the + * buffer is out of scope for static analysis. + * + * @since 2.1.0 + * + * @param int $start_ptr The position of the ob_start() call. + * @return bool True when the buffer appears to be closed via an adjacent hook callback. + */ + private function is_hook_paired( $start_ptr ) { + $next_statement = $this->phpcsFile->findEndOfStatement( $start_ptr ); + if ( false === $next_statement ) { + return false; + } + + $hook_ptr = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $next_statement + 1 ), null, true ); + if ( false === $hook_ptr ) { + return false; + } + + $name = $this->get_global_function_name( $hook_ptr ); + if ( null === $name ) { + return false; + } + + return in_array( strtolower( $name ), array( 'add_action', 'add_filter' ), true ); + } +} diff --git a/phpcs-sniffs/PluginCheck/Tests/CodeAnalysis/UnclosedObStartUnitTest.inc b/phpcs-sniffs/PluginCheck/Tests/CodeAnalysis/UnclosedObStartUnitTest.inc new file mode 100644 index 000000000..8a5b4df3b --- /dev/null +++ b/phpcs-sniffs/PluginCheck/Tests/CodeAnalysis/UnclosedObStartUnitTest.inc @@ -0,0 +1,42 @@ + Key is the line number and value is the number of expected errors. + */ + public function getErrorList() { + return array(); + } + + /** + * Returns the lines where warnings should occur. + * + * @return array Key is the line number and value is the number of expected warnings. + */ + public function getWarningList() { + return array( + 14 => 1, // Case: ob_start() at file scope with no closing call. + 17 => 1, // Case: multiple ob_start() in the same scope, only one closed. + 29 => 1, // Case: ob_start() closed only conditionally via an if block. + ); + } + + /** + * Returns the fully qualified class name (FQCN) of the sniff. + * + * @return string The fully qualified class name of the sniff. + */ + protected function get_sniff_fqcn() { + return UnclosedObStartSniff::class; + } + + /** + * Sets the parameters for the sniff. + * + * @throws \RuntimeException If unable to set the ruleset parameters required for the test. + * + * @param Sniff $sniff The sniff being tested. + */ + public function set_sniff_parameters( Sniff $sniff ) { + } +} diff --git a/phpcs-sniffs/PluginCheck/ruleset.xml b/phpcs-sniffs/PluginCheck/ruleset.xml index 047e63acf..3abdeaab3 100644 --- a/phpcs-sniffs/PluginCheck/ruleset.xml +++ b/phpcs-sniffs/PluginCheck/ruleset.xml @@ -10,6 +10,7 @@ + diff --git a/tests/phpunit/testdata/plugins/test-plugin-unclosed-ob-start/load.php b/tests/phpunit/testdata/plugins/test-plugin-unclosed-ob-start/load.php new file mode 100644 index 000000000..3b66517bc --- /dev/null +++ b/tests/phpunit/testdata/plugins/test-plugin-unclosed-ob-start/load.php @@ -0,0 +1,53 @@ +get_categories(); + + $this->assertIsArray( $categories ); + $this->assertContains( Check_Categories::CATEGORY_PERFORMANCE, $categories ); + $this->assertContains( Check_Categories::CATEGORY_PLUGIN_REPO, $categories ); + $this->assertCount( 2, $categories ); + } + + public function test_run_with_errors() { + $check = new Unclosed_Ob_Start_Check(); + $check_context = new Check_Context( UNIT_TESTS_PLUGIN_DIR . 'test-plugin-unclosed-ob-start/load.php' ); + $check_result = new Check_Result( $check_context ); + + $check->run( $check_result ); + + $warnings = $check_result->get_warnings(); + + $this->assertNotEmpty( $warnings ); + $this->assertArrayHasKey( 'load.php', $warnings ); + $this->assertEquals( 3, $check_result->get_warning_count() ); + + // 14: ob_start() at the top of the file with no closing call. + $this->assertArrayHasKey( 14, $warnings['load.php'] ); + $this->assertEquals( 'PluginCheck.CodeAnalysis.UnclosedObStart.UnclosedObStart', $warnings['load.php'][14][1][0]['code'] ); + + // 18: Multiple ob_start() in the same scope, only one closed. + $this->assertArrayHasKey( 18, $warnings['load.php'] ); + $this->assertEquals( 'PluginCheck.CodeAnalysis.UnclosedObStart.UnclosedObStart', $warnings['load.php'][18][2][0]['code'] ); + + // 32: ob_start() inside a function, closed only conditionally (if). + $this->assertArrayHasKey( 32, $warnings['load.php'] ); + $this->assertEquals( 'PluginCheck.CodeAnalysis.UnclosedObStart.UnclosedObStart', $warnings['load.php'][32][2][0]['code'] ); + } +}