-
Notifications
You must be signed in to change notification settings - Fork 798
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
Sync: consistent id field when syncing terms #14759
Conversation
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: March 3, 2020. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my understanding, this will only affect sites that somehow still have not yet split terms. Should be safe to use. I couldn't find any places where the id_field method is used elsewhere, so it should not break any other functionality.
In review, I also don't see any dependency outside of this class for the id_field in addition all internal functions for estimating counts are based on term_taxonomy so it makes sense that that be the primary table. I'm actively running Sync and Full Sync tests if they show no regression concerns then will merge as part of this release. |
Sync Actions -> new Terms, edited Terms, deleted terms and Term Relationships all sync as expected. Full Sync -> All Terms and Relationships are persisted. |
@jeherve - I've tested this in full and as it is a bug fix lets put it in 8.3. Confirmed that there are no regression of sync functionality for incremental or Full. |
Not quite. It will affect sites that existed before terms were split in WP. The terms have since been split, which is why the IDs are different now (and why WP core needed this term_tax_id in the first place afaik). Not that this changes the patch, just wanted to mention it in case of changelog notes or something. |
* 8.3 release: changelog * Changelog: add #14516 * Changelog: add #14574 * Bring in changes from 8.2.1 and 8.2.2 * Update stable version * Bring in 8.2.3 changes * Changelog: add #14714 * Changelog: add #14639 * Changelog: add #14678 * Changelog: add #14673 * Changelog: add #14687 * Changelog: add #14704 * Changelog: add #14702 * Changelog: add #14541 * Changelog: add #14657 * Changelog: add #14622 * Changelog: add #14582 * Changelog: add #14638 * Changelog: add #14633 * Changelog: add #14571 * Changelog: add #14592 * Changelog: add #14539 * Changelog: add #14514 * Changelog: add #14643 * Changelog: add #14494 * Changelog: add #13739 * Changelog: add #14707 * Changelog: add #14736 * Changelog: add #14706 * Changelog: add #14730 * Changelog: add #14685 * Changelog: add #14727 * Changelog: add #14711 * Changelog: add #14742 * Changelog: add #14746 * Changelog: add #14725 * Changelog: add #13999 * Changelog: add #14740 * Changelog: add #14759 * Changelog: add #14703 * Changelog: add #14753 * Changelog: add #14754 * Changelog: add #14645 * Cahngelog: add #14599
Fixes #14752
Changes proposed in this Pull Request:
"In short, the Sender is sending terms_ids but then later querying for the data as if it sent term_taxonomony_id.
On newer WP sites, this wouldn't be an issue because term_id === term_taxonomony_id usually I believe. This is prevalent on sites before the term split days though, as these two ids are not the same thing."
Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
pull
from the serverProposed changelog entry for your changes: