WordPress.org

Make WordPress Core

Opened 9 months ago

Closed 8 months ago

Last modified 8 months ago

#24822 closed defect (bug) (invalid)

XMLRPC wp.editPost doesn't update existing keys

Reported by: jmaimarc Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.3
Component: XML-RPC Keywords: has-patch
Focuses: Cc:

Description

file: wp-includes/class-wp-xmlrpc-server.php
line 288:
Currently, the code is:

update_metadata_by_mid( 'post', $meta['id'], $meta['value']);

Since it already checks for an existing key, shouldn't the code be:

update_metadata_by_mid( 'post', $meta['id'], $meta['value'], $meta['key'] );

or at least switch between a key and FALSE (default value)?

Attachments (1)

24822.diff (722 bytes) - added by Zengy 9 months ago.

Download all attachments as: .zip

Change History (13)

comment:1 SergeyBiryukov9 months ago

  • Version changed from trunk to 3.3

Related: [18445], [18501].

Zengy9 months ago

comment:2 Zengy9 months ago

Submitting a patch with the suggested code. I reviewed core and it does appear that this param was missing and should be added.

comment:3 DrewAPicture9 months ago

  • Cc xoodrew@… added
  • Keywords has-patch added; needs-patch removed

comment:5 nacin8 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

If you look a few lines above, the metadata is only edited if the passed key is equal to the key in the DB. This is by design.

We don't allow for the updating, via XML-RPC, of a key of an existing row of metadata. You'd have to delete the metadata and re-create a new key/value pair.

comment:6 follow-up: jmaimarc8 months ago

Okay, but shouldn't that be the user's choice, as long as the mechanism is available?

comment:7 in reply to: ↑ 6 nacin8 months ago

Replying to jmaimarc:

Okay, but shouldn't that be the user's choice, as long as the mechanism is available?

It's related in part to metadata security. The XML-RPC API is rich enough for whatever you need to do here, you just need to use it properly.

comment:8 follow-up: jmaimarc8 months ago

I have to continue to disagree. I don't mean to be obtuse. I want to understand.

The function, as it's written, checks if 'id' is set, or adds a new postmeta row. WP allows to check for unique meta keys (c.f. add_post_meta() ), but doesn't employ it in this function. (This actually refers to ticket #25146, but it's all related).

That means either the burden of storing the WP postmeta ids is on the remote application, or the remote application has to double the work by first fetching the post data via wp.getPost, parse out the custom fields, set the updated custom field array with id,key & value , and then send the wp.editPost back. That will cause response delay, increase bandwidth, potential routing errors, hurt kittens, etc. I'm not sure this is the most efficient way to do it.

I'm also not sure what metadata security refers to. You mean data tainting?

comment:9 in reply to: ↑ 8 DrewAPicture8 months ago

Replying to jmaimarc:

<snip>
That means either the burden of storing the WP postmeta ids is on the remote application, or the remote application has to double the work by first fetching the post data via wp.getPost, parse out the custom fields, set the updated custom field array with id,key & value , and then send the wp.editPost back. That will cause response delay, increase bandwidth, potential routing errors, hurt kittens, etc. I'm not sure this is the most efficient way to do it.

I'm also not sure what metadata security refers to. You mean data tainting?

I hit this on a recent project when we noticed multiple values were being stored on the same meta key without overwriting. We used the exact suggestion @nacin made above about deleting the key first then re-writing the value and it works fine.

Since our custom method was basically a clone of wp.editPost() and still leveraged _insert_post(), we had to effectively "flush" the key before trying to update it so that server-side get_post_meta() calls could return only the first (and now most-recent) value.

There was no performance hit to my knowledge. As @nacin made pretty clear above:

The XML-RPC API is rich enough for whatever you need to do here, you just need to use it properly.

In our case, we used a custom method brought in via the xmlrpc_methods filter.

comment:10 follow-up: nacin8 months ago

I hit this on a recent project when we noticed multiple values were being stored on the same meta key without overwriting. We used the exact suggestion @nacin made above about deleting the key first then re-writing the value and it works fine.

Deleting the key and then re-adding is only if you wanted to take the same row in the metadata table and change the key.

If all you want to do is update an existing key, then you need to pass the meta ID. This is the unique identifier for metadata in the XML-RPC API and is very important to ensuring that deletes, adds, and updates all happen properly.

comment:11 in reply to: ↑ 10 DrewAPicture8 months ago

Replying to nacin:

I hit this on a recent project when we noticed multiple values were being stored on the same meta key without overwriting. We used the exact suggestion @nacin made above about deleting the key first then re-writing the value and it works fine.

Deleting the key and then re-adding is only if you wanted to take the same row in the metadata table and change the key.

If all you want to do is update an existing key, then you need to pass the meta ID. This is the unique identifier for metadata in the XML-RPC API and is very important to ensuring that deletes, adds, and updates all happen properly.

Hmm, I guess I assumed the struct for custom fields was the same for wp.editPost() and wp.newPost(), apparently not. That's what I get for assuming.

This kind of situation where there's accurate docs in the Codex and basically none in the source makes me wonder what else is missing elsewhere. It also leads me to commend @maxcutler or whomever wrote the original Codex article for this method.

comment:12 jmaimarc8 months ago

@nacin

It occurred to me last night that the function's way (your way...) was the only way *to* include multiple meta_key in the postmeta table, if desired.

So I'm fetching, parsing and resending. Not as horrific as I thought; No kittens were hurt.

Thank you for making me think.

Note: See TracTickets for help on using tickets.