Fix featured image delete push issue

[ravichdev]

If we delete the featured image for a post, it does not delete for the distributed post. This happens only if the featured image is selected from Media Library, works fine if the featured image is uploaded to the actual post.

Steps to reproduce:

• Create a new post and select a feature image from the Media library, make sure to use an existing image
• Distribute the post
• The distributed post will show the featured image
• Remove the featured image in the original/source post and update
• The distributed post will still show the featured image

This PR contains an else statement to delete featured image if distributor_media is empty. There are still other places (NetworkSiteConnection.php) which would need a similar condition, I am happy to add those as well if this approach looks good to you.

[helen]

Meta deletion is broadly a problem, see #284. Happy to keep this PR open and milestone it if media should be handled separately though, I haven't dug into the code to find out.

[dkotter]

So we do handle media and meta separately at the moment (as far as processing and setting), so I think we will probably handle the deletion separately as well. That said, in my mind it makes sense to deal with deletion as one larger task, so we keep in mind all scenarios when implementing this.

I'm not opposed to the approach in this PR but a couple things come to my mind:

1. Some posts might have media set but not have an actual featured image set. If that post then removes those media items, this approach would delete the featured image meta, even though that meta isn't actually set. Not a huge deal but I like the idea of only deleting things that should be deleted.
2. This approach only deals with deleting the featured image meta. What about other media meta that might get changed or deleted? Or what about the media items themselves? If the original post removes those, should we delete those entirely from the receiving site?

[adamsilverstein]

What about other media meta that might get changed or deleted? Or what about the media items themselves? If the original post removes those, should we delete those entirely from the receiving site?

This feels dangerous: what if the images themselves had been used in another post on the destination site?

[helen]

Doesn't seem like we're quite at a consensus about approach/result yet, punting pending further dedicated discussion.

[jeffpaul]

First, let's separate the terms removal and deletion as they seem to be used interchangably in this PR when they should be considered separate cases. Removal, to me, is that the image is unattached from the post but still available in the Media Library. Deletion, to me, is that the image is deleted/trashed from the site and no longer available on the post or in the Media Library.

My feeling on this is that we could limit this PR to the following use case:
An origin post removes the feature image and that removal is handled on distributed remote posts as well, however it merely removes the featured image from those posts, the image is still available in the remote Medial Libraries, and does not delete the image from those remote sites.

For all other cases, we can cover them via a separate PR in a future release that more fully handles meta and other media removals/deletions.

[arsendovlatyan]

Side note: I think it will be good to blacklist _thumbnail_id meta, the value that we are pushing with this meta never works as expected, it's the attachment ID in source, but in destination, it's something else and we are updating this value during media handling process.

For deletion we developed our solution, which is based on dt_original_media_id . Every time, before applying updates on a post in destination, we retrieve all original media IDs of all attachments, that are connected with the post being updated, then we look up this IDs in new update, if an ID is not found, we delete that attachment. For example, we a have post in source website, which has attachments with following IDs (1, 2, 3), when you distribute this post, a new post in destination will have this attachments with different IDs, like ( 5, 6, 7), but their original media ID is saved in dt_original_media_id , if at some point destination receives an update, where attachment IDs are (1, 3), it's means 2 has been deleted in the source, so, we can delete 6 in destination.

I'm not sure if this approach is acceptable or not, but you can find our solution here https://github.com/NovemBit/distributor/blob/develop/includes/utils.php ( I know that it needs improvements before PR)

I think this behavior is safe as long as dt_original_media_id exists only if media was created by Distributor, so, you won't delete attachments created and attached in destination.

[jeffpaul]

As this PR still needs discussion and we're working to clear the milestone to get nearer a release, I'm going to punt this to the next release in hopes we can get through our discussion and to an agreed approach by then.

[peterwilsoncc]

This PR looks good to me and worked while testing with:

• internal push
• internal pull
• external push
• external pull

It looks like the set_media() utility function was intended to handle this situation but the empty() check means the relevant code doesn't run:

distributor/includes/utils.php

Lines 790 to 792 in f769e29

	if ( ! $found_featured_image ) {
	delete_post_meta( $post_id, '_thumbnail_id' );
	}

I've put in a couple of coding style nitpicks but otherwise consider this PR as approved. ✅

This PR is on the 2.1.0 milestone so I'm going to hold of clicking the approve button for now so I don't accidentally merge it prior to the release of 2.0.0 in the coming weeks.

[peterwilsoncc]

Hitting the approve button and merging now that 2.0.0 is released.

This has been added to the milestone for 2.0.1 so should be in the next release.