WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 3 years ago

#39439 new enhancement

Should wp_insert_attachment() update GUID

Reported by: jlambe Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.8
Component: Posts, Post Types Keywords: dev-feedback
Focuses: Cc:
PR Number:

Description

I have a "special" case scenario related to media library attachments.

For the context, I've built a plugin that connects to an external server and sync images stored into the remote server into local WordPress media library. The importer upload files, generate the thumbnails and create a new "attachment" post.

Now, an import process is ran everyday, and basically we re-import the remote images into WordPress but in order to avoid duplicates, we're updating existing attachments if exist. Now, during this update process, we only remove the files stored into the uploads directory, import the new one with its thumbnails and update the attachment post.

Before doing so, our first approach was to check if an attachment exists, if exists, delete the attachment + its files and then simply create a new attachment. This first approach works but we're having huge ID number into the DB after the import as it generates thousand of new records which we didn't want. So now, only the files are updated, if an attachment post exists, we update its metadata and everything works as expected.

Now the issue/remark we have is that during the update of the attachment post, each time we import the new image file, we set the guid property to change and use the new imported file path. But when we check the database table, the guid is not updated at all and reflects the path to the image file imported the first time. Looking at source code, during an update, the guid is not modified in general as for RSS feeds we need that unique guid.

Our question is, is it relevant in this scenario to keep old guid property that points to a file that no longer exists? and so should we allow the guid to be updated by default in core for the attachment?

If RSS readers need that guid, why attachments use the file path and not an attachment page permalink ? Is there another location that uses this guid property in place of the meta data for the file path ?

Change History (4)

#1 follow-up: @MattyRob
3 years ago

  • Keywords close added

@jlambe

Thanks for posting this issue, I'm not entirely sure it's a bug though as the GUID is not expected to change by the WordPress core code.

That said - it is possible to force the issue if desired. You said above you set it to change - how have you done this? Something similat to the code snippet below works for me in a different situation where I change GUIDs. You'll need to create your new GUID and insert it into the database table at the correct point.

Obviously messing with the database directly can cause issues so be careful when testing this and your own code.

if ( 'inherit' === $post->post_status ) {
	$guid = get_option( 'siteurl' ) . '/?p=' . $new_id;
	// update the GUID
	$wpdb->update( $wpdb->posts,
		array(
			'guid' => $guid,
		),
		array(
			'ID' => $post_id,
		)
	);
}

#2 in reply to: ↑ 1 @jlambe
3 years ago

@MattyRob It's not a bug but can't choose another term for this issue discussion :)

Currently our code do not update the guid. We use the wp_insert_attachment() function to create or update the attachment post and set the guid property for its $args on each import like so:

$args = array_merge($args, [
    'post_title' => $file['basename'],
    'post_content' => '',
    'post_status' => 'inherit',
    'post_mime_type' => $image['type'],
    'guid' => $image['url'],
]);

/*
 * Insert/update attachment to posts table.
 */
$id = wp_insert_attachment($args, $image['file'], $parent_id);

When the core function is called, the guid property passed on update is different from the one already stored. The issue is that the core function do not update the guid because of this RSS feed unique identifier thing but in our case it is non sense even if it doesn't break things (at least for now) as it do not reflects "reality" as the guid value points to a file that no longer exists.

So the discussion here is, should we update the core function to also handle the guid for attachments (on update) if the original file no longer exists? or perhaps provide an option to authorize guid update whatever post type is (but this is dangerous as the developer should provide an expected guid to avoid to break things) ?

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

#3 @MattyRob
3 years ago

  • Keywords dev-feedback added; close removed
  • Summary changed from Attachment update and GUID to Should wp_insert_attachment() update GUID
  • Type changed from defect (bug) to enhancement

@jlambe

Having looked at the core code wp_insert_attachment() calls wp_insert_post(). This latter function is specifically written to create a GUID but use any existing GUID in preference so it's not going to be a simple patch.

Perhaps we need a core developer to give an opinion.

My personal opinion is that this is straying into a risky area and that GUIDs should not really be changed, the code I use simply moves them about in the database.

#4 @jlambe
3 years ago

@MattyRob Yes definitely need a core developer opinion on this. I'm aware it's not a simple patch and a core developer might bring light on the impact of updating an attachment guid.

Note: See TracTickets for help on using tickets.