Fatal error: PHP class_exists() bail out at top of class files is not robust across PHP versions.

[hellofromtonya]

Description

Problem Statement

The current class_exists() guard at the top of the plugin's class files is not adequate enough to avoid name conflict fatal errors across all WP supported PHP versions.

A Fatal error: Cannot declare class can happen on PHP 7.0, 7.1, and 7.3.

More Context

A fatal error happened today on ma.tt (which runs on WP trunk) after packages were updated in Core yesterday:

PHP Fatal error: Cannot declare class WP_Navigation_Block_Renderer, because the name is already in use in plugins/gutenberg/lib/compat/wordpress-6.5/class-wp-navigation-block-renderer.php on line 16

The WP_Navigation_Block_Renderer file had a class_exists() guard at the top to bail out if that class is already loaded into memory (i.e. that class exists in Core). That guard works on PHP 7.4+, but does not on 7.0, 7.1, and 7.3.

Step-by-step reproduction instructions

1. Set up your environment to use PHP 7.4 or greater with WordPress 6.4 or trunk.
2. For testing convenience, use this gist to add a tester plugin to your test site. Activate the plugin.
3. View the front-end of the test site. It should work ✅
4. Change the environment to use PHP 7.0, 7.1, or 7.3.
5. Refresh your browser. A fatal error should happen 🐞 ❌

Screenshots, screen recording, code snippet

Environment info

• WordPress 6.4 or trunk
• PHP 7.4+ will work; PHP 7.0, 7.1, or 7.3 will fatal error
• For convenience, using the test plugin (see above).

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[hellofromtonya]

This issue has happened before, e.g. see #54103.

[hellofromtonya]

Possible solutions are:

Approach 1: Move the class and all of its code inside of the class_exists() like this:

if ( ! class_exists( 'WP_Font_Face' ) ) {
	class WP_Font_Face {
		// all of its code goes here.
	}
}

Advantages:

• Easier to catch with a sniff per @anton-vlasenko

I'm thinking of changing the linter to only allow wrapping the class within the class_exists() check. I can submit a PR for that.

• No fatal errors for any theme, plugin, and/or script that attempts to load the class file directly - the guard is internal to the file.
• Same performance as today - no performance gains.

Disadvantages:

• Harder to read the code - one more code shift to the right.
• Files must be modified at merge into Core time:
  When it's time to sync / merge the code into WordPress Core, the file will require modification to remove the guard wrapper. It's an extra step in the PHP sync workflows. This step can be avoided with the next suggestion.

Approach 2: Move the class_exist() to wrap around the class file's require in the lib/load.php:

if ( ! class_exists( 'WP_Font_Face' ) ) {
	require path/to/the/class/file.php;
}

This design was used in PR #54103.

Advantages:

• More performant.
  Less files to load into memory.
• Less work at merge into Core time due to no class file modifications.
  The class file will not need modification when it's time to introduce the class into WordPress Core. No modification can mean less errors and work at merge into Core time. Copy/paste instead of modify the file and then copy/paste.

Disadvantages:

• May be more changing to build sniff to catch it discussing with @anton-vlasenko :

Catching this with a linter will be challenging, but not impossible.

• Extender fatal error.
  A fatal error can happen if a theme, plugin, and/or script attempts to directly load the class file into memory without guarding it in their code.

My suggestion?

I'd suggest changing all instances to Approach 2, prevent the class file from loading into memory.

[azaozz]

Just to clarify, using a return in the global scope in a PHP file prevents execution of the code after it. From the PHP manual:

If (return) called from the global scope, then execution of the current script file is ended. If the current script file was included or required, then control is passed back to the calling file.

However by the time PHP "executes" that return the file has already been read and processed and (in some PHP versions) functions and classes that are defined in the file are already available. From a comment in the PHP manual:

Note that because PHP processes the file before running it, any functions defined in an included file will still be available, even if the file is not executed.

[ironprogrammer]

+1 preference for Approach 2. This is an established pattern for handling this situation (e.g. WP_Font_Face and WP_HTML_Processor).

Minimizing code changes when syncing to Core and not needing to modify the class file better supports that this code is Core code waiting for merge, and I'm sure the smaller diff would be welcome to reviewers and committers.

[anton-vlasenko]

In my opinion, approaches 1 and 2 do not necessarily have to conflict with each other. Approach 1 is already implemented; it is simply necessary to prohibit the use of the check ! class_exist() at the beginning of the file. Approach 2 can be implemented alongside approach 1.

[getdave]

I've raised #58454 which can act as a stub for documenting whichever route we approve

[gziolo]

I'm in favor of Approach 2 as it drastically simplifies syncing code between the Gutenberg plugin and WordPress core. It also makes the review process less intense.

[hellofromtonya]

Heads up: A PR and commit was done yesterday that implements Approach 1.

That approach can be iterated upon if Approach 2, which is currently the favorite so far, is selected instead.

[azaozz]

Seems everybody prefers approach 2, another +1 for it. Lest just do it :)

The require calls for the WP_HTML_Processor are a good example.

[anton-vlasenko]

Let me reflect on what happened over the last 3 days:
Approach 1 had already been implemented even before this GitHub issue was created. A PR and commit made today removed the ability to guard classes like:

if ( class_exists( 'WP_Class' ) ) {
    // Return early to avoid loading the class.
    // Yes, this approach doesn't always work.
    return;
}

The only way to guard them now is to wrap them in:

if ( ! class_exists( 'WP_Class' ) ) {
   // class implementation
}

Again, this was already in place; the PR simply disabled the "return early" approach that was possible before the changes introduced by #58500 (1st example above).
In the light of an upcoming Gutenberg release, there were no time to implement a more sophisticated solution, such as approach 2. Something quck had to be done to prevent fatal errors in the future.

On a personal note, I am open to approach 2 and am currently in the process of implementing it.

[sirreal]

This problem has come up before and seems to be limited to early PHP 7 versions, but I still don't understand exactly what is happening and why it's manifesting in these particular places with these particular versions. Is there a good explanation of what exactly is happening to trigger this error? I understand the redeclared class error, but why in these files and in these PHP versions? That pattern is not new and not limited to WP_Navigation_Block_Renderer, for example (from the 16.9 Gutenberg release):

gutenberg/lib/compat/wordpress-6.4/html-api/class-wp-html-processor.php

Lines 10 to 12 in 2053535

	if ( class_exists( 'WP_HTML_Processor' ) ) {
	return;
	}

I've been unable to create a case where php 7.0 and 8.3 behave differently. Here's a gist with some of the things I've been testing.

To be clear, I support these changes. I think it's an improvement to move the class check to conditionally require the files. However, I would like to have a better understanding of what is causing this problem in order to avoid it in the future.

[anton-vlasenko]

That's a very good question, @sirreal.
I can't test with early PHP versions either. However, I can reproduce the issue with functions.
If you add

function foo() {}

to the very bottom of the b.php, c.php, and unconditional.php files from your snippet, you should get a cannot redeclare foo() (previously declared in error.
So, using return doesn't stop the file from being parsed.

[sirreal]

I also suspected functions and classes were treated similarly, after reading this section of the include docs (which are referenced from the require docs):

If there are functions defined in the included file, they can be used in the main file independent if they are before return or after. If the file is included twice, PHP will raise a fatal error because the functions were already declared. It is recommended to use include_once instead of checking if the file was already included and conditionally return inside the included file.

But, as you mention, they're clearly treated differently. Swapping classes and class_exists for functions and function_exists in my example demonstrates that. A minimal example shows the "early return" form is broken with functions, but just fine with classes.

<?php
// Fatal error: Cannot redeclare f()
function f() {}
if (function_exists('f')) { return; }
function f() {}

<?php
// No errors
class C {}
if (class_exists('C')) { return; }
class C {}

All that to say, I still don't understand how these errors were caused 🙂

[anton-vlasenko]

All that to say, I still don't understand how these errors were caused 🙂

Same here, @sirreal.

I am also unable to reproduce the error. And thank you for raising this issue.
I've set up a small Docker container for testing the return operator in the context of loading classes across PHP versions 5.4-8.3, but I cannot verify that the issue is specific to PHP 7.0-7.3.
PHP behaves the same regardless of the version, and bailing out at the top of the file doesn't cause fatal errors.

Therefore, if anyone would like to test it, I suggest cloning this repository: https://github.com/anton-vlasenko/test_bail_out_using_return.

It includes a README.md file that explains how to use the docker container.

UPD: I will look into it tomorrow, as the issue could be related to a specific environment, as mentioned by @hellofromtonya.

[azaozz]

A minimal example shows the "early return" form is broken with functions, but just fine with classes.

@sirreal Hmm, seems to throw a fatal error here regardless if functions or classes, only I'm actually including a file in another file. Are you testing in PHP 7.x? Testing in 7.4.33 here.

Also tried this:

• File 1, name: incl.php, it is inwp-content/plugins:

<?php

class A {
   public function aa() {
   	echo 'A::aa() was called<br>';
   }
}

return;

class B {
   public function bb() {
   	echo 'B::bb() was called<br>';
   }
}

• File 2, name plugin.php, also in wp-content/plugins:

<?php
/**
 * Plugin Name: Private tests
 */

include plugin_dir_path( __file__ ) . '/incl.php';

$a = new A;
$a->aa();

$b = new B;
$b->bb();
exit;

Activating the Private tests plugin shows:

A::aa() was called
B::bb() was called

(The B class is defined and works properly despite that the code is after a return in the included file.)

Then changing the plugin.php file to:

class B {
	public function bbb() {
		echo 'B::bbb() was called<br>';
	}
}

include plugin_dir_path( __file__ ) . '/incl.php';

$a = new A;
$a->aa();

$b = new B;
$b->bb();
exit;

outputs:

Fatal error: Cannot declare class B, because the name is already in use in /var/www/wp-content/plugins/incl.php on line 11

[sirreal]

I've created a repo to make this easier. It's currently running PHP 7.0, 7.4, and 8.3. I can define a class, include/require another file that redeclares the same class after an early return, and there's no error. I've been trying to reproduce this in a minimal setup to better understand what's happening, so I'm invoking PHP on the command line. Could there be some difference there? Is there something special about running in WordPress or inside a plugin? I don't understand how PHP can behave so differently in some cases.

I'd really like to reach a minimal reproduction case. I understand the error, I understand how to fix it, but I still haven't seen a compelling explanation for why this happens only under very specific circumstances.

[anton-vlasenko]

@sirreal It appears that the processing of included files varies depending on the way PHP is used.
I can reproduce this issue when PHP is loaded as an Apache module, but not when using PHP CLI.
This is quite interesting and it explains what we've been experiencing.

[swissspidy]

Why isn't it an option to just prefix all those classes with Gutenberg_ when used in the plugin? Then, when the files are built for npm / core, the prefix can be changed to WP_. That should fix any such conflicts, no?

[anton-vlasenko]

Why isn't it an option to just prefix all those classes with Gutenberg_ when used in the plugin? Then, when the files are built for npm / core, the prefix can be changed to WP_. That should fix any such conflicts, no?

Thanks for bringing that up, @swissspidy.
My understanding is that certain functions and classes still require the WP_ prefix.
This is because some WP_ prefixed functions/classes must be included in Gutenberg for compatibility with older versions of WordPress.
Gutenberg needs to be compatible with the current (trunk) WordPress version as well as the two previous stable versions.
Therefore, certain WP_ prefixed classes and functions may not be present in older WordPress versions but must be included in Gutenberg.
Additionally, defining a class with the WP_ prefix assumes that the class should be backported to WordPress Core at some point. So, if all functions/classes are prefixed with Gutenberg_, it may (or may not) create some confusion.
But I find your idea intriguing, and I'd be interested in hearing other opinions.

[anton-vlasenko]

As I've started working on updating the Gutenberg.CodeAnalysis.GuardedFunctionAndClassNamesSniff sniff in line with approach #2 discussed in this ticket, I've realized several issues arise from doing so:

1. There is no guarantee that the conflicting class is loaded only from lib/load.php. It could technically be loaded from anywhere. Thus, if the class_exists() calls are moved out of the class files into lib/load.php, the sniff needs to check all Gutenberg PHP files, decide which files were included via (require|include)_once constructs and if they were properly guarded, and then disable the requirement to guard classes inside these PHP files. This significantly raises the complexity of the sniff and creates room for error, as there is no reliable way to match file paths used in (require|include)_once constructs with the actual absolute paths of the PHP files being scanned. Additionally, some places do not use file names at all, but rather variables like require_once $path, making it impossible to check these instances.

2. Addressing the issue above makes it impossible to enable parallel processing for sniffs, as pctnl_fork() spawns several processes which cannot communicate with each other (at least not with tools that are available in PHPCS/PHP by default). This makes it impossible to pass information about scanned files between the processes, thus forcing Gutenberg not to use parallel processing for sniffs. While this is not a blocker, it is a performance issue.

3. Since the minimum PHP version requirement for Gutenberg has recently been raised to 7.2, it leads me to wonder how long it will be until it is raised again to 7.4 or 8.0. Raising the PHP version to 7.4 or higher would naturally solve this issue, allowing to revert to the earlier practice of having

if ( class_exists( 'WP_Class' ) ) {
    // Return early to avoid loading the class.
    return;
}

in the class files.

In light of these concerns, I propose pausing further development until the issues I outlined above are discussed/resolved. I'd happy to hear any feedback or suggestions regarding this matter.

[swissspidy]

Can‘t we just update the PHPCS config to exclude/disable that sniff for certain files? To me that‘s what configs are for.

[anton-vlasenko]

Yes, this approach can be taken, thank you for sharing.
I'm just thinking there is no guarantee that these files will not be included in places other than load.php, which could result in fatal errors.
Therefore, all instances in the project that use (require|include)_once must be checked to ensure they are properly guarded.
However, this does not always work due to issues with parsing the file paths being included (as mentioned in item #1 of the previous comment).

Having the check inside a class file is 100% reliable and should always work.