Make WordPress Core

Opened 6 weeks ago

Last modified 4 weeks ago

#65754 new defect (bug)

Revisionable, Multivalue Post Meta are not saved properly via REST autosave

Reported by: wongjn Owned by:
Priority: normal Milestone: Future Release
Component: REST API Version: 6.4
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

The Autosave REST API controller does not take into account meta fields that may be multivalued, it seems to assume all meta fields passed to it are single (trac source):

// Attached any passed meta values that have revisions enabled.
if ( ! empty( $meta ) ) {
	foreach ( $revisioned_meta_keys as $meta_key ) {
		if ( isset( $meta[ $meta_key ] ) ) {
			update_metadata( 'post', $revision_id, $meta_key, wp_slash( $meta[ $meta_key ] ) );
		}
	}
}

Set up

  1. Register a revisionable, multivalue post meta field. Minimal plugin example:
    <?php
    /*
     * Plugin Name: Test Autosave
     */
    
    function _test_autosave_register_meta() {
    	$args = array(
    		'sanitize_callback' => 'sanitize_text_field',
    		'show_in_rest'      => array(
    			'schema' => array(
    				'type' => 'string',
    			),
    		),
    		'revisions_enabled' => true,
    	);
    	register_post_meta( 'post', 'foo', $args );
    }
    add_action( 'init', '_test_autosave_register_meta' );
    
  1. Set the AUTOSAVE_INTERVAL to something low for easier testing, like 5:
    wp config set AUTOSAVE_INTERVAL 5 --raw
    
    Or
    define( 'AUTOSAVE_INTERVAL', 5 );
    
  1. Add some post meta values:
    wp post meta add 1 foo bar
    wp post meta add 1 foo baz
    
  1. Verify the data in REST, GET /wp-json/wp/v2/posts/1?_fields=meta
    {
      "meta": {
        "foo": [
          "bar",
          "baz"
        ],
        "footnotes: ""
      }
    }
    
  1. Open the post in the block editor.
  2. Open devtools to be ready.
  3. Edit the content.
  4. Wait for autosave.
  5. View the POST /wp-json/wp/v2/posts/1/autosaves response from the server in devtools network tab. You'll see something like:
    {
      …
      "meta": {
        "foo": [
          null,
          null
        ],
        …
      }
      …
    }
    

If you query the database directly, you'll see in the postmeta table, there will be 2 rows for the autosave, with identical serialized array values. Each member being null, and there being 2 (one for each value):

+---------+---------------------+--------------------+
| post_id | meta_key            | meta_value         |
+---------+---------------------+--------------------+
| 1       | foo                 | a:2:{i:0;N;i:1;N;} |
| 1       | foo                 | a:2:{i:0;N;i:1;N;} |
+---------+---------------------+--------------------+

This is further supports my assertion that this is incorrect behavior.

WordPress Playground Reproduction

Steps 1, 2 & 3 already set up for testing.

Link

Blueprint

{
  "$schema": "https://playground.wordpress.net/blueprint-schema.json",
  "login": true,
  "landingPage": "/wp-admin/post.php?post=1&action=edit",
  "steps": [
    {
      "step": "writeFile",
      "path": "/wordpress/wp-content/plugins/test-autosave.php",
      "data": "<?php\n/*\n * Plugin Name: Test Autosave\n */\n\nfunction _test_autosave_register_meta() {\n\t$args = array(\n\t\t'sanitize_callback' => 'sanitize_text_field',\n\t\t'show_in_rest'      => array(\n\t\t\t'schema' => array(\n\t\t\t\t'type' => 'string',\n\t\t\t),\n\t\t),\n\t\t'revisions_enabled' => true,\n\t);\n\tregister_post_meta( 'post', 'foo', $args );\n}\nadd_action( 'init', '_test_autosave_register_meta' );"
    },
    {
      "step": "activatePlugin",
      "pluginName": "Test Autosave",
      "pluginPath": "/wordpress/wp-content/plugins/test-autosave.php"
    },
    {
      "step": "wp-cli",
      "command": "wp post meta add 1 foo bar"
    },
    {
      "step": "wp-cli",
      "command": "wp post meta add 1 foo baz"
    },
    {
      "step": "wp-cli",
      "command": "wp config set AUTOSAVE_INTERVAL 5 --raw"
    }
  ],
  "preferredVersions": {
    "wp": "latest",
    "php": "8.3"
  },
  "features": {}
}

Change History (5)

This ticket was mentioned in PR #12788 on WordPress/wordpress-develop by jigneshbhavani.


6 weeks ago
#1

  • Keywords has-patch has-unit-tests added

## Problem

WP_REST_Autosaves_Controller::create_post_autosave() stores every revisioned meta key with a single call:

update_metadata( 'post', $revision_id, $meta_key, wp_slash( $meta[ $meta_key ] ) );

update_metadata() called without a $prev_value updates *every* row that shares the meta key. For a meta key registered with single => false the autosave revision already has one row per value, copied there by _wp_copy_post_meta(). Each of those rows is then overwritten with the complete array, so a key with two values ends up as:

meta_key | meta_value
foo      | a:2:{i:0;s:3:"bar";i:1;s:3:"qux";}
foo      | a:2:{i:0;s:3:"bar";i:1;s:3:"qux";}

instead of one row holding bar and another holding qux.

There is a second, smaller problem in the change detection a few lines above:

$old_meta = get_metadata_raw( 'post', $post_id, $meta_key, true );
$new_meta = $meta[ $meta_key ] ?? '';

$single is hardcoded to true, so for a multiple value key the stored data is read back as a single string and compared against an array. That comparison can never match, so the autosave is always considered different from the post even when nothing actually changed, and a revision is written on every request.

## Fix

Whether a key is single is now taken from the single argument it was registered with, rather than from whether the submitted value happens to be an array. That distinction matters: a key registered with single => true can legitimately hold an array, and the existing test_update_item_with_json_meta covers exactly that case. Keys with no registration keep the previous single behaviour.

For multiple value keys the stored rows are replaced the same way _wp_copy_post_meta() writes them in the first place, with delete_metadata() followed by one add_metadata() per value. The change detection reads back with the matching $single argument so the comparison is like for like.

Single value meta is unaffected and still goes through update_metadata().

## Testing instructions

Register a revisionable, multiple value post meta field:

add_action(
        'init',
        function () {
                register_post_meta(
                        'post',
                        'foo',
                        array(
                                'show_in_rest'      => true,
                                'revisions_enabled' => true,
                                'single'            => false,
                                'type'              => 'string',
                        )
                );
        }
);

Add two values to a post, then open it in the block editor, change the content and wait for an autosave:

wp post meta add 1 foo bar
wp post meta add 1 foo baz

Inspect the POST /wp-json/wp/v2/posts/1/autosaves response. Before this change the returned meta.foo does not reflect the submitted values and the revision rows each contain the whole serialized array. After it, each value is stored in its own row and comes back correctly.

Automated coverage is included:

phpunit --group restapi-autosave

test_update_item_with_multiple_value_meta fails on trunk with:

Failed asserting that two arrays are identical.
-    0 => 'bar'
-    1 => 'qux'
+    0 => Array ( 0 => 'bar', 1 => 'qux' )
+    1 => Array ( 0 => 'bar', 1 => 'qux' )

and passes with this change. The full restapi, revision and meta groups pass.

#2 @bejignesh
6 weeks ago

Reproduced on trunk, and the stored data is a bit worse than the report suggests. PR: https://github.com/WordPress/wordpress-develop/pull/12788

The relevant part is in WP_REST_Autosaves_Controller::create_post_autosave(). When the autosave revision is created, _wp_copy_post_meta() copies each value of a multiple value key into its own row, which is right. The controller then writes the submitted values with:

update_metadata( 'post', $revision_id, $meta_key, wp_slash( $meta[ $meta_key ] ) );

update_metadata() without a $prev_value updates every row sharing that meta key, so each of those rows gets overwritten with the whole array rather than one value each:

meta_key | meta_value
foo      | a:2:{i:0;s:3:"bar";i:1;s:3:"qux";}
foo      | a:2:{i:0;s:3:"bar";i:1;s:3:"qux";}

That matches the two identical serialized rows you saw in postmeta.

There is a second, smaller issue just above it, in the change detection:

$old_meta = get_metadata_raw( 'post', $post_id, $meta_key, true );
$new_meta = $meta[ $meta_key ] ?? '';

$single is hardcoded to true, so for a multiple value key the stored data is read back as a single string and compared against an array. That comparison never matches, so the autosave is always treated as different from the post and a revision gets written on every request even when nothing changed. Not part of the original report, but it has the same cause, so it is fixed in the same patch. Happy to split it out if that is preferred.

The patch keys off the single argument the meta key was registered with, rather than checking whether the submitted value happens to be an array. That distinction matters, because a key registered with single => true can legitimately hold an array, and the existing test_update_item_with_json_meta covers that case. For multiple value keys it replaces the stored rows the same way _wp_copy_post_meta() writes them, delete_metadata() followed by one add_metadata() per value. Single value meta still goes through update_metadata() and is unaffected.

Added a regression test. On trunk it fails with:

Failed asserting that two arrays are identical.
-    0 => 'bar'
-    1 => 'qux'
+    0 => Array ( 0 => 'bar', 1 => 'qux' )
+    1 => Array ( 0 => 'bar', 1 => 'qux' )

and passes with the change. The restapi, revision and meta groups all pass.

#3 @wildworks
4 weeks ago

  • Milestone Awaiting Review6.4

Thanks for the report. My investigation indicates that this issue first appeared in r56714, which corresponds to WordPress 6.4.

#4 @bejignesh
4 weeks ago

Confirmed, r56714 is the one. It added both of the lines this fixes to create_post_autosave(): get_metadata_raw( 'post', $post_id, $meta_key, true ) with $single hardcoded, and the update_metadata() call with no $prev_value. The first release containing it is 6.4.0.

Should the 6.4 go in Version rather than Milestone? The ticket is currently the only open one in the 6.4 milestone, which completed in 2023, so I am not sure it will surface in triage from there.

#5 @wildworks
4 weeks ago

  • Milestone 6.4Future Release
  • Version trunk6.4

Should the 6.4 go in Version rather than Milestone?

Thank you for noticing, I meant versions, not milestones. I will correct it.

Note: See TracTickets for help on using tickets.