Skip to content

Conversation

@roccotripaldi
Copy link
Contributor

Fixes an issue where we cannot validate a site whose terms are out of sync

Changes proposed in this Pull Request:

  • Ensure sync histogram endpoint can calculate a checksum for term relationships

Is this a new feature or does it add/remove features to an existing part of Jetpack?

  • Ip7rcWF-12N-p2

Testing instructions:

  • requires an upstream patch D30444-code to test

Proposed changelog entry for your changes:

  • None

@roccotripaldi roccotripaldi added the [Status] Needs Review This PR is ready for review. label Jul 15, 2019
@roccotripaldi roccotripaldi added this to the 7.6 milestone Jul 15, 2019
@roccotripaldi roccotripaldi requested review from a team July 15, 2019 19:00
@jetpackbot
Copy link
Collaborator

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 2a60440

case 'term_relationships':
$object_table = $wpdb->term_relationships;
$object_count = $this->term_relationship_count();
$id_field = 'object_id';
Copy link
Member

Choose a reason for hiding this comment

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

The object_id column isn't really a unique ID, so we can't use it for identifying a particular row. It's as much of a ID field as the term taxonomy ID. Should we be concerned about this? Should we be able to calculate histogram for tables that lack a unique ID field?

I'm basically concerned for cases when we have 1000 term relationships for 1 post, and then 1 term relationship per post after that. It just feels like it's not a reliable way to limit terms.

What do you think? Should we use some sort of calculation of both IDs?

@roccotripaldi roccotripaldi added [Status] In Progress and removed [Status] Needs Review This PR is ready for review. labels Jul 16, 2019
@roccotripaldi
Copy link
Contributor Author

Will revisit this with a different approach now that #13077 is open

@roccotripaldi roccotripaldi deleted the update/sync-histogram-term-relationships branch July 19, 2019 15:10
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.

5 participants