Post IDs remain in metadata even after trashing

[gyrus]

I think this a major bug. The plugin is great otherwise, and I used it a lot - I surprised I've not spotted this before. Let me know if you're not able to fix it immediately, I'm going to have to find time to fix it myself.

I've replicated this on a vanilla install with only CMB2 and this plugin.

Basically, if you have a multi-value field like pw_multiselect and populate it with posts, when you trash one of the selected posts, it remains in the meta field in the database. This isn't so bad, but when I go to edit the post again, obviously the trashed post isn't shown in the select. But when I now save, the trashed post's ID still remains in the data. It's doesn't even make a difference if the post is permanently deleted - it persists, invisibly in terms of the admin interface, but obviously this creates all sorts of issues on the front.

What's worse, I've seen this actually affect non-pw_multiselect fields, though behaviour seems to be harder to replicate.

I'm not that familiar with CMB2's code but I guess cmb-field-multiselect2 is doing something on the save, where it should be clearing old values out. If I've not had a response to this before next week, I'll be diving in and trying to fix it as a site it's affecting is about to launch.

[mustardBees]

Hi @gyrus. Great issue! Thanks for raising this. It is something that I’ve thought about before. I use this field a lot for associating posts with other posts (for example, saving a list of related services on a sector page). What happens when a service is deleted?

This is a generic field that accepts any key-value data. In your instance, I’m guessing you’re using a generated list post IDs and post titles.

Say for example you’re getting your IDs from an API and the API returns an empty response, it’s not up to my field at that point to clear all the previously selected data. Maybe the API is temporarily down. Maybe the saved values are still valid in my system.

I believe this should be handled gracefully on the front end.

Example front end query:

$related_service_ids = get_post_meta( get_the_ID(), '_cmb_related_service_ids', true );

$args = array(
    'posts_per_page' => -1,
    'post_type'      => 'service',
    'post__in'       => $related_service_ids,
    'orderby'        => 'post__in'
);
$related_services = new WP_Query( $args );

if ( $related_services->have_posts() ) {
    while ( $related_services->have_posts() ) {
        $related_services->the_post();

        // the_title();
    }
}

wp_reset_postdata();

It’s difficult and I’m open to discussion. However, I’m not sure it’s the fields responsibility. Thoughts?

[gyrus]

I wrote my own custom fields plugin (although I'm generally moving towards CMB2 now), and I appreciate the issue of things being deleted that may have been included in metadata is tricky. There's no real way to know how a developer is using each custom field, in order to do some kind of automated check for now-redundant object IDs.

However, I'm not sure this issue is about that. As far as I can tell, your plugin is working differently to CMB2. Try this on a vanilla install with just CMB2:

1. Create a CMB2 multi checkbox field, populated with posts
2. Select a few posts, update
3. Check the postmeta table, you'll see the serialised array with the posts you selected
4. Trash one of the posts
5. Reload the page with the custom field. In the admin interface, you'll see the trashed post has disappeared. However, of course, you'll still be able to see its ID in the postmeta. (This is where it's clear that the issue isn't about automatically checking for trashed post IDs in postmeta.)
6. Now update the page with the custom field, and check in postmeta. The trashed post ID will have gone from the postmeta, because when the page is saved without the trashed post being output in the interface, only the values that are present in the interface are saved.

I'm not sure why your plugin's countering this behaviour, which seems like the expected behaviour, and in any case is the default behaviour of CMB2.

[mustardBees]

I see where you are coming from and the inconsistency with other fields is not great!

I believe this affects just the multiselect field. It's going to be related to how the data is loaded in. The values are loaded in this way to allow for drag and drop reordering.

Perhaps the field should hook in on save and check all the values exist in the options array? If this something you can help with, I'd happily accept a pull request. I'm unlikely to find the time over the next few weeks and I'm aware this is urgent to your project.

[gyrus]

I think I've come up with a fix:

https://github.com/gyrus/cmb-field-select2/commit/adec06e46bfd8801675dbdbb2db32d8a1463655a

When the field is rendered, if there's an ID in the hidden input value that isn't present in the JS data listing all the posts, it's removed. Then when the post is saved, that ID of a trashed post won't be saved back into the postmeta.

The only issue I can imagine is if for some reason someone uses / expects the current behaviour, where trashed post IDs are invisibly persisted. I can't imagine a scenario, but if someone has come up with one, this fix could break that aspect of their site. If you think this is a concern, maybe there should be a setting - a constant? - which can be used to trigger this new behaviour.

However, I consider this a bug, and would be fine with the fix just being added as-is.

[mustardBees]

That's an elegant solution @gyrus. I agree this could be a breaking change for someone. I think a bump in major version number and a clear message in the release notes should suffice.