carlomanf / wp-super-network

Share content between wordpress sites and create offspring networks.
https://wordpress.org/plugins/wp-super-network/
3 stars 0 forks source link

Writing detached data fails when using `update_metadata_by_mid`/`delete_metadata_by_mid` #10

Closed carlomanf closed 1 year ago

carlomanf commented 2 years ago

There exist functions update_metadata_by_mid and delete_metadata_by_mid that appear to be mostly used by core for ajax actions. For example, updating and deleting post meta through the Custom Fields meta box.

Given that these functions use the meta ID rather than the post ID, so they can not be transformed to the correct site. This means they will either fail, or worse, update/delete an irrelevant piece of metadata on the local site that happens to have the same meta ID.

One potential solution is to disallow collisions of the meta ID, in the same way as the post ID, although given the apparent limited use of these functions by core, it may be more sensible to simply disable their use in v1.2.0 in a similar way that v1.1.0 disables editing the post totally.

carlomanf commented 2 years ago

This issue is a bigger one than originally thought, because it turns out that delete_metadata also uses the meta ID internally, despite taking a post ID in its parameters. The same is not the case for update_metadata, thankfully.

Manually eliminating collisions of the meta ID is something I am reluctant to do, because of the high volume of meta values and also because not all meta values are intended for users to deal with directly.

Because delete_metadata does still have a record of the post ID, it might still be possible to correctly transform the query in these cases.

Another idea is to eliminate collisions by automatically re-assigning meta ID's. This never seemed like a good option for post ID collisions, since the post ID is often used as a reference in many places, but the same is not true for meta ID's.

carlomanf commented 1 year ago

ea64c97 attempts a fix for the issue, so it can be closed for now and re-opened in the case that any unwanted effects are found.

delete_metadata_by_mid and update_metadata_by_mid are being disabled altogether, so while they unfortunately can not currently be used to update or delete data, they will no longer update or delete the wrong data mistakenly.

The fix for delete_metadata involves noting down the set of meta ID's as a kind of "nonce" along with the associated entity ID, and when a query is found that deletes the same set of meta ID's, it is assumed to be the same entity. Any query at all getting in ahead of the expected one is highly unlikely, let alone one that deletes the exact same set of meta ID's from another site.

If the selected approach is found to be problematic, there is the option of trying one of the other approaches suggested in the previous comment.

If not, the approach can be extended in a follow-up patch. It should be noted that the process of deleting comments, posts and terms also uses delete_metadata_by_mid and therefore will leave behind orphan metadata as a result of the patch here. I consider this to be a lower priority because it does not have any significant effect from the user's perspective, but it should be possible to fix it using the same approach that was used for delete_metadata. This can be reported and tracked in a new issue.