Implement post_meta insertion/update - #68

Thanks for the pull request; the request itself looks great :)

My only concern is how we handle the post meta. We need to provide some mechanism to delete post meta, so I think this needs to run through the existing meta and do an array_diff_key to find any meta keys that haven't been set, then delete those. What's your thoughts on that?

(Oh, and my one tip for any future pull requests: consider branching so that you can do multiple pull requests without needing to combine them. Not an issue for this, but could be in the future. :) )

some mechanism to delete post meta

I obviously didn't think this one bit through 😬

find any meta keys that haven't been set, then delete those.

Are we talking about a brute(ish) delete/recreate of the post_meta? I wasn't too certain either, actually

I obviously didn't think this one bit through 😬
No sweat 😃

Are we talking about a brute(ish) delete/recreate of the post_meta? I wasn't too certain either, actually

For example:

• Post's current metadata is { 'a': '1', 'b': '2' }
• Submitted metadata is { 'a': '2', 'c': '3' }

The way we could handle this is:

• Run the current code from your patch (update_post_meta with the array)
• Find any keys not in the submitted data that are in the database (using array_key_diff)
• Delete the missing keys (delete_post_meta)

May I ask, why should updating a post through the API delete all missing meta, when a save in WP admin does no such thing (that I'm aware of at least)?

May I ask, why should updating a post through the API delete all missing meta, when a save in WP admin does no such thing (that I'm aware of at least)?

I thought the admin did? Note that this isn't including underscore-prefixed meta, which isn't visible via the API.

Just to be clear, the final result would be what? { 'a': '2', 'c': '3' }or { 'a': '2', 'b' : '2', 'c': '3' }. The reason I ask is because of the additional, optional parameter of update_post_meta (omitted in this patch),

$prev_value
(mixed) (optional) The old value of the custom field you wish to change. 
This is to differentiate between several fields with the same key. 
If omitted, and there are multiple rows for this post and meta key, 
all meta values will be updated.
Default: Empty

Just to be clear, the final result would be what? { 'a': '2', 'c': '3' }or { 'a': '2', 'b' : '2', 'c': '3' }. The reason I ask is because of the additional, optional parameter of update_post_meta (omitted in this patch),

Sorry, to be clear, the result would be { 'a': '2', 'c': '3' }, deleting the missing b key.

I don't know of anyone that uses $prev_value in code, but it's certainly something we should consider supporting for the API, to avoid race conditions. That can probably be handled in a separate issue though, as long as we have the basic functionality sorted.

I thought the admin did? Note that this isn't including underscore-prefixed meta, which isn't visible via the API.

I just had a quick look through edit_post() in post.php, and it seems as though the only meta being deleted is passed in $_POST['deletemeta'], which to me seems like the missing meta key will be left undeleted.

I just had a quick look through edit_post() in post.php, and it seems as though the only meta being deleted is passed in $_POST['deletemeta'], which to me seems like the missing meta key will be left undeleted.

Hmm. Adding an extra key to contain meta to delete makes it unambiguous, but also breaks the entity-body to resource mapping, which would manifest as weird data in something like Backbone.

From a REST standpoint, excluding the entry from the post meta is really the only "correct" way to achieve this, but we might have to make tradeoffs here.

Just an idea..., not sure about "REST correctness":

Check for POST/PUT/PATCH null values on any subset of meta keys to delete a post meta key. By checking for any explicit null value a decision which keys to delete is fairly straightforward.

Or...

As far as I know POST is meant for creating and should occur on /index endpoint, but I don't see a reason why it could not occur on POST /index/<id> endpoint. Could be considered as "recreating".

It is a way around. This way delete would be possible only by POST and expect full set of keys, missing keys would be removed. Still an extra GET might be always necessary.

You can give it a try. Here's a quick patch for lib/class-wp-json-posts.php.

You can give it a try. Here's a quick patch for lib/class-wp-json-posts.php.

Thanks for that! I've only had a quick look so far, but I'll take a more in-depth one ASAP.

One thing: you should be able to get all the meta for a post by doing get_post_meta( $post_ID ) with no additional parameters. :)

We need meta_id column to delete a meta and get_post_meta() makes array of pairs meta_key => meta_value which is insufficient.

See wp-admin/includes/post.php, around line 241. Before deleting, term is first checked if its meta_id is written in DB using get_post_meta_by_id( $key ), where $key is integer. Function returns full row from wp_postmeta including the meta_id column.

We are sending post meta as key-value pairs, where key is meta_key not meta_id and there is no such method as get_post_meta_by_meta_key($meta_key) to get full row from wp_postmeta.

Even though there would be such one, when deleting many fields, many of DB calls will occur. I did it in one call up-front (performance), and since output was array of arrays, I recreated it to more appropriate array of indexes (meta_key) and values (meta_id) for later use. This way an array can be used with conjunction with array_key_exists() (I used is_set(), 'cause I guess I've read it's a little bit faster). If meta_key exists, the array pair has value of its meta_id and therefore can be used in the next delete_meta($meta_id).

FYI: In the meantime I've forked the project, and hacked it a little. Now messing around with the terms, which might require an endpoint to delete a term... breaks post_meta "convention". Will think of it a little bit more.

Cheers

We need meta_id column to delete a meta and get_post_meta() makes array of pairs meta_key => meta_value which is insufficient.

We should be able to do it with delete_post_meta, which is the higher-level API, right?

If those checks from post.php are not important to reimplement, than yes, it is a way to go.

One thing still bugs me: $unique parameter from add_post_meta($post_id, $meta_key, $meta_value, $unique); More than one meta_values with same meta_key can be attached to a post.

Here are some of my thoughts about how to rethink the feature:

One [post] to many post meta [of the same meta_key]

If more than 1 value exists in wp_postmeta, there is no good way to decide (without explicit meta_ids being provided) which values to update (or by sending old values side by side with new values in array). Neither way is nice, therefore we can only expect to update full array of metavalues of one meta_key and keep only those explicitly sent.

Why do we still treat post_meta as second class citizens?

Is there a reason why post meta are under post.post_meta. They could be easily merged with post attributes like content or excerpt and on update, metadata are all that are left and can be handled. When any attribute is set to null, than is truncated (content, etc.) or expecting to be autogenerated (name also known as slug), but meta would be deleted. If not send, nothing should happen (update nor delete).

Any thoughts?

One thing still bugs me: $unique parameter

Definitely agree. Multiple post meta is a pain to deal with, especially since it's not that common.

I'm going to think over this a bit and see if I can come up with a good solution. If you want to try treating post meta as a first-class citizen, I'd love to see what you'd come up with. :)

Thanks for all your work on this @attitude, and for caring about the project! Ditto for @zedejose for the initial patch and kicking off the effort here. 🍰

First-class they are. Check out the branch where post_meta is part of post attributes.

I have rewrote the update post_meta part, also with the repeating arrays of "loop fields". Would require some rewriting this experiment for production.

Examples:

• { "mark_as_read": 1 } — value is meant to be singular and only one value should exist, update or create happens
• { "colours": ["red", "green", "blue"] } — any other colours should be removed and only RGB is meant to remain (replace is default behaviour)
• { "colours": null } — destroys all values
• { "colours": [] } — destroys all values

PS: I am aware that post meta functionality works only with an extra page refresh to see the fresh data. Some caching is messing around with me probably.

@attitude Thanks for writing all of this up. I'm going to take a look at it this week and read over your notes and patch. :)

I don't see the point in merging post meta and post attributes. I am unsure whether delete post meta should be supported out-of-the-box. I think there should be a hook/filter that one can utilize to delete meta.

My take on this is similar to zedejose but offers a few extra advantages. First, it is isolated in it's own method. This is useful for people who end up extending the class and overwriting the insert_post method. Also, my code provides a mechanism for sanitizing meta before inserting them into the DB; this is absolutely critical IMHO.

New to WordPress but not to PHP, I'm trying to get the users endpoint to a point where I can use it for a paid project: https://github.com/tobych/WP-API/compare/users. I'd like to get something like it pulled into WP-API so I don't have to maintain my own fork. Well, things were straightforward until I hit metadata, and of course it's pretty much the same story as metadata for posts. What I did was add a user_meta member to the User entity. I'm certainly not doing everything gorgeously though and reading this thread I realize there are plenty of alternatives.

The non-meta part of my User representation looks like this:

{
    "ID": 4861,
    "name": "Toby Champion",
    "slug": "tobychampion",
    ...
    "registered": "2009-10-09 00:00:00",
    "meta": {
        "links": {
            "self": "http://example.com/wp-json/users/4861",
            "archives": "http:/example.com/wp-json/users/4861/posts"
        }
    },

So far, pretty much the same as the representation used in a Post's author.

The metadata part adds a user_meta member:

    "user_meta": {
        "first_name": [
            "Toby"
        ],
        "last_name": [
            "Champion"
        ],
       "rich_editing": [
            "true"
        ],

So far, so straightforward. Everything's a string. And every metadata field is representation as an array; so far each has only one value, so they're all single-length arrays. Over-the-top perhaps, but consistent.

Then come fields that are serialized using PHP's serialize() method. I'm doing this:

        "wp_capabilities": [
            {
                "unserialized": {
                    "subscriber": true
                },
                "serialized": "a:1:{s:10:\"subscriber\";b:1;}"
            }

That solves the serialization problem for me. But what about single vs multiple values for each key? As with posts, most metadata fields have just one value. Usually, more than that wouldn't make sense. At this point I realized I was up against history versus current usage in WordPress and all sorts of other issues, and don't know what I'm doing. Certainly, if the client POSTs or PUTs a representation using a single value, my server code treats it just as it would a single-length array.

But which of these should GET give you?

    "first_name": [
        "Toby"
    ],

or

    "first_name": "Toby",

Should properly RESTful representations have just the one true way, or is some flexibility okay? I imagine it would be best just to have things look like the second.

As to deleting metadata values, it seems to me that it'd be best to treat metadata items as first-class entities for this purpose, and perhaps to use the same representation inline in the User representation. I'm wondering whether @rmccue (Ryan) and @attitude (Martin) have the same understanding of "first class", above. I would have understood first class as being a separate resource, so for example /wp-json/users/65/meta/description/32 is the metadata entry with with umeta_id=32 (you could even leave out the description piece in the path). That way if there were multiple values for a key, each with large bunch of text, you could delete one of them (with DELETE) without needing to include all the text in the POST (which might even have changed since you did your GET).

@tobych From my quick glance over your code, I'm liking it. Can you open a pull request for that code, but with the meta handling removed? We can discuss user specifics on a separate ticket. :)

My thinking at the moment is that we should handle all of the meta handling at once, separate to the other endpoints, so the user stuff can be handled independently of how we handle this ticket.

I will come back and look at the rest of this ticket. We need to work out post meta, and get it handled. We've got a couple of alternative approaches here, and I need to thinking over how we handle it.

We need to handle all of the following:

• Meta retrieval
• Meta creation
• Meta updating
• Meta deletion

We can't leave any of these out. In addition, we need to work out a system that works for not just post meta, but other types of meta (such as user meta). If we don't, we'll introduce a lot of client complexity to handle all the different types of meta.

I'm not against including it both with the entity and also as separate endpoints, but we need to think through whether this is appropriate.

Again, I will come back and add more thoughts, these are just some quick ones as I do triaging.

Will do!

I don't see the point in merging post meta and post attributes.

Agreed on this one; let's keep it namespaced under post_meta as-is for now. Plugins can add their own handling for other fields if needed.

@tlovett1 I like your approach in tlovett1/WP-API@3ef9712 so far, but needs to handle the other cases.

I'm wondering whether @rmccue (Ryan) and @attitude (Martin) have the same understanding of "first class", above. I would have understood first class as being a separate resource

Indeed; I meant first-class handling to have the meta as separate resources available from e.g. /posts/<id>/meta/<key>

But what about single vs multiple values for each key? As with posts, most metadata fields have just one value.

Right. The issue with this is as follows:

• Meta fields can have multiple values, and we don't know ahead of time whether fields have multiple values or not (it's a flag you use on get_post_meta/update_post_meta, not in something like register_post_meta)
• If we use arrays and handle serialization of values, what's the difference between multiple values, and a single value containing an array?
• If we use arrays and don't handle serialization of values (for now), how do you update/delete individual values? We can either use the previous-value parameter to update_post_meta, or use the meta ID; the former could have race conditions, the latter means we need to flesh meta out into a full object (or think of a nicer way to handle it)
• If we use arrays, it makes the API much less nice, since meta values are arrays even for the common case of single values.
• If we don't handle serialization, we pass that burden off to the client and introduce complexity there
• If we do handle serialization, we might pass objects/etc through the API when they can't be represented that way. This also affects updating.

You can easily see why this is a contentious issue. :)

Here's what I propose:

• Handling serialized values
  • Ignore any serialized values from retrieval (is_serialized) but allow strings (is_serialized_string)
  • Give an error when attempting to update/add serialized values or arrays/objects
• Handling multiple values
  • Return an array of values for the get_post data
  • Add /post/<id>/meta/<key> with more data as needed, including meta IDs
  • Add /post/<id>/meta/<key>/<id> for specific values, allowing updates/deletion of those values

We can try this approach with post meta first, and if it works, port it to user meta too.

In the future, we can think about serialized meta handling separately, but I want to get something shippable here.

Thoughts? I'd like to get some action happening on this ticket and get some momentum, but we should all try and agree on a baseline here.

@rmccue , let me know what you think of this one. Update/add/delete post meta. Assume values are passed as they are intended to be stored. This isn't the prettiest solution, but in my opinion the only way to really handle all the cases you mentioned.

The commit needs a little cleaning. Sorry about that.

let me know what you think of this one. Update/add/delete post meta. Assume values are passed as they are intended to be stored.

Doesn't appear to give an error for serialized values. Also, sanitization should be handled at a lower level; see sanitize_meta and register_meta which already handle this.

@rmccue Removed the sanitization callback stuff. I don't understand the serialization problem you are trying to solve. If you want to store a single value in a meta, you send a string/int/boolean. If you want to store an array in meta, you pass an array. If you want to store meta multiple times with the same key, you set the action to "add".

I don't understand the serialization problem you are trying to solve. If you want to store a single value in a meta, you send a string/int/boolean. If you want to store an array in meta, you pass an array. If you want to store meta multiple times with the same key, you set the action to "add".

Right, but when you're retrieving the data back, what if you get something like...

[
    "a",
    "b"
]

Is that a single value that's an array, or multiple values?

@rmccue - Ah I see. I was only thinking about pushing content not pulling it. I think the best way to handle this is to represent the data like it is in the database:

'post_meta' : {
   'key1' : [ 'a', 'b', 'c' ],
   'key2' : 'x',
   'key3' : 'i',
   'key3' : 'm'
}

I think the best way to handle this is to represent the data like it is in the database:

Right, so then...

'post_meta' : {
   'key1' : [ 'a', 'b', 'c' ],
   'key2' : [ 'a', 'b', 'c' ]
}

If key1 is multiple string values and key2 is a deserialized array, how do you tell the difference?

Similarly, if a value is { 'a': 'b' }, is the PHP representation of that $o = new stdClass; $o->a = 'b'; or $o = new WP_User; $o->a = 'b';?

Deserializing means we have to face possible lossy data update/insertion, plus a confusing API, which is why we should just skip handling it for now so that we have something to ship.

@rmccue - I see what you are saying now. Following the convention I have for pushing data:

'post_meta' : [
   {
     'key' : 'key1',
     'value' : 'a'
   }
   {
     'key' : 'key1',
     'value' : 'b'
   }
]

Regarding, not knowing if something is a user or not. When pulling from the API you should be expecting specific data. I'm not looking for users when pulling for posts. I don't like the idea of sending serialized data. It just seems hacky. I think we sacrifice the strange data loss situation for the less difficult-to-use API.

I might be inclined to agree with you, if you could provide an example of a situation someone might actually encounter where their data gets serialized and they don't know how to handle it.

I agree something should be shipped on this ASAP. I think we are over-complicating.

I don't like the idea of sending serialized data.

Right; that's why I'm thinking we just ignore it completely for now, and don't send/handle it at all. The only real issue we have with doing so is that not all data will be displayed, but we're already hiding data (is_protected_meta()), so I don't have an issue with that.

I might be inclined to agree with you, if you could provide an example of a situation someone might actually encounter where their data gets serialized and they don't know how to handle it.

Real life example from some of my own code:

class Token {
    protected $data;

    public function __construct( $key, $secret, $expiration ) {
        $this->data = array(
            'key' => $key,
            'secret' => $secret,
            'expiration' => new DateTime( $expiration ),
        );
    }

    // ...
}
// ...
$data = new Token( $key, $secret, $expiration );
update_post_meta( $post, 'token', $data );

Storing that in serialized form is fine, however the JSON-encoded version is useless: {} - This is because it contains protected properties, which aren't encoded into JSON data.

Hence, a GET request would give token: {} (assuming a simplified schema for argument's sake). A naive consumer/user could believe this is empty.

Worse than that, doing a GET, then POSTing the data is no longer idempotent, as it would overwrite the Token object with a blank stdClass object.

Interesting example.

Where should we go from here? What is left that is blocking a merge? Certainly some unit tests are needed.

See #168 for the current approach and related feedback in relation to this issue.

Posted for feedback on the o2.