Saving a certain meta key saves other unmodified keys, using core/editor editPost action

[garciaalvaro]

Describe the bug When saving a meta key in a post all other non-modified keys are also saved. This leads to keys which havent been modified to save empty values.

To reproduce Steps to reproduce the behavior:

1. Register two or more meta keys:

   add_action( 'init', __NAMESPACE__ . '\register_meta' );
   function register_meta() {
   register_post_meta(
       'post',
       'my_meta_a',
       array(
           'show_in_rest' => true,
           'single'       => true,
           'type'         => 'string',
       )
   );
   register_post_meta(
       'post',
       'my_meta_b',
       array(
           'show_in_rest' => true,
           'single'       => true,
           'type'         => 'string',
       )
   );
   register_post_meta(
       'post',
       'my_meta_c',
       array(
           'show_in_rest' => true,
           'single'       => true,
           'type'         => 'boolean',
       )
   );
   }

2. Go to a post editor, and publish the post. Open the console and run:

   wp.data.dispatch("core/editor").editPost({ meta: { my_meta_a: 'aaa' } })

3. Update the post
4. Check the saved meta. This can be done in a phpMyAdmin in wp_postmeta table or printing the meta value in a template inside a hook, for example:

   add_action( 'wp_head', __NAMESPACE__ . '\print_meta' );
   function print_meta() {
   var_dump( get_post_meta( get_the_ID() ) );
   }

Expected behavior With WordPress 5.1 and 5.2, saving meta using wp.data.dispatch("core/editor").editPost only saves the meta keys that changed. This means that the ouput is:

array(4) { ... ["my_meta_a"]=> array(1) { [0]=> string(3) "aaa" } }

However in the latest Gutenberg (and at least the previous version) it saves the value of the changed key and an empty value in any non-saved key:

array(4) { ... ["my_meta_a"]=> array(1) { [0]=> string(3) "aaa" ["my_meta_b"]=> array(1) { [0]=> string(0) "" } ["my_meta_c"]=> array(1) { [0]=> string(0) "" } }

This is problematic as its saving values that were not intended. Before, one could check if a key was saved using get_post_custom_keys. This function now returns both my_meta_b and my_meta_c but WP 5.1, 5.2 do not.

I hope its clear, thanks.

[garciaalvaro]

@epiqueras is there a chance to take a look at this issue? This regression is still present in WordPress 5.2.4 and Gutenberg 6.7.0. If there is anything to clarify please let me know. Thank you very much

[epiqueras]

When saving an object property like meta. We merge in any changes to the current persisted object and send that.

In general, PUTing an object edit like this should not depend on the previous state of the object.

The issue here is that the API is returning empty strings or false booleans for undefined meta keys and those are being sent back when saving, masking the difference between an actual undefined value and an empty string or false boolean. I believe this should be fixed at the API level.

cc @youknowriad @TimothyBJacobs

[TimothyBJacobs]

IIRC the last time @kadamwhite and I discussed default meta, we came to the conclusion that if you updated a resource with the default value, that it should save the default value.

It'd be a breaking API change to stop saving default metadata when it is posted back. But I can see how this would be frustrating in this use case. Something to discuss at an upcoming Rest API meeting for sure.

---

Was Gutenberg pre 6.7.0 not sending back the previous meta property and just including edited fields?

[epiqueras]

I guess so.

But, then why doesn’t get_post_meta also return the default values?

On Wed, Oct 30, 2019 at 1:32 PM Timothy Jacobs notifications@github.com wrote:

IIRC the last time @kadamwhite https://github.com/kadamwhite and I discussed default meta, we came to the conclusion that if you updated a resource with the default value, that it should save the default value.

It'd be a breaking API change to stop saving default metadata when it is posted back. But I can see how this would be frustrating in this use case. Something to discuss at an upcoming Rest API meeting for sure.

Was Gutenberg pre 6.7.0 not sending back the previous meta property and just including edited fields?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WordPress/gutenberg/issues/17864?email_source=notifications&email_token=AESFA2BSMAGKYINEHCPNVW3QRHVNJA5CNFSM4I7AJTMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECVVNIQ#issuecomment-548099746, or unsubscribe https://github.com/notifications/unsubscribe-auth/AESFA2BZIKR4E3WUGDZ4ZSTQRHVNJANCNFSM4I7AJTMA .

[TimothyBJacobs]

It does to the extent that it can. get_post_meta will return an empty string if the meta key does not exist.

Currently, you can't specify a specific default value for the underlying get_metadata APIs. We're looking to change that in 5.4 though. See #43941

[epiqueras]

I meant for the variant of get_post_meta that just takes a post ID and returns all keys.

[TimothyBJacobs]

If you are using a "multiple" meta key, then the REST API won't return default values either. This would change after 43941.

[epiqueras]

I see. What do you think the timeline for that change will be (43941)? I can hack this in the client for now if necessary.

[TimothyBJacobs]

The goal is 5.4, I think were trying for 5.4-early to give it as much time to soak as possible.

[TimothyBJacobs]

I created a trac ticket with an alternate way to solve the problem: https://core.trac.wordpress.org/ticket/48478

[epiqueras]

Thanks!

[TimothyBJacobs]

Following up on this, we chatted briefly about that idea at WordCamp US, and we weren't thrilled with the idea.

I was looking at some other code and I think restoring the previous behavior in Gutenberg would be a good idea for another reason too. If an invalid value is stored for a meta key, the REST API will surface this by returning the field as null. If you then try and send null back, you'll receive a rest_invalid_stored_value error: The %s property has an invalid stored value, and cannot be updated to null.. Simply filtering out null wouldn't work either, as null signals to the REST API to delete the meta key.

[epiqueras]

When the meta key has no value, either the empty value or a custom default in show_in_rest.schema.default is used.

I understand why with custom defaults we would want to save when the defaults are posted back. But, I don't think we should be saving the empty value. Is that something we can change? Otherwise, we are kind of breaking the stateless transfer contract of REST APIs and hacking a fix into what should be a general client library that works with any REST API.

The problem lies in the fact that we are not differentiating empty from default.

What I'm proposing is:

if default == empty && savedValue == undefined && postedValue == empty
  dontSave()

[TimothyBJacobs]

It’d be a BC break we’d have to justify. I’ll make sure it’s on the next meeting agenda. We aren’t meeting tomorrow because of the holidays, but for next week.

I’m not dead set against it, but even if we do make that change, there is still the issue with invalid values.

I’m still unclear on what actually changed in Gutenberg or what package the change happened in. Could you possibly illustrate what the hack would be?

[epiqueras]

I’ll make sure it’s on the next meeting agenda. We aren’t meeting tomorrow because of the holidays, but for next week.

Thanks :smile:

I’m not dead set against it, but even if we do make that change, there is still the issue with invalid values.

If you can't send back null, how do you delete an invalid value? Ideally, deleting a value should be done by omitting the key in the payload. Since it's being PUT.

I’m still unclear on what actually changed in Gutenberg or what package the change happened in. Could you possibly illustrate what the hack would be?

The saving was done in a more ad-hoc way instead of being in core-data which expects the PUT APIs to be stateless. We would have to hardcode a different behavior for WP post meta so that it only sends back the edited keys.

[TimothyBJacobs]

If you can't send back null, how do you delete an invalid value?

You can't. The idea is to not unintentionally remove data. I'm actually in the middle of writing the docs for that 😃. I think it would be a good idea to allow some way for that. Discussion on meta: https://github.com/WP-API/WP-API/pull/68

Ideally, deleting a value should be done by omitting the key in the payload. Since it's being PUT.

As we've found out through this issue and the other areas where you can't always PUT back what you GET. Semantics wise, the REST API treats PUT more as a PATCH. And of course without a form of If-Match, it would be dangerous to delete data if a key was omitted.

The saving was done in a more ad-hoc way instead of being in core-data which expects the PUT APIs to be stateless. We would have to hardcode a different behavior for WP post meta so that it only sends back the edited keys.

So core-data merges edits with the stored value and then does the update? I thought core-data was specific to WordPress?

[epiqueras]

You can't. The idea is to not unintentionally remove data.

Well, sending back null wouldn't be unintentional right?

it would be dangerous to delete data if a key was omitted.

Purely because of backwards compatibility?

So core-data merges edits with the stored value and then does the update?

Yes.

I thought core-data was specific to WordPress?

It works on an entity config abstraction which defines a REST endpoint among other things.

[TimothyBJacobs]

Well, sending back null wouldn't be unintentional right?

Yes, but the REST API can't disambiguate between you intentionally sending null and sending null because that is the value the REST API returned from GET and you PUT it back.

Purely because of backwards compatibility?

No. Because of the risk of conflicts. If you don't know if your GET response is fresh, and new values between the GET and PUT will be lost. This is also an issue for meta fields that are controlled outside of the post editor. If those changed, those changes would also be lost.

Yes

Is there an issue number describing the change? The more I think about merging with stored data, instead of only persisting the changed values, the more possible conflicts I can see. This would lose any edits that happened between writes, even if those fields are never exposed to Gutenberg.

It works on an entity config abstraction which defines a REST endpoint among other things.

Right, but those are all fetched from WordPress and use hardcoded WordPress routes. The package is also described as a package providing data for WordPress. I don't understand why this package would be CMS agnostic.

[youknowriad]

This would lose any edits that happened between writes, even if those fields are never exposed to Gutenberg.

This is the same for any edit on any API, if you don't have a fresh API response and you "PUT" a new value, any changes in the meantime are lost, not sure it should be a consideration. If we don't want to merge with stored data, the best way to achieve this is to provide "PATCH" endpoints. Can we consider that all existing PUT endpoints in Core are in fact PATCH endpoints?

[TimothyBJacobs]

This is the same for any edit on any API, if you don't have a fresh API response and you "PUT" a new value, any changes in the meantime are lost, not sure it should be a consideration.

That's not what I'm saying. To demonstrate what I think the incorrect behavior is in the most clear case, let's say field_1 is not editable in the Gutenberg interface. It is there because the REST API is used for more than just Gutenberg.

1. Make a GET to /wp/v2/posts/1. The following data is returned.

{
  "meta": {
    "field_1": "Hello",
    "field_2": "World"
  }
}

2. field_1 is updated by another process to "Hi".
3. wp.data.dispatch("core/editor").editPost({ meta: { field_2: 'Earth' } })
4. WordPress persists the following data.

{
  "meta": {
    "field_1": "Hello",
    "field_2": "Earth"
  }

Changes to field_1 are now lost. Despite the fact that field_1 has nothing to do with Gutenberg.

core-data is considering the entirety of meta as one value, but it isn't. Each value within the dictionary is distinct. This isn't like saying that if the title is updated on the server, when you edit the title on the client, the data will be lost.

Even though it is semantically incorrect, many APIs implement a looser version of PUT behavior. Many of the APIs I've personally encountered allow for PUT behavior where you can update with only a subset of fields.

Even when updating a nested property of a specific field, for instance how Stripe updates metadata.

The client also needs to be pragmatic.

Can we consider that all existing PUT endpoints in Core are in fact PATCH endpoints?

AFAIK all endpoints in Core have PATCH semantics and it is advertised as such in the Allow header.

I would personally like for the REST API to have different handlers for PUT and PATCH each following the correct semantics. But I don't think that could even be discussed as a possibility until we think about wp/v3 endpoints.

[youknowriad]

Changes to field_1 are now lost. Despite the fact that field_1 has nothing to do with Gutenberg.

That's exactly what I understood and what I was saying is that PUT requests are and should remove field_1 regardless of Gutenberg or not because they are PUT requests and that's fine, that's in the contract of PUT requests, if something happens between GET and PUT it's lost.

but you're saying that all endpoints are actually PATCH endpoints, that's good news and I think it means we should just update core-data to use PATCH and treat all updates as PATCH (not merge things when sending the requests and just send the edits).

[epiqueras]

Yes, but the REST API can't disambiguate between you intentionally sending null and sending null because that is the value the REST API returned from GET and you PUT it back.

That seems conflicting with the way sending back default values is handled and contradicts that the API has PATCH semantics for PUT.

No. Because of the risk of conflicts. If you don't know if your GET response is fresh, and new values between the GET and PUT will be lost. This is also an issue for meta fields that are controlled outside of the post editor. If those changed, those changes would also be lost.

That should be dealt with by making post meta a separate resource in the REST API, not by breaking REST contracts.

Is there an issue number describing the change? The more I think about merging with stored data, instead of only persisting the changed values, the more possible conflicts I can see. This would lose any edits that happened between writes, even if those fields are never exposed to Gutenberg.

17368

Right, but those are all fetched from WordPress and use hardcoded WordPress routes. The package is also described as a package providing data for WordPress. I don't understand why this package would be CMS agnostic.

It's API agnostic, so that custom blocks can sync to whatever APIs they need to sync with.

Even when updating a nested property of a specific field, for instance how Stripe updates metadata.

That's a POST endpoint.

AFAIK all endpoints in Core have PATCH semantics and it is advertised as such in the Allow header.

If this is true for all endpoints, then I agree with Riad that we should just change core-data to only send the edits for everything.

If it is not, then I think we should move forward with https://github.com/WordPress/gutenberg/issues/17864#issuecomment-559264504.

[TimothyBJacobs]

That seems conflicting with the way sending back default values is handled and contradicts that the API has PATCH semantics for PUT.

That's true, but it is how the REST API operates. We can't just change that without breaking backwards compatibility.

That should be dealt with by making post meta a separate resource in the REST API, not by breaking REST contracts.

I linked earlier to one of the many discussions about support post meta in the API. That is where it worked with separate routes. And it changed hereish to the version we have today: https://github.com/WP-API/wp-api-meta-endpoints/pull/15. I'm trying to find more reasoning around those changes.

That's a POST endpoint.

It is a POST endpoint. But it is for an update request. The REST API uses WP_REST_Server::EDITABLE for all edit routes which has the POST,PUT, and PATCH HTTP methods defined. I don't think the REST API has ever advertised itself as 100% adhering to the PUT semantics.

Again, I don't disagree about it not following the semantics. And I agree it would be ideal if we did follow the expected semantics and RFCs more closely. But I think we really have to be pragmatic here. And collect these improvements for a possible v3 at some point in the future.

[epiqueras]

It is a POST endpoint. But it is for an update request.

Yeah, non-REST APIs will use POST generically for any request that needs to send a payload.

Again, I don't disagree about it not following the semantics. And I agree it would be ideal if we did follow the expected semantics and RFCs more closely. But I think we really have to be pragmatic here. And collect these improvements for a possible v3 at some point in the future.

Totally agree, that's why my 2 suggestions for moving forward don't involve changing the semantics.

https://github.com/WordPress/gutenberg/issues/17864#issuecomment-559548705

One is a small fix to how meta default and null value saving works and the other is fully client-side, but to make a decision we first need to know if this PATCH behavior is true for all relevant endpoints.

[TimothyBJacobs]

Yeah, non-REST APIs will use POST generically for any request that needs to send a payload.

Sure, but then it is just an argument about what constitutes a REST API. Stripe describes its API as a REST API. And is generally heralded as a good example of API design.

Totally agree, that's why my 2 suggestions for moving forward don't involve changing the semantics.

Yeah, using PATCH works for me. I'll check in with @kadamwhite too.

One is a small fix to how meta default and null value saving works and the other is fully client-side, but to make a decision we first need to know if this PATCH behavior is true for all relevant endpoints.

I'm not aware of any wp/v2 endpoints that is not PATCH.

[epiqueras]

Sure, but then it is just an argument about what constitutes a REST API. Stripe describes its API as a REST API. And is generally heralded as a good example of API design.

https://restfulapi.net/

Yeah, using PATCH works for me. I'll check in with @kadamwhite too.

Great, thanks! :smile:

[jordesign]

@epiqueras @TimothyBJacobs Just looping back to this - can you please confirm whether or not this is still an issue that needs to remain open?