Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Implement post_meta insertion/update #68

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants

Logic to read post_meta from the sent json and update a post with it. Code has been moved to after the post is created or updated, since we need an ID for update_post_meta.

This is my first pull request ever, if there are errors or misconceptions please be so kind as to tell me what I'm doing wrong

Owner

rmccue commented Jan 17, 2014

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?

Owner

rmccue commented Jan 17, 2014

(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

Owner

rmccue commented Jan 17, 2014

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

alisspers commented Jan 17, 2014

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

Owner

rmccue commented Jan 17, 2014

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
Owner

rmccue commented Jan 17, 2014

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.

Contributor

alisspers commented Jan 17, 2014

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.

Owner

rmccue commented Jan 17, 2014

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.

Owner

rmccue commented Feb 13, 2014

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

Owner

rmccue commented Feb 13, 2014

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?

Owner

rmccue commented Feb 13, 2014

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.

@rmccue rmccue added this to the 1.0 milestone Mar 4, 2014

@rmccue rmccue added the Enhancement label Mar 4, 2014

Owner

rmccue commented Mar 4, 2014

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

tlovett1 added a commit to tlovett1/WP-API that referenced this pull request Mar 5, 2014

Member

tlovett1 commented Mar 5, 2014

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.

Contributor

tobych commented Mar 30, 2014

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

Owner

rmccue commented Apr 6, 2014

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

@rmccue rmccue referenced this pull request Apr 6, 2014

Closed

Add user endpoints #20

Contributor

tobych commented Apr 6, 2014

Will do!

Owner

rmccue commented Apr 28, 2014

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.

Owner

rmccue commented Apr 28, 2014

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.

tlovett1 added a commit to tlovett1/WP-API that referenced this pull request Apr 28, 2014

Post meta handler; Allow for updating, adding, and deleting of post m…
…eta. Filter for passing sanitization callbacks; filter for requiring santizition callbacks. This method assumes values are passed as they are intended to be stored (array or single). In response to #68
Member

tlovett1 commented Apr 28, 2014

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

Owner

rmccue commented Apr 29, 2014

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.

tlovett1 added a commit to tlovett1/WP-API that referenced this pull request Apr 29, 2014

Member

tlovett1 commented Apr 29, 2014

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

Owner

rmccue commented Apr 29, 2014

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?

Member

tlovett1 commented Apr 29, 2014

@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'
}
Owner

rmccue commented Apr 29, 2014

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.

Member

tlovett1 commented Apr 29, 2014

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

Owner

rmccue commented Apr 30, 2014

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.

Member

tlovett1 commented Apr 30, 2014

Interesting example.

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

tlovett1 added a commit to tlovett1/WP-API that referenced this pull request May 3, 2014

Post meta handler; Allow for updating, adding, and deleting of post m…
…eta. Filter for passing sanitization callbacks; filter for requiring santizition callbacks. This method assumes values are passed as they are intended to be stored (array or single). In response to #68

Remove sanitization callback functionality - see #68
Owner

rachelbaker commented May 8, 2014

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

Owner

rmccue commented May 12, 2014

Posted for feedback on the o2.

@rachelbaker rachelbaker closed this in #207 May 24, 2014

rachelbaker added a commit that referenced this pull request May 24, 2014

Merge pull request #207 from WP-API/post-meta-handling-v4
Add ability to add, update, delete post_meta.  Fixes #68.  Closes #189 and #168.

kellbot pushed a commit to kellbot/WP-API that referenced this pull request Aug 1, 2014

Post meta handler; Allow for updating, adding, and deleting of post m…
…eta. Filter for passing sanitization callbacks; filter for requiring santizition callbacks. This method assumes values are passed as they are intended to be stored (array or single). In response to #68

Remove sanitization callback functionality - see #68

kellbot pushed a commit to kellbot/WP-API that referenced this pull request Aug 1, 2014

Merge pull request #207 from WP-API/post-meta-handling-v4
Add ability to add, update, delete post_meta.  Fixes #68.  Closes #189 and #168.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment