Can't save registered post meta field of type string that equals registered default value via REST API

[kraftner]

Current behaviour

Currently if you register a post meta key as string and set a default value:

register_post_meta(
  'my_cpt,
  'my_meta',
  [
    'show_in_rest' => true,
    'single' => true,
    'type' => 'string',
    'default' => 'foo'
  ]
);

and then save the default value it is not actually being written to the database.

If you save any other type, e.g. a boolean

register_post_meta(
  'my_cpt,
  'my_meta',
  [
    'show_in_rest' => true,
    'single' => true,
    'type' => 'boolean',
    'default' => true
  ]
);

it is being written to the database.

Also not saving via the REST API, but via update_metadata() works for strings.

What seems to happen

The problem seems to happen like this:

• Post meta is only saved if it differs from the currently stored value.
• So before saving each field the new value is compared with the stored value.
• When doing so the assumption is (rightfully) that all stored data is a string since that is how post meta is stored in the DB.
• If the key doesn't exist though, the comparison happens with the default value.
• The default value though isn't necessarily a string, but is handled as such in the strict equality check.
• So the strict equality check fails on anything but a string field.

-> String fields can't save the default value, anything else can.

How did it come to this?

I am not absolutely sure what is actually the intended behavior, but I assume if there is nothing stored in the DB the value should be saved, even if it equals the default.

I assume this because the behavior of update_metadata() got changed to only consider DB data, not default values for the comparison by retrieving the stored data using get_metadata_raw() instead of get_metadata().

WP_REST_Meta_Fields duplicates some of the checks in update_metadata() (​even explicitly states that in a comment) but WP_REST_Meta_Fields::update_meta_value() (and some other places in that class) didn't get the change to use get_metadata_raw() instead of get_metadata().

I assume this got overlooked when introducing the default metadata values in https://core.trac.wordpress.org/changeset/48402

It seems that replacing get_metadata with get_metadata_raw in that class should fix the issue, but I haven't found time yet to prepare a patch, so I thought I'd at least document the issue for now to see if someone else finds time for a fix before me.

[ramon fincken]

I try to reproduce this but I fail.

vanilla WP, twentyeleven -> post (gutenberg)

<?php
register_post_meta(
  '', // all
  'my_metastring',
  [
    'show_in_rest' => true,
    'single' => true,
    'type' => 'string',
    'default' => 'foo'
  ]
);

register_post_meta(
  '', // all
  'my_metabool',
  [
    'show_in_rest' => true,
    'single' => true,
    'type' => 'boolean',
    'default' => true
  ]
);

and

<?php
register_post_meta(
  'post',
  'my_metastring',
  [
    'show_in_rest' => true,
    'single' => true,
    'type' => 'string',
    'default' => 'foo'
  ]
);

register_post_meta(
  'post',
  'my_metabool',
  [
    'show_in_rest' => true,
    'single' => true,
    'type' => 'boolean',
    'default' => true
  ]
);

nothing is stored on create new post, and nothing is stored in update post.
I am looking in the postmeta table.

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

[kraftner]

@ramon-fincken Thanks for having a look and sorry if my instructions for reproduction weren't clear. What was missing to reproduce this was trying to save the defaults via the REST API: ​https://developer.wordpress.org/rest-api/reference/posts/#schema-meta

I just took the time to create a PR including a test. I am still not 100% sure this fully covers the whole issue but maybe you can test the PR and it is more clear now what the issue is.

[ramon fincken]

shall we discuss this in 2 days in real life ? :)

[ramon fincken]

Confirmed: the patch works for single meta values.

However we cannot get the tests setup for multi valued meta values, therefore we cannot validate the patch for default value(s?) of multiple rows of meta value for the same meta key.

[ramon fincken]

also : the registration of meta should be in the set_up method of the test class

[kraftner]

@ramon-fincken I just found out why we failed with the multi values: Because my patch was incomplete. I think I've fixed this now.

I need to check this some more, but have pushed it to the PR anyway so I don't forget where we were at until after WCEU. :)

[dmsnell]

In my opinion it's not necessary to fix the behavior for single values and for multiple values for the patch to be valuable. if we're having a difficult time getting the test to run for multi-value updates, let's focus on solving the single-value case and then follow-up with a fix for the multi-value cases.

@kraftner - I know that you mentioned confirming a full rubric of expected behaviors. For posterity sake, could you add that here? A simple table showing the ways that the direct PHP function and the REST API behave differently could quickly summarize the core of this issue, as well as clarify what a fix ought to do.

Also, please reach out any time for help pushing this forward. I don't want to let this linger in limbo, but I can get distracted easily, so it's not a bother for you to ping me here or in Slack.

[ramon fincken]

@kraftner we need to adjust the PHPunit to move some lines of code to the setUp function

[ramon fincken]

Checked => unit tests run OK!

my compliments for first checking for assertNotEmpty first, then assertCount, then followed by the actual string check

[kraftner]

@dmnsell It wouldn't make sense to only fix one case because than the behavior would be even more confusing. Anyway, I got this working now for all types including extensive test coverage. I just really can't do any productive work on Contributor Day - I just need my proper setup and a calm room :)

@ramon-fincken I don't think we should move the registration to the setUp function. They would then be registered for every test in this test class which is not needed. There are plenty of tests in this file that do set up their own fields just for one particular test. I think those on the setUp function are only those that are used across multiple tests.

For the sake of completeness: Here is the table we've been talking about @dmsnell

It shows whether a default value of a specific type is being saved to the DB using either the PHP API or the REST API.

	update_metadata()	WP_REST_Meta_Fields::update_meta_value()
boolean default (single)	Yes	Yes
boolean default (multi)	Yes	No (500 Error)
integer default (single)	Yes	Yes
integer default (multi)	Yes	No (500 Error)
number default (single)	Yes	Yes
number default (multi)	Yes	No (500 Error)
object default (single)	Yes	Yes
object default (multi)	Yes	No (500 Error)
array default (single)	Yes	Yes
array default (multi)	Yes	No (500 Error)
string default (single)	Yes	No
string default (multi)	Yes	No

All in all I've thought this through again and it looks good to me now as is.

[kraftner]

As it is the back of my brain kept grinding on the issue and I went back to double check again if/how the tests fail without the patch.

While doing so I noticed that my table above was wrong. The 500 error @ramon-fincken and me noticed with non-single/multi value keys is actually worse.

Without the change from get_metadata to get_metadata_raw any updates of keys registered as non-single make the whole request fail with a 500 error.

This is due to the fact that ​`update_multi_meta_value` does delete existing values before it re-adds them. But since it wrongly thinks of the non-stored values as stored and then tries to delete them the deletion fails resulting in the whole request to abort and fail with a 500.

Funnily this applies in the inverse: To any meta type except string: Since strings are the only one considered equal to the default the update bails early and doesn't run into this issue.

All in all this aspect of the bug is even worse than my initially reported one since it might abort mid-way updating multiple meta values which can result in an inconsistent state of the meta values.

The good news is that the patch in my PR also fixes this issue.

For completeness sake I'll update the table in my previous comment also considering that original error state.

[kraftner]

Okay, I did a final refactor for the tests.

The tests now dynamically cover all combinations of scalar types including having them in objects and arrays.

Also each of these now runs as a single test which makes it easy to see the specific failures as shown in the table above without the patch applied.

Trac ticket: Core-55600

Trac ticket: Core-55600

Trac ticket: Core-55600

@kraftner @ramonfincken I have made several updates to the test code and added some @since tags to the source. In doing this I hope I didn't mess up the tests.

Additionally I merged in trunk but only to ensure that when the tests run they have the updated code. If you make any changes, please feel free to reset the branch tip to the commit before trunk (as if it hadn't been merged in).

You are welcome to modify or undo my changes, as they are meant as suggestions but without requiring you do all the steps I suggest.

[dmsnell]

@kraftner thanks for your patience. I've finally dug in and verified everything you said. I think it's nice to note that the additional 500s are resolved now, though as you point out, there remains a concurrency issue with updating multiple values, as any failure caused an interruption and failure for the entire request, even when updates have been partially applied. a good earmark for another ticket.

It does seem like #43941 is the culprit which introduced this bug, and the explanation seems to fit the code change. Also, the cause explains why the REST API behavior is different than when calling update_ methods directly. Thank you for detailing the cause, fixing the bug, and adding tests.

Since it appears like the original code was in 5.5 I've marked this as a bug in that release; someone can verify that this is correct. I'm requesting a second opinion from anyone more familiar with the REST API to verify, perhaps someone from the original ticket would like to review it: cc: @spacedmonkey @TimothyBlynJacobs @flixos90 since you seemed more involved when this was introduced.

If nobody has objection I'll try and ensure this gets into 6.7.

@dmsnell I looked through you changes and couldn't spot any issues. :heart:
Thanks for the cleanup and adding all the documentation too. I thought about it but since all the other tests barely had any I thought it might not be a common thing to do in tests. Having said that I personally love it and think it makes those pretty complex setups more understandable.

Trac ticket: Core-55600

[dmsnell]

In 58831:

REST API, Meta: Store updates in database when they are equal to the defaults.

This patch fixes an oversight from when default metadata values were introduced
in #43941 in WordPress 5.5: metadata updates should persist in the database
even if they match the registered default value (because the default values
can change over time).

Previously, the REST API code was comparing updated values against the value
returned by the default-aware get_metadata() method. This meant that if no
value existed in the database, and the default value was supplied to the update,
WordPress would think that the updated value was already persisted and skip
the database call.

Now, the get_metadata_raw() method is called for comparing whether or not
a database update is required, fixing the bug.

In this patch both issues are resolved.

Developed in ​https://github.com/wordpress/wordpress-develop/pull/6782
Discussed in https://core.trac.wordpress.org/ticket/55600

Follow-up to [48402].

Props: dmsnell, kraftner, ramon-fincken.
Fixes #55600.

Merged in [58831]
​https://github.com/wordpress/wordpress-develop/commit/9ed3553da6a61a38a1d4c5585e5ac3c9d13d351a

Trac ticket: Core-55600

[dmsnell]

Given that there was no further feedback I felt it would be best to get this merged sooner rather than later to give folks ample time to discover any issues with it in trunk.

Thanks for your work on this @kraftner and @ramon-fincken!

@peterwilsoncc (or anyone) - given that this originated in 5.5, what is the policy for backporting fixes? is it old enough that we simply move forward, or should we attempt to apply this fix to the old branches?

[swissspidy]

We don‘t backport bug fixes all the way back to when a bug was first introduced, this one isn‘t any different. So 6.7 it is.

A code review from a REST API maintainer like @TimothyBlynJacobs would have been helpful, since they worked on the original implementation mentioned here.

[dmsnell]

Thanks @swissspidy - yeah I was hoping someone more familiar with the code would review (which is why I pinged them a couple of weeks ago). Without hearing from anyone I figured getting testing going would be best. Since it's still early in the release cycle we have plenty of time to revert or fix any issues that arise. I did go through the code carefully and everything looks sound. Happy to take further guidance.