wp_save_post_revision() assumes ALL post meta is text! Fatal error in PHP 8.2.

[jsmoriss]

The wp_save_post_revision() function assumes that ALL post meta is text:

​https://github.com/WordPress/wordpress-develop/blob/6.3/src/wp-includes/revision.php#L164

When using register_meta() with 'revisions_enabled' => true to register post meta that is an array, it causes a fatal error in PHP 8.2:

PHP Fatal error: Uncaught TypeError: trim():
Argument #1 ($string) must be of type string, array given in /.../wp-includes/formatting.php:5486

Since registering post meta is announced for WP 6.4 (see ​https://make.wordpress.org/core/2023/10/24/framework-for-storing-revisions-of-post-meta-in-6-4/), it's a pretty serious problem.

I've tested the issue on WP 6.3.2 and WP 6.4 (although the footer says 6.5-alpha-57075).

js.

[TimothyBlynJacobs]

This seems like it can be fixed in a point release to support non-string meta keys. The revisions_enabled is false by default, so users won't be upgrading to something that breaks without making code changes.

[jsmoriss]

As a plugin author, I can tell you that using 'revisions_enabled' => true to save/rollback post settings changes (ie. settings which are an array) in post meta is already scheduled for WP 6.4.

js.

[jorbin]

If this is an issue in 6.3, then it's also not related to the new meta revisions.

[jsmoriss]

Replying to jorbin:

If this is an issue in 6.3, then it's also not related to the new meta revisions.

The new meta revisions will highlight the issue, which still exists in WP 6.4. Right now I'm guessing that plugin authors are not using revisions_enabled => true, but this ​https://make.wordpress.org/core/2023/10/24/framework-for-storing-revisions-of-post-meta-in-6-4/ is meant to change that. If plugin authors can only register text strings for revisions, then we need to know.

js.

[jorbin]

@jsmoriss do you have code that demonstrates the issue?

[hellofromTonya]

Update:

​A discussion happened in Make/Core slack to determine if this ticket is a blocker for the 6.4.0 release, i.e. which is scheduled to start in a few minutes.

It's unclear how an array is being passed. As @azaozz and @Clorith noted, all DB meta entries are serialized (as strings). The related code was not introduced in 6.4. Further step-by-step instructions are needed to reproduce the reported issue.

There's consensus this ticket is not a blocker for 6.4.0.

Thank you everyone for rallying :)

[jsmoriss]

Replying to jorbin:

@jsmoriss do you have code that demonstrates the issue?

Adding an error_log() to the foreach() loop:

​https://github.com/WordPress/wordpress-develop/blob/6.3/src/wp-includes/revision.php#L163-L168

foreach ( array_keys( _wp_post_revision_fields( $post ) ) as $field ) {

    error_log( 'comparing ' . $field . ' values of type ' . gettype( $post->$field  ) );

    if ( normalize_whitespace( $post->$field ) !== normalize_whitespace( $latest_revision->$field ) ) {
        $post_has_changed = true;
        break;
    }
}

Produces the following error when saving a post/page:

[07-Nov-2023 17:57:40 UTC] comparing post_title values of type string
[07-Nov-2023 17:57:40 UTC] comparing post_content values of type string
[07-Nov-2023 17:57:40 UTC] comparing post_excerpt values of type string
[07-Nov-2023 17:57:40 UTC] comparing footnotes values of type string
[07-Nov-2023 17:57:40 UTC] comparing _wpsso_meta values of type array
[07-Nov-2023 17:57:40 UTC] PHP Warning:  trim() expects parameter 1 to be string, array given in /var/www/wpadm/wordpress/wp-includes/formatting.php on line 5486
[07-Nov-2023 17:57:40 UTC] PHP Warning:  trim() expects parameter 1 to be string, array given in /var/www/wpadm/wordpress/wp-includes/formatting.php on line 5486

js.

[jsmoriss]

Note that there are no hooks here ​https://github.com/WordPress/wordpress-develop/blob/6.3/src/wp-includes/revision.php#L163-L168 to manage the comparison.

In the upcoming WP v6.4 release there is a new filter '_wp_post_revision_field_{meta_name}' to manage the visual comparison. In the case of an array, the var_export() function can be used to allow the revision diff to work as expected.

js.

[jorbin]

It ​looks like the _wpsso_meta field is coming from ​https://wordpress.org/plugins/wpsso/ which in the 16.6.0 version isn't using register_post_meta, register_meta, or the _wp_post_revision_fields filter.

Do you have a minimal code example that can be used to replicate the issue? Ideally something small that can be installed as an mu-plugin or an automated test.

[jsmoriss]

Replying to jorbin:

It ​looks like the _wpsso_meta field is coming from ​https://wordpress.org/plugins/wpsso/ which in the 16.6.0 version isn't using register_post_meta, register_meta, or the _wp_post_revision_fields filter.

Do you have a minimal code example that can be used to replicate the issue? Ideally something small that can be installed as an mu-plugin or an automated test.

Registering post meta for revisions is scheduled for the next release of WPSSO Core.

Registering the post meta:

​https://github.com/surniaulula/wpsso/blob/17.0.0/lib/abstract/wp-meta.php#L278-L287

Adding the revision title filter:

​https://github.com/surniaulula/wpsso/blob/17.0.0/lib/abstract/wp-meta.php#L292

Adding the comparison filter:

​https://github.com/surniaulula/wpsso/blob/17.0.0/lib/abstract/wp-meta.php#L309

The arrays do need to be converted to text for the comparison diff:

​https://github.com/surniaulula/wpsso/blob/17.0.0/lib/abstract/wp-meta.php#L319-L333

I don't have time to write a simple plugin to trigger this issue today, but hopefully in the next few days.

js.

[jsmoriss]

Replying to jorbin:

Do you have a minimal code example that can be used to replicate the issue? Ideally something small that can be installed as an mu-plugin or an automated test.

Here you go:

​https://github.com/jsmoriss/jsm-test-revision-array/blob/main/jsm-test-revision-array.php

Although the plugin updates the Page metadata with an array, you need to manually save the Page to create the revision, and then view the revision changes to trigger the bug:

PHP Warning:  trim() expects parameter 1 to be string, array given in /var/www/wpadm/wordpress/wp-includes/formatting.php on line 5506

js.

See https://core.trac.wordpress.org/ticket/59827

[adamsilverstein]

@jsmoriss

Thanks for reporting (plus demo and PR!), I'll take a closer look at this. It looks like your plugin still returns a string for the '_wp_post_revision_field_' . meta_key filter, so it doesn't make sense that it triggers this error, I'll have to dig in further to see why. Also, I suspect this issue exists in older versions of WordPress, the meta revisions just made it easier to see.

In any case, I agree having some handling here for non string values would be an improvement.

ps. it would be good to add a test here to validate any fix we come up with.

[jsmoriss]

Replying to adamsilverstein:

It looks like your plugin returns a string for the '_wp_post_revision_field_' . meta_key filter, so it doesn't make sense that it triggers this error

The "_wp_post_revision_field_{meta_key}" filter is only used for the diff shown to users on the revision page.

The problem is the compare done in wp_save_post_revision() (see ​https://github.com/WordPress/wordpress-develop/blob/6.3/src/wp-includes/revision.php#L109) which WordPress uses to determine if a revision needs to be saved - it assumes that all post meta are strings.

I didn't actually need to include the "_wp_post_revision_fields" and "_wp_post_revision_field_{meta_key}" filters in the test plugin to create the error - it's just nice to have some visual feedback on the revisions page. :) To trigger the bug, you just need to save the Page with the test plugin active.

ps. it would be good to add a test here to validate any fix we come up with.

The test plugin creates a PHP error in the current WordPress development branch, and no error with the PR.

js.

[adamsilverstein]

The "_wp_post_revision_field_{meta_key}" filter is only used for the diff shown to users on the revision page.

Ah, thanks for clarifying, that makes sense. Testing now.

[adamsilverstein]

I was able to reproduce the bug using the test plugin. It wasn't obvious how to reproduce, so I am giving the steps here:

1. Create a new page
2. add some content, save post
3. make some changes, update the (draft) post
4. visit the revisions screen then return to editor (or maybe just reload?)
5. make a change and try publishing the post

Publishing will fail with the Fatal noted in the ticket description.

[adamsilverstein]

@jsmoriss writing tests for this I have realized that it is the use of _wp_post_revision_fields that creates the issue. Once you add the meta field to the revisions screen using this filter, it triggers the comparison section to see if the field changed when storing revisions.

While we plain to add the meta revision hook automatically in https://core.trac.wordpress.org/ticket/59636 (so you won't need to use _wp_post_revision_fields to add your field to the display), I still think its work adding some protection against the issue you discovered.

…included in _wp_post_revision_fields.

Trac ticket: https://core.trac.wordpress.org/ticket/59827

[adamsilverstein]

I added a test for this that triggers the fatal in ​https://github.com/WordPress/wordpress-develop/pull/5680

Adding the code fix next based on the existing PR with a slightly different approach.

[adamsilverstein]

59827.diff​ includes the new test as well as the fix. I'm going to let the PR fail to demonstrate the fatal error, then push the fix to the PR to verify it is resolved.

[adamsilverstein]

Note that although this issue has actually existed for a long time, it is surfaced by the new support for post meta revisions in 6.4. Previous to this release, fully supporting post meta revisions was a ​bit complicated although you could still reproduce this issue.

Because this issue is severe (a PHP fatal) and surfaced by meta revisions added in 6.4, I would be in favor of back porting the fix to 6.4.x

[adamsilverstein]

I'm going to let the PR fail to demonstrate the fatal error, then push the fix to the PR to verify it is resolved.

Tests ​failed, pushed fix, tests pass.

Suggest adding a 'pre_wp_save_post_revision_post_has_changed' filter.

Trac ticket: https://core.trac.wordpress.org/ticket/59827

[jsmoriss]

@adamsilverstein While we're at it, maybe we could add a 'pre_wp_save_post_revision_post_has_changed' filter to allow plugin authors to do their own comparison logic?

js.

[adamsilverstein]

@adamsilverstein While we're at it, maybe we could add a 'pre_wp_save_post_revision_post_has_changed' filter to allow plugin authors to do their own comparison logic?

What is the use case?

You can use the wp_save_post_revision_check_for_changes filter already to bypass the change checks, would that work?

[jsmoriss]

Replying to adamsilverstein:

maybe we could add a 'pre_wp_save_post_revision_post_has_changed' filter to allow plugin authors to do their own comparison logic?

What is the use case?

The idea was to have an option to bypass the WordPress comparison checks. The existing filter is done after WordPress has already done its comparison. Its not a big deal - I'm just hoping the comparison gets fixed soon so I can release the meta revisions feature for my plugins.

js.

Add a check to normalize_whitespace so that it will only run on strings, this prevents a fatal error if a post meta is stored in revisions that isn't just a string.

[th0masmb]

I'm suggesting just adding a check to normalize_whitespace where it checks if the variable is in fact a string, and if not, simply return it. Like mentioned above, this has been a long standing issue. Either way, I really hope we see these merged into core soon.