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

Sync empty taxonomies to distributed posts. #951

Merged
merged 9 commits into from
Oct 20, 2022

Conversation

peterwilsoncc
Copy link
Collaborator

@peterwilsoncc peterwilsoncc commented Sep 30, 2022

Description of the Change

This modifies prepare_taxonomy_terms() to include both populated and unpopulated taxonomies when distributing sites.

This allows for the situation in which an original post is updated to remove all terms within a shared taxonomy.

To account for empty arrays being stripped from $_POST data (I've not been able to determine where this happens), update notifications are now sent as JSON encoded data to the remote sites REST API. This may be a backward compatibility issue for sites using hooks and getting POSTed data rather than using the $request object.

Taxonomy updates are limited to those shown in the REST API.

Closes #625

How to test the Change

  1. Check out the develop branch.
  2. Create a post and assign it two or more tags.
  3. Distribute the post
  4. Observe distributed version of post has two tags
  5. Delete one tag on original post, update it.
  6. Observe distributed version of post has one tag.
  7. Delete all tags on original post, update it.
  8. Observe distributed version of post has one tag (error).
  9. Check out this branch and repeat
  10. After deleting tags on the original post, the distributed version should have no tags.

Notes

  1. E2E tests are failing for trunk due to a bug in @wordpress/env -- it was fixed five days ago but is still to be released WordPress/gutenberg@3983fee
  2. I've had to change some of the tests to work around an issue in wp_mocks. I've created an issue and linked to that within the comments.

Changelog Entry

Fixed - Distribute empty taxonomies to external sites.

Credits

Props @peterwilsoncc, @manolobevia, @jeffpaul, @cadic

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

This allows updates which include an empting of a taxonomy of all it’s terms. Sending a taxonomy with an empty array as standard post data gets removed from the REST request.
@peterwilsoncc peterwilsoncc self-assigned this Sep 30, 2022
@peterwilsoncc peterwilsoncc added this to the 2.0.0 milestone Sep 30, 2022
@peterwilsoncc
Copy link
Collaborator Author

peterwilsoncc commented Sep 30, 2022

The tests here are failing because the following returns null rather than an empty array. I've tried figuring out why but need some help from someone else.

\WP_Mock::userFunction(
	'\Distributor\Utils\prepare_taxonomy_terms', [
		'return' => [],
	]
);

I did a var_dump on the following line within test_send_notifications_remote_post_exists()

/vagrant/content/plugins/distributor/tests/php/SubscriptionsTest.php:514:NULL

@peterwilsoncc peterwilsoncc marked this pull request as ready for review October 17, 2022 03:15
@peterwilsoncc peterwilsoncc requested review from a team and cadic and removed request for a team October 17, 2022 03:20
Copy link
Contributor

@cadic cadic left a comment

Choose a reason for hiding this comment

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

The code looks good.

BTW, I was not able to replicate the original issue (at develop branch: after second-the-last tag was deleted in the original post, the distributed post has no tags as well on the step 8).

@peterwilsoncc
Copy link
Collaborator Author

@cadic Do you recall if you were testing on an internal (multisite) connection or an external connection?

@cadic
Copy link
Contributor

cadic commented Oct 19, 2022

@peterwilsoncc oh, thanks for clarifying that, it does reproduce with the external connection.

@peterwilsoncc peterwilsoncc merged commit 5f345c2 into develop Oct 20, 2022
@peterwilsoncc peterwilsoncc deleted the fix/625-empty-taxonomy-updates branch October 20, 2022 22:48
@peterwilsoncc
Copy link
Collaborator Author

Thanks Max, sorry I missed that step in the reproduction notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Taxonomy terms don’t sync among sites.
2 participants