Make WordPress Core

Opened 8 months ago

Last modified 5 months ago

#59827 assigned defect (bug)

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

Reported by: jsmoriss's profile jsmoriss Owned by: adamsilverstein's profile adamsilverstein
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 6.3.2
Component: Revisions Keywords: reporter-feedback has-patch has-unit-tests
Focuses: php-compatibility Cc:

Description

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.

Attachments (1)

59827.diff (4.0 KB) - added by adamsilverstein 7 months ago.

Download all attachments as: .zip

Change History (33)

#1 @TimothyBlynJacobs
8 months ago

  • Severity changed from blocker to normal

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.

#2 @jsmoriss
8 months ago

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.

#3 follow-up: @jorbin
8 months ago

  • Severity changed from normal to blocker

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

#4 @jorbin
8 months ago

  • Severity changed from blocker to normal

#5 in reply to: ↑ 3 @jsmoriss
8 months ago

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.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


8 months ago

#7 follow-up: @jorbin
8 months ago

@jsmoriss do you have code that demonstrates the issue?

#8 @hellofromTonya
8 months ago

  • Focuses php-compatibility removed
  • Keywords reporter-feedback added

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 :)

#9 in reply to: ↑ 7 @jsmoriss
8 months ago

  • Focuses php-compatibility added
  • Keywords reporter-feedback removed

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.

#10 @jsmoriss
8 months ago

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.

#11 follow-ups: @jorbin
8 months ago

  • Keywords reporter-feedback added

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.

#12 in reply to: ↑ 11 @jsmoriss
8 months ago

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.

Last edited 8 months ago by jsmoriss (previous) (diff)

#13 in reply to: ↑ 11 @jsmoriss
8 months ago

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.

Last edited 8 months ago by jsmoriss (previous) (diff)

This ticket was mentioned in PR #5648 on WordPress/wordpress-develop by jsmoriss.


8 months ago
#14

  • Keywords has-patch added

#15 follow-up: @adamsilverstein
8 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

@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.

Version 0, edited 8 months ago by adamsilverstein (next)

#16 in reply to: ↑ 15 @jsmoriss
8 months ago

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.

Last edited 8 months ago by jsmoriss (previous) (diff)

#17 @adamsilverstein
7 months ago

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.

#18 @adamsilverstein
7 months ago

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.

#19 @adamsilverstein
7 months ago

  • Keywords needs-unit-tests added

#20 @adamsilverstein
7 months ago

@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.

This ticket was mentioned in PR #5680 on WordPress/wordpress-develop by @adamsilverstein.


7 months ago
#21

  • Keywords has-unit-tests added; needs-unit-tests removed

…included in _wp_post_revision_fields.

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

#22 @adamsilverstein
7 months ago

  • Keywords needs-unit-tests added; has-unit-tests removed

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.

#23 @adamsilverstein
7 months ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#24 @adamsilverstein
7 months ago

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.

Last edited 7 months ago by adamsilverstein (previous) (diff)

#25 @adamsilverstein
7 months ago

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

#26 @adamsilverstein
7 months ago

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.

Last edited 7 months ago by adamsilverstein (previous) (diff)

This ticket was mentioned in PR #5688 on WordPress/wordpress-develop by jsmoriss.


7 months ago
#27

Suggest adding a 'pre_wp_save_post_revision_post_has_changed' filter.

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

#28 @jsmoriss
7 months ago

@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.

#29 follow-up: @adamsilverstein
7 months ago

@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?

#30 in reply to: ↑ 29 @jsmoriss
7 months ago

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.

This ticket was mentioned in PR #5809 on WordPress/wordpress-develop by thomasmb.


6 months ago
#31

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.

#32 @th0masmb
6 months ago

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.

Last edited 6 months ago by th0masmb (previous) (diff)
Note: See TracTickets for help on using tickets.