Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Post meta is never actually deleted on the receiving end #284

Open
helen opened this issue Dec 5, 2018 · 3 comments · May be fixed by #419
Open

Post meta is never actually deleted on the receiving end #284

helen opened this issue Dec 5, 2018 · 3 comments · May be fixed by #419
Assignees
Labels
needs:discussion This requires discussion to determine next steps. type:bug Something isn't working.
Milestone

Comments

@helen
Copy link
Contributor

helen commented Dec 5, 2018

Noticed in #258 / #259:

set_meta() never actually deletes any post meta. This appears to always have been an issue but was exacerbated by a bug we introduced in 1.3.4 that was duplicating empty meta values that are now kind of stuck in place because of the way update_post_meta() works with previous values.

I don't yet know what the right thing to do is. We could:

  • Be less performant and just wipe out everything not blacklisted before adding again
  • Try to do some trickery with slicing the existing meta arrays down and matching up against the length of the new data and deleting the ones that are no longer represented. The issue with this is that for these situations where we have duplicated values (I don't know if there's ever a time where that's a valid thing people want to do??) it will wipe out all of the post meta with that same value regardless of how many items there are supposed to be. They will get added back, presumably, but that still seems like a weird routine.
  • Something else I haven't managed to think of yet

I guess the biggest question to start is: what kind of situations would there be where the distributed post has meta that the origin post did not have and need to keep?

@tlovett1

@helen helen added type:bug Something isn't working. meta labels Dec 5, 2018
@helen helen added this to the 1.4 milestone Dec 5, 2018
@helen helen mentioned this issue Mar 6, 2019
@avag-novembit avag-novembit linked a pull request Jun 28, 2019 that will close this issue
@avag-novembit
Copy link
Contributor

avag-novembit commented Jun 28, 2019

Created a PR (#419) where implemented the 'delete meta' functionality. Doing checks during post re-distribute (in the SubscriptionsController::receive_item(..) method) by comparing stored previous request meta data (stored in dt_subscription_update post meta key) versus new one and deleting post meta which existed in previous request but don't exist in the current one.

@arsendovlatyan
Copy link
Contributor

We have discussed this issue with @avag-novembit and I would like to add my thoughts, hopefully it will give more info about our approach and explain PR sent by @avag-novembit.

The basis is following - It's a must and it's safe to delete part of the post (in this case post meta), if it has been added/modified by Distributor. In other words, if will delete part of the post, that has been generated by Distributor, it's OK and expected, but if we will delete non-distributed data, somehow generated in directly destination - it's not OK and unexpected.

For example, we may have a plugin in source and destination, and that plugin can add/update a meta on certain events, it may also delete this meta from all posts on deactivation, now, when we will deactivate this plugin in source and then update a post, Distributor will delete this meta in destination as well and this can generate unpredictable issues, but this issues are expected for destination site maintainer, the same way we could have issues when we were adding meta data from source.

Deletion will be unexpected for maintainer if Distributor was never distributed that data, but all of sudden will decide to delete it. That's why we want to make sure that the most recent update from source brought this data, that we want to delete now.

I'm not sure if we need to compare meta values too, in case of duplicated meta keys it can be really important, but from the other side, we have many filters allowing third party code to extend and modify meta values before they will be inserted into DB (for example our ACF addon will map the meta value, if it contains an attachment ID), however, in my point of view, this data is still added by Distributor and has to be deleted, so, comparison with meta values needs to consider this modifications as well.

@jeffpaul
Copy link
Member

I'm moving this to the Future Release milestone to be considered alongside a broad approach to how we handle meta removal/deletion from remote sites. Apologies for the delay on response to this one @arsendovlatyan, but I want to be cautious with how we handle meta deletion.

@jeffpaul jeffpaul modified the milestones: 3.0.0, Future Release Mar 26, 2020
@jeffpaul jeffpaul added the needs:discussion This requires discussion to determine next steps. label Mar 26, 2020
@vikrampm1 vikrampm1 removed the meta label Dec 17, 2021
@jeffpaul jeffpaul modified the milestones: Future Release, 2.1.0 May 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:discussion This requires discussion to determine next steps. type:bug Something isn't working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants