WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 9 months ago

Last modified 9 months ago

#18310 closed defect (bug) (fixed)

Attachments inserted with NULL value for GUID

Reported by: docfish Owned by: nacin
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.2.1
Component: XML-RPC Keywords: has-patch mobile commit dev-feedback
Focuses: Cc:

Description

This is the error I get when trying to post via xml-rpc interface:

Empty delimiter in .../wp-includes/class-wp-xmlrpc-server.php on line 2469 Warning: Cannot modify header information - headers already sent by (output started at .../wp-includes/class-wp-xmlrpc-server.php:2469) in .../webraces/wp-includes/class-IXR.php on line 471

The line 2469 of class-wp-xmlrpc-server.php looks like:

if ( strpos( $post_content, $file->guid ) !== false )

Apparently $file->guid has invalid value as this is fixed by inserting

if($file->guid && !($file->guid == NULL))

right above the line in question. See also http://wordpress.org/support/topic/windows-live-writer-no-go-with-wp-312

Attachments (3)

18310.diff (518 bytes) - added by ericmann 3 years ago.
Automatically inserts a GUID if none was passed to wp_insert_attachment()
18310-2.diff (1.2 KB) - added by ericmann 20 months ago.
Patch refresh, and also check that $file->guid is actually set before using it.
18310-3.diff (1.2 KB) - added by markoheijnen 12 months ago.
No isset but empty check. Also change arrray to array

Download all attachments as: .zip

Change History (20)

comment:1 ericmann3 years ago

I'd want to look at that a bit deeper. Your fix would remove the error, but wouldn't fix the underlying problem.

In this case, $file is an element returned directly from $wpdb->get_results(). If it contains a null value, then something else is going wrong when attachments are being inserted into the database. I'd want to see what exactly get_results() is returning when this error happens and fix the cause of that rather than apply a bandaid.

$file->guid shouldn't be empty ...

comment:2 solarissmoke3 years ago

  • Keywords reporter-feedback added

Can you give us a sample XML-RPC request that triggers this error? I can't reproduce it and like ericmann says, $file->guid shouldn't be empty.

comment:3 jcox3 years ago

I encountered this error after using wp_insert_attachment() without including a guid value, because guid is not listed as a required key/value for the $attachment array in the function reference at http://codex.wordpress.org/Function_Reference/wp_insert_attachment.

Last edited 3 years ago by jcox (previous) (diff)

comment:4 ericmann3 years ago

  • Component changed from XML-RPC to Database
  • Keywords has-patch added; reporter-feedback removed
  • Severity changed from major to normal
  • Summary changed from xml-rpc posting returns Empty delimiter error to Attachments inserted with NULL value for GUID

True, guid is not a required key/value for that $attachment array in that function. And that is where the problem is, not in XML-RPC.

By default, the GUID is set to if nothing is passed in the $attachment array. Unfortunately, that's one of the only fields not sanitized/reset by WordPress before actually inserting the attachment.

In my opinion, this is a defect. $file->guid shouldn't be empty. If no guid is passed to wp_insert_attachment(), then a guid should be created along the same lines as for wp_insert_post().

I'm attaching a patch that updates the attachment to set the GUID if it wasn't originally sent in the $attachment array.

This should fix the real problem of a null GUID for attachments, which will also fix the observed issue with XML-RPC.

ericmann3 years ago

Automatically inserts a GUID if none was passed to wp_insert_attachment()

comment:5 iandunn2 years ago

  • Cc ian_dunn@… added

Thanks for writing the patch Eric. I agree that a GUID should be generated if one isn't passed to wp_insert_attachment(), but that doesn't solve the problem reported here by docfish and also on the Android app forums.

This line in attach_uploads() in wp-includes/class-wp-xmlrpc-server.php will still generate a PHP warning when $file->guid is empty because of the invalid paramater.

if ( strpos( $post_content, $file->guid ) !== false )

That warning causes some XMLRPC clients to freak out and display an error to the user, even though the post content and upload are successfully posted.

So, before calling strpos(), attach_uploads() also needs to check for a valid GUID, and if necessary, generate one.

Maybe the code that generates the GUID in both functions (and possibly others) should be refactored into its own function so that it's DRY.

Last edited 2 years ago by iandunn (previous) (diff)

comment:6 nacin21 months ago

  • Component changed from Database to XML-RPC

ericmann20 months ago

Patch refresh, and also check that $file->guid is actually set before using it.

comment:7 ericmann20 months ago

New patch refreshes the existing one (setting the GUID if it wasn't previously set). But it also throws an isset() check on $file->guid inside attach_uploads() to fix the existing errors.

comment:8 julien.stlaurent18 months ago

The attach_uploads function also makes a similar query, at line 3955, and would require the same 'isset' check.

comment:9 koke13 months ago

  • Cc koke added
  • Keywords mobile added

comment:10 markoheijnen13 months ago

  • Milestone changed from Awaiting Review to 3.6

Moving to 3.6 since this issue is causing apps to stop working. the isset wouldn't work. It should just be ! $file->guid ?

Also not sure why wp-includes/post.php should be fixed.

comment:11 markoheijnen13 months ago

never mind on the post.php fix. Understand it now after reading comment 4.

comment:12 markoheijnen12 months ago

In 1278/tests:

Add test to check if guid isn't empty when not provided. See #18310

markoheijnen12 months ago

No isset but empty check. Also change arrray to array

comment:13 markoheijnen12 months ago

  • Keywords commit dev-feedback added

It's ready to be committed. Only need a pair of eyes on setting guid when it was empty.

comment:14 nacin9 months ago

If no guid is passed to wp_insert_attachment(), then a guid should be created along the same lines as for wp_insert_post().

Sure, I guess. But that's not really part of the bug fix here. That's a separate discussion (which probably involves #21963).

comment:15 nacin9 months ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In 24639:

Avoid notice in XML-RPC when attaching uploads, when attachments do not have a guid in the DB. props ericmann, markoheijnen. fixes #18310.

comment:16 nacin9 months ago

The notice is for an empty delimiter. isset( $guid ) is therefore not sufficient; if it comes back as an empty string or the like, the strpos() call will fail and emit a warning. Note, #23811 more or less fixes this anyway.

I'm going to suggest a new ticket for wp_insert_attachment()... I'm leaving a comment in #21963 as well.

Last edited 9 months ago by nacin (previous) (diff)

comment:17 nacin9 months ago

Code in wp_insert_attachment() should probably model wp_insert_post() here:

if ( !$update && '' == $current_guid )
	$wpdb->update( $wpdb->posts, array( 'guid' => get_permalink( $post_ID ) ), $where );

The ! $update is key. Anyway, #21963 or a new ticket.

Note: See TracTickets for help on using tickets.