Ensure compat functions do not rely on external functions

[jorbin]

This is a follow up to #61680.

compat.php is loaded very early on and its functions need to be standalone without dependency on functions in the rest of the codebase.

I think that there are two improvements to be made:

1. The top of compat.php should include some documentation to remind future devs of this
2. We could add some testing of the functions to ensure they have no reliance on external functions (or if there are external functions, they are included properly and those are standalone such as with the sodium functions)

[joemcgill]

Thanks for opening this issue. I agree with the two improvements you suggest and suggest a third, which is to review the current functions defined in compat.php and resolve any current dependencies on external functions that may not be loaded when these functions are called.

Currently, this includes calls to get_option() in _mb_substr() and _mb_strlen(), but previously included calls to is_utf8_charset(). This was fixed in [58763] by splitting the stateless logic into an internal function that is included in compat.php directly. For uses of get_option() there really isn't any stateless logic that could be split, so a different approach is needed, like wrapping calls to internal functions in try/catch blocks or function_exists() checks to avoid potential fatals.

Alternately, we could take the stance that these compat functions are only meant to support WP Core's own usage of the functions being pollyfilled when not available at the platform level and allow potential fatals triggered by third party code to be handled by those third parties. I'm not advocating for this stance, but do think it's a valid option that should be discussed.

[dmsnell]

I'm not proficient in writing Github Actions, but this code reasonably guesses if there's a call to a function not-yet-defined in compat.php. It could be expanded without much work to capture function_exists() checks.

<?php
$compat_path = __DIR__ . '/src/wp-includes/compat.php';
require_once $compat_path;

$functions = get_defined_functions();
$tokens    = token_get_all( file_get_contents( $compat_path ) );
$last_i    = count( $tokens ) -  1

foreach ( $tokens as $i => $token ) {
        // A function call looks like [ T_STRING function_name, '(' ]
        if ( is_string( $token ) || $i === $last_i || 'T_STRING' !== token_name( $token[0] ) || '(' !== $tokens[ $i + 1 ] ) {
                continue;
        }

        $name = $token[1];
        if ( ! in_array( $name, $functions['internal'], true ) && ! in_array( $name, $functions['user'], true ) ) {
                echo "Possible call to undefined function '{$name}' on line {$token[2]}\n";
        }
}

When run against the existing compat.php it shows

Possible call to undefined function 'get_option' on line 124
Possible call to undefined function 'get_option' on line 209

For checking more cases we could pull in Nikic/parser but I think this could be enough to help. We could create a comment on a PR that changes this file with possible calls.

I'm not worried about malicious intent here; and I wouldn't want to propose blatantly rejecting code based on naive parsing and understanding of it, but a comment on my PR automatically generated which could have said "did you realize this?" would have prevented this from happening.

Trac ticket:

[jorbin]

First pass on converting @dmsnell's script into an action is up at ​https://github.com/WordPress/wordpress-develop/pull/7090 and you can see a failing run at ​https://github.com/WordPress/wordpress-develop/actions/runs/10097769344/job/27923356283

If we want to use this we will need to remove the get_option calls or otherwise silence them.

Is it possible to do this as a custom sniff rather than a separate action?

That will allow the errors to display in IDEs rather than relying on contributors pushing PRs which may not always happen. There is some prior art for custom sniffs in the Gutenberg Repo ​https://github.com/WordPress/gutenberg/tree/trunk/test/php/gutenberg-coding-standards

I'm not sure it's worth putting in the main WPCS repo (others may have a different view)

[jorbin]

@jrf Are you aware of any existing sniffs that check to see if a file only contains either functions defined in the file or functions in the PHP standard library so that we don't need to duplicate work?

[jrf]

Replying to jorbin:

@jrf Are you aware of any existing sniffs that check to see if a file only contains either functions defined in the file or functions in the PHP standard library so that we don't need to duplicate work?

@jorbin No, I'm not aware of any such sniff existing. Would be quite doable to write in principle, but will never do exactly what you want - get_defined_functions()['internal'] (which such a sniff would use too) will get you all the functions defined at runtime of the sniff and has no information on whether the function is defined in the PHP standard/Core library, in a bundled, always-on PHP extension or in an optional or even a PECL extension which happens to be enabled in the runtime running the sniff/script.

I don't think adding sniffs to WP itself (or Gutenberg) is a good idea (at all, ever).
Adding it to WPCS is also not an option, as that would automatically enforce the rule on all files being scanned by WPCS, not just on the compat.php file, which would have undesired consequences.

A sniff like this could live in the PHPCSExtra library, which is used by WPCS anyway. This also means that the sniff could then be used by WP Core via an explicit rule include in the phpcs.xml.dist file (and applied to specific file(s) only).
I have a utility lined up for PHPCSUtils 1.2.0 which will make writing the sniff straight-forward (and reliable).

If so desired, knowing the limitations, please open an issue in PHPCSExtra.

[jorbin]

I've opened ​https://github.com/PHPCSStandards/PHPCSExtra/issues/315 so that this sniff can be created.

Keeping this open so that the sniff can be added to WP after it exists.

[jorbin]

Based on the upstream discussion making it not likely we can get a sniff in quickly, I think in the short term we should add some documentation at the top of the file. It's not perfect, but it adds a guide for folks working in there.

[jorbin]

Attached a first pass at the documentation, if there are any edits please post them.

[dmsnell]

This new comment @jorbin seems like a non-controversial help. There might be other changes that could further improve the resilience of that file, but the comment should be a valuable step on its own.

[dmsnell]

Perhaps there could be additional word-smithing to clarify some of the jargon:

This Ffile is loaded before WordPress has been initialized (before {@see functions.php}.
Ergo, please ensure you do not call functions from this file defined later in the boot process.
Only use functions in this file which are built into all supported PHP versions or otherwise
defined within this file.

This despite the fact that other function calls are already called, but that's the nature of legacy code.

[helen]

In 59043:

Bootstrap/Load: Give more context and warning about editing compat.php.

As indicated by name, this is a compatibility file which warrants more care to begin with, but it's still worth warning folks about how narrow function availability is in this file.

Props jorbin, dmsnell, helen.
See #61694.

[peterwilsoncc]

Ticket/report maintenance pending the upstream changes to the coding standards library. If the sniff becomes available during the 6.7 cycle please feel free to move this back on the milestone.