PHP 8.2 | Gutenberg is introducing deprecated callables into WP Core

[jrfnl]

Copied over from https://core.trac.wordpress.org/ticket/56467#comment:242 for visibility

While looking at the test runs of WP Core, I've found that WP Core PR 3199 as committed in WP Core 54156 introduces four new incompatibilities with PHP 8.2.

The tests committed with that same PR are insufficient and did not flag the problem. The tests committed in WP Core 54211 luckily did flag the problem.

The problem is with the following code:

class WP_Style_Engine {
	const BLOCK_STYLE_DEFINITIONS_METADATA = array(
		/* <snip> */
		'border'     => array(
			/* <snip> */
			'top'    => array(
				'value_func' => 'static::get_individual_property_css_declarations',
				'path'       => array( 'border', 'top' ),
				'css_vars'   => array(
					'color' => '--wp--preset--color--$slug',
				),
			),
			'right'  => array(
				'value_func' => 'static::get_individual_property_css_declarations',
				'path'       => array( 'border', 'right' ),
				'css_vars'   => array(
					'color' => '--wp--preset--color--$slug',
				),
			),
			'bottom' => array(
				'value_func' => 'static::get_individual_property_css_declarations',
				'path'       => array( 'border', 'bottom' ),
				'css_vars'   => array(
					'color' => '--wp--preset--color--$slug',
				),
			),
			'left'   => array(
				'value_func' => 'static::get_individual_property_css_declarations',
				'path'       => array( 'border', 'left' ),
				'css_vars'   => array(
					'color' => '--wp--preset--color--$slug',
				),
			),
		),
		/* <snip> */
	);

	/* <snip> */
}

Each of these definitions use 'value_func' => 'static::get_individual_property_css_declarations'.

That syntax for declaring a callable is deprecated as of PHP 8.2 and will result in a fatal error as of PHP 9.0.

Changing this may require an architectural change to the class.

To explain:

1. The typical PHP-cross-version replacement for 'static::get_individual_property_css_declarations' would be array( static::class, 'get_individual_property_css_declarations' ).
2. However, static::class cannot be used in a constant declaration as constant values have to be ''constant'' and how static::class resolves can change at runtime depending on the class hierarchy.

I'd like to understand why the choice was made to use static:: in these declarations.
I currently do not see any class within WP Core which extends the WP_Style_Engine class, so why is static:: being used instead of self:: ?

As for solution directions:

• If static:: was not used intentional and can be replaced with self::, the callables need to be rewritten to array( self::class, 'get_individual_property_css_declarations' ).
• If static:: was used intentionally, an architectural change is needed as the callable cannot be declared like that in the constant.
  • Changing the constant to a private property without the callables and adding them to the array in the class constructor is not an option as the class only contains static methods and does not seem to be intended to be instantiated.
  • Changing the constant to a private static property is not an option either, as same as constants and non-static properties, the initial value for a property needs to be ''constant''.

In other words: If static:: was used intentionally, the class will probably need a rethink...

As this is a new class and any architectural change in it in the future would be considered a breaking change, this MUST be fixed before WP 6.1 is released.

And for the love of code, please, please add better tests to safeguard against these kind of problems and check the PHP 8.1 and 8.2 test logs to prevent introducing new issues like this.

Also see: https://wiki.php.net/rfc/deprecate_partially_supported_callables

[talldan]

check the PHP 8.1 and 8.2 test logs to prevent introducing new issues like this.

@jrfnl Is there any way the tests can be made to fail if there are such critical issues? It'd be great if continuous integration could help prevent these kind of errors.

edit: I'm also wondering how a developer can check the logs. I don't see anything being logged in the github check - https://github.com/WordPress/wordpress-develop/actions/runs/3084932834/jobs/4987654678.

[ramonjd]

Thanks for creating this issue and the great explanation.

I'd like to understand why the choice was made to use static:: in these declarations.

I believe the use of static:: was in imitation of the WP Theme JSON class, which, at least in the Gutenberg plugin relies on extensibility to overwrite class functionality. Example

I currently do not see any class within WP Core which extends the WP_Style_Engine class, so why is static:: being used instead of self:: ?

Switching the callables to array( self::class, 'get_individual_property_css_declarations' ) as you suggest appears to be the best option. I'm pretty sure the WP_Style_Engine class will not (and should not) be extended, in fact, I might add a comment to that effect in the class.

this MUST be fixed before WP 6.1 is released.

Agree. I'm happy to throw up 2 x patches (Core and a Gutenberg sync). Will do it ASAP

[jrfnl]

check the PHP 8.1 and 8.2 test logs to prevent introducing new issues like this.

@jrfnl Is there any way the tests can be made to fail if there are such critical issues? It'd be great if continuous integration could help prevent these kind of errors.

@talldan Well, to start, it would help if the tests for the PHP code in GB would actually be run on PHP 5.6 up to PHP 8.2, instead of only being run on PHP 7.4.... 😢

See: #43333 (comment)

edit: I'm also wondering how a developer can check the logs. I don't see anything being logged in the github check - https://github.com/WordPress/wordpress-develop/actions/runs/3084932834/jobs/4987654678.

That's correct as you are looking in the wrong place, though I have to admit, it's on my pet peeve list that the PHPCompatibility workflow hasn't been set up to also show the issues in the logs. It's on my long term to-do list to get that fixed.

For now, issues thrown by PHPCompatibility will only show as comments in the PR, but PHPCompatibility wouldn't pick up on the issues I reported here yet, as there is no release (yet) containing PHP 8.2 related sniffs.

What I meant was to check the actual unit test run logs. The test runs for PHP 8.1 and PHP 8.2 (for WP Core) are still "allowed to fail" due to the sheer amount of issues which had to be fixed, but the logs will show the issues.

So for this issue, you can see the errors in the actual unit test runs on PHP 8.2 here: https://github.com/WordPress/wordpress-develop/actions/runs/3084932812/jobs/4987657992#step:19:306

Hope that helps ?

[jrfnl]

I believe the use of static:: was in imitation of the WP Theme JSON class, which, at least in the Gutenberg plugin relies on extensibility to overwrite class functionality. Example

@ramonjd thank you for providing this context. Highlights again that we all need to be careful when copying over code patterns as they may not be appropriate for another class.

Switching the callables to array( self::class, 'get_individual_property_css_declarations' ) as you suggest appears to be the best option.

👍🏻

I'm pretty sure the WP_Style_Engine class will not (and should not) be extended, in fact, I might add a comment to that effect in the class.

A comment is a good first step, but what about making the class final ? That way we can prevent the class from ever being extended.

(After all, we all know that plugins will try to do things if they can, no matter what we say in comments/docblocks - if they can't extend the class, we don't have that problem).

this MUST be fixed before WP 6.1 is released.

Agree. I'm happy to throw up 2 x patches (Core and a Gutenberg sync). Will do it ASAP

Much appreciated! Feel free to ping me if you want a review/approval.

[ramonjd]

A comment is a good first step, but what about making the class final ?

I don't see any issues here. Given that it'd be great to avoid the compat woes that exist for WP Theme JSON, I'm for it.

cc @aristath and @andrewserong

Patches incoming... !

In fact it was already raised by @costdev in the original backport PR

[andrewserong]

I don't see any issues here. Given that it'd be great to avoid the compat woes with WP Theme JSON I'm for it.

Same here — so far I think the intention has been for the style engine classes to be mostly used internally in WordPress, and for public functions to be used as the entry point for other use cases, e.g. plugins and themes. But I believe Ari has had more of the plugin/theme use case in mind so might have some insights to add there, too.

[talldan]

The test runs for PHP 8.1 and PHP 8.2 (for WP Core) are still "allowed to fail" due to the sheer amount of issues which had to be fixed

I suppose an option could be to run the tests on trunk and then on the PR and check whether the number of errors reported increased. If so, fail the job. I don't know whether that's easy to do though. It wouldn't be foolproof if the PR fixed some errors but introduced others.

[jrfnl]

The test runs for PHP 8.1 and PHP 8.2 (for WP Core) are still "allowed to fail" due to the sheer amount of issues which had to be fixed

I suppose an option could be to run the tests on trunk and then on the PR and check whether the number of errors reported increased. If so, fail the job. I don't know whether that's easy to do though. It wouldn't be foolproof if the PR fixed some errors but introduced others.

I am actively working, together with some other people, to get it down to zero and remove the continue-on-error for both PHP 8.1 and PHP 8.2. We are nearly there, only a few more issues to go (which is why anything new being introduced now really stands out like a sore thumb...)

[jrfnl]

That's correct as you are looking in the wrong place, though I have to admit, it's on my pet peeve list that the PHPCompatibility workflow hasn't been set up to also show the issues in the logs. It's on my long term to-do list to get that fixed.

@talldan FYI: WordPress/wordpress-develop#3368