Open hellofromtonya opened 7 months ago
This issue has happened before, e.g. see https://github.com/WordPress/gutenberg/pull/54103.
Possible solutions are:
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:
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.
Disadvantages:
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 https://github.com/WordPress/gutenberg/pull/54103.
Advantages:
Disadvantages:
Catching this with a linter will be challenging, but not impossible.
I'd suggest changing all instances to Approach 2, prevent the class file from loading into memory.
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.
+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.
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.
I've raised https://github.com/WordPress/gutenberg/pull/58454 which can act as a stub for documenting whichever route we approve
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.
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.
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.
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 https://github.com/WordPress/gutenberg/pull/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.
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):
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.
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.
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 🙂
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.
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:
wp-content/plugins
:
<?php
class A {
public function aa() {
echo 'A::aa() was called
';
}
}
return;
class B {
public function bb() {
echo 'B::bb() was called
';
}
}
- File 2, name plugin.php, also in `wp-content/plugins`:
```php
<?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
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.
@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.
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?
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 toWP_
. 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.
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:
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.
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.
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.
Can‘t we just update the PHPCS config to exclude/disable that sniff for certain files? To me that‘s what configs are for.
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.
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 WPtrunk
) after packages were updated in Core yesterday:The
WP_Navigation_Block_Renderer
file had aclass_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
trunk
.Screenshots, screen recording, code snippet
Environment info
trunk
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