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

Update revision meta storage format for consistency #56

Merged
merged 12 commits into from
Mar 13, 2020

Conversation

aaemnnosttv
Copy link
Contributor

This PR fixes the way post meta is saved on the revision. Currently post meta values are saved as an array of values under a single meta key, instead of multiple meta values (rows). This change updates the way meta is stored on the revision to mirror the meta on the post it is created from.

As is, this is a breaking change as existing meta values are all saved under a single key, so restoring to a revision that uses meta saved in the old format would restore the array of values as a single value rather than adding a value for each item in the array. I'm not sure there is a good way around this, without adding some additional level of versioning to the revisioned meta since an array is a valid single meta value. Alternatively, there could be a database upgrade routine that would update from the old single array value to the proposed format which is consistent with how it is stored on the source post.

This PR also introduces a new method to reduce the duplication for copying meta from one post to another which is used when revisioning and restoring.

Resolves #13

@adamsilverstein
Copy link
Owner

@aaemnnosttv - Very nice, this does seem like a huge improvement, I do wish the original code had used a similar approach. This does seem like a breaking change so we would probably want to include this in version 1.1 of the plugin.

Regarding upgrading data, we could probably autoupgrade existing installs when users open the post for the first time. However, I worry a bit about users who stored arrays, or multiple values on the same key, or potentially users who added their own meta storage. We might want to make a CLI command for the upgrade for this reason, so users can more explicitly choose to change their data.

As part of this change we should also improve test coverage for storing and retrieving meta, and we can add tests for upgrading from each type of storage as well to validate the transition. There is a good outline of possible types here: #13 (comment)

* to ensure metadata is added to the revision post and not its parent.
*/
add_metadata( 'post', $revision_id, $meta_key, wp_slash( $meta_value ) );
$this->copy_post_meta( $post_id, $revision_id, $meta_key );
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent!

@adamsilverstein
Copy link
Owner

This looks great, a few things remain here:

  • Add test coverage that validates the new storage approach.
  • document this change.
  • consider and test upgrade path requirements.

plus resolving merge conflicts :)

Copy link
Owner

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks for your help here @aaemnnosttv!

@aaemnnosttv
Copy link
Contributor Author

Great to see this going in! Would be great to add #55 for 2.0 as well (although it could be safely added to 1.x too) as that would be a very small change on top of this 😄

Thanks for putting the finishing touches on here!

@adamsilverstein adamsilverstein merged commit ca45992 into adamsilverstein:master Mar 13, 2020
@aaemnnosttv aaemnnosttv deleted the fix/meta-format branch March 14, 2020 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Why are post metas are stored as an array instead of sperarate keys
2 participants