WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

#10521 closed defect (bug) (fixed)

Schema error breaks XMLRPC image uploading

Reported by: Otto42 Owned by: josephscott
Milestone: 2.9 Priority: high
Severity: major Version: 2.8
Component: XML-RPC Keywords: dev-feedback
Focuses: Cc:

Description

Issue is described in greater detail here:
http://ardentdev.com/fix-for-wordpress-xmlrpc-500-internal-server-error/

Short version: Uploading an image or other attachment via XMLRPC tries to set the post_parent of the upload to -1, and then changes the value to the correct ID after the post is created and the ID is known.

In WordPress 2.8 and up, this fails, because the schema changed to define the post_parent as unsigned, so -1 becomes invalid. This disables the capability for image attachments to work via XMLRPC uploads.

Fix: Either change the way uploads work (use a 0 for post_parent perhaps?) or change the schema back to not being unsigned on post_parent.

Attachments (2)

xmlrpc-post_parent-patch.txt (479 bytes) - added by c0nstruct 6 years ago.
Patch used to fix my xml-rpc.php
10521.diff (895 bytes) - added by ryan 5 years ago.
Refreshed patch

Download all attachments as: .zip

Change History (28)

comment:1 @Otto426 years ago

  • Version set to 2.8

Change that broke this was here: [10852] , for ticket #9422.

comment:2 follow-up: @Denis-de-Bernardy6 years ago

Why is it not trying to insert it using a post_parent of 0 instead?

comment:3 in reply to: ↑ 2 @Otto426 years ago

Replying to Denis-de-Bernardy:

Why is it not trying to insert it using a post_parent of 0 instead?

That was my suggestion as well. Seems to me that zero would work okay.

You'd have to change xmlrpc.php. At a glance, I'd say to alter mw_newMediaObject and attach_uploads to make them both use zero instead of -1. Needs testing to be sure it doesn't interfere with anything else.

comment:4 @Otto426 years ago

Alternatively, if zero does turn out to be a problem, we can just be sooper clever about it and set it to the max bigint (264, or 18446744073709551615).

If somebody can get that many posts, they deserve to have their database kersplode. ;)

comment:5 follow-up: @Denis-de-Bernardy6 years ago

It shouldn't be any problem. It might be the software that's using the XML-RPC interface. But still, we could catch a negative parent and turn that into a 0 instead...

comment:6 in reply to: ↑ 5 @Otto426 years ago

Replying to Denis-de-Bernardy:

It shouldn't be any problem. It might be the software that's using the XML-RPC interface. But still, we could catch a negative parent and turn that into a 0 instead...

Oh, no, the client software is not sending it. It's in the xmlrpc.php file, definitely.

Line 2877 is where it gets set to -1 when the image is uploaded.

Line 2272 is where it searches for the -1 entries and corrects their parent id's (after the actual post comes in and gets created, since uploads happen first).

My worry is that we're using zero in that field somewhere else for something else and using that could cause interference. But I don't think that will be an issue, just that it's possible.

comment:7 @Denis-de-Bernardy6 years ago

you're kidding me. you're meaning there's potential for akward behavior when race conditions occur?

comment:8 follow-up: @Denis-de-Bernardy6 years ago

  • Keywords dev-feedback added

when do we switch to using UUIDs all over for this kind of stuff?

comment:9 in reply to: ↑ 8 @Otto426 years ago

Replying to Denis-de-Bernardy:

you're kidding me. you're meaning there's potential for akward behavior when race conditions occur?

No need for that now. :-P

Replying to Denis-de-Bernardy:

when do we switch to using UUIDs all over for this kind of stuff?

Agreed, proper UUIDs would eliminate this issue, more or less. However, few problems with using them for IDs.

  • People are already annoyed that their post_id's are no longer sequential. No, I don't get it either.
  • Lots and lots and lots of code that uses ints for ID's. All of that going to change?
  • Database bloat. UUIDs are twice the size of bigints.

Plus you're always going to get the whole "where's the advantages" argument, of which I can only think of one, which is that UUID identifiers on everything makes it a lot easier to merge/split databases.

Right now, UUIDs have a lot of downsides. Unless we made them entirely internal (no exposure to the outside world) and left some of the int id remnants for people to look at, it would be awkward.

comment:10 @Denis-de-Bernardy6 years ago

oh, I was meaning something similar to what's discussed here: #9471

comment:11 @josephscott6 years ago

  • Cc joseph@… added

I've been trying to reproduce this but so far haven't had any luck. Given the conditions though it seems like this would be a problem, so it's worth looking at changes to address it. Perhaps there's a strict MySQL setting that I don't have enabled, or a difference between MySQL versions or table types.

I'm going to dig through the code path more and run a few more tests. Switching to a post_id of zero sounds like it would be safe, so I'll focus on testing that.

@c0nstruct6 years ago

Patch used to fix my xml-rpc.php

comment:13 in reply to: ↑ 12 @c0nstruct6 years ago

Replying to josephscott:

Another confirmation on this issue: http://iphone.forums.wordpress.org/topic/attachments-post_parent?replies=7#post-873

If it's of any use, I am having trouble with the same issue. I have changed the xml-rpc.php file in my dev instance to use '0' instead of '-1' for the orphan parent post_id and it now works just like expected.

I am attaching the patch I have applied to xml-rpc.php.

comment:14 @leenewton6 years ago

I had put a ticket in over on the wpmu side after I'd debugged this on our setup. Should have looked here first :) Anyway regarding setting the post_parent to 0 rather than -1; the problem I see here is wp installations with a lot of activity most likely have very many unattached images. The default behaviour is to query for all post_parents = -1 where the guid matches what's in the post_content for the post added after the image upload. If this was changed to 0, a great many posts would potentially be looped through.

comment:15 follow-up: @brianlayman6 years ago

I decided to get some actual numbers from 10 of b5's blogs to see how many string compares attach_uploads() would have to do with the current patch via: "SELECT count(*) FROM wp_#_posts WHERE post_parent = '0' AND post_type = 'attachment'"

The counts I came up with were 6758, 5027, 671, 1200, 3684, 4377, 3095, 21776, 15885, & 9846.

It looks like many of our individual entertainment blogs will be up in the 5k-10k range. So looping through ALL of those records, for every new attachment when publishing a 5 attachment post, would probably produce noticeable lag.

Heck, for that one blog it would mean 65,328 if statements before the loop was exited, for just one attachment.

Plus it would be better if we didn't make our db throw around that many records around the network when we know the overwhelming majority don't match. If we must keep this index unsigned, we could consider throwing an index on the quid column and allowing the comparison with $post_content to occur there. I'd have to do some testing to see which method would be more costly.

comment:16 in reply to: ↑ 15 ; follow-up: @Otto426 years ago

It wouldn't loop through them every time. It'd attach them all to the first post made via XML-RPC. Then they wouldn't have a post_parent of zero any more.

Clearly, using zero instead of -1 isn't a viable option, because having attachment posts with a zero parent is possible using the normal media uploader, which creates "unattached" attachment posts.

comment:17 in reply to: ↑ 16 ; follow-up: @leenewton6 years ago

Replying to Otto42:

It wouldn't loop through them every time. It'd attach them all to the first post made via XML-RPC. Then they wouldn't have a post_parent of zero any more.

Not to nitpick but that's not how I read the xmlrpc, mw_newPost calls attach_uploads which will only change the post_parent if( strpos( $post_content, $file->guid ) !== false ) if an img src match isn't found while processing the post uploaded via xmlrpc then nothing happens and all those images will be involved in subsequent post creation. So while this will capture and reassign the image you uploaded, all other post_parent = 0 rows will remain untouched.

Clearly, using zero instead of -1 isn't a viable option, because having attachment posts with a zero parent is possible using the normal media uploader, which creates "unattached" attachment posts.

What he said.

comment:18 in reply to: ↑ 17 @Otto426 years ago

Replying to leenewton:

Not to nitpick but that's not how I read the xmlrpc, mw_newPost calls attach_uploads which will only change the post_parent if( strpos( $post_content, $file->guid ) !== false ) if an img src match isn't found while processing the post uploaded via xmlrpc then nothing happens and all those images will be involved in subsequent post creation. So while this will capture and reassign the image you uploaded, all other post_parent = 0 rows will remain untouched.

Ahh, yes. Missed that bit. Fair enough.

Still would be a problem of speed if you've got a few thousand zero'd attachments.

comment:19 follow-up: @ryan5 years ago

Introduce an "unattached" post_status?

comment:20 in reply to: ↑ 19 @Otto425 years ago

Replying to ryan:

Introduce an "unattached" post_status?

If you do that, and it doesn't get changed back to attachment (when the post is added), then the attachment won't show up in the media section. Unless you modify all that jazz as well.

comment:21 @ryan5 years ago

We could modify the attachment query to look for inherit|unattached. Since there is no parent to inherit from, unattached kinda makes sense.

comment:22 @ryan5 years ago

Go with 0 for now, see how it goes, mess with post_status if need be?

comment:23 @Otto425 years ago

Zero will fix the problem for now, even if it is not optimal for sites with lots of unattached images.

I say to do zero now simply to get it working *at all*, work on a better way for a long term fix.

@ryan5 years ago

Refreshed patch

comment:24 @ryan5 years ago

Looks like the detached check in upload.php and elsewhere looks for < 1 so we're still good there.

comment:25 @ryan5 years ago

(In [11969]) Use a post_parent of 0 instead of -1 to indicate unattached posts. Props c0nstruct. see #10521

comment:26 @nacin5 years ago

  • Milestone changed from Unassigned to 2.9
  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.