refactor: OOP for distributed and linked posts #160

miguelpeixe · 2024-12-09T16:03:40Z

Refactors the Content Distribution class to use instances of the new Outgoing_Post and Incoming_Post classes.

The Outgoing_Post is initialized with a post object or ID:

$post_id = 1; // ID for a distributed post.
$outgoing_post = new Newspack_Network\Content_Distribution\Outgoing_Post( $post_id );
$config = $outgoing_post->get_config();
// = [
//     "site_urls" => [
//       "http://node1.local",
//     ],
//     "network_post_id" => "0ac636e2f9dfc3ddd9d1934bfa97b252",
//  ]

$post_id = 2; // ID for a non-distributed post.
$outgoing_post = new Newspack_Network\Content_Distribution\Outgoing_Post( $post_id );
$config = $outgoing_post->get_config();
// = [
//     "site_urls" => [],
//     "network_post_id" => "",
//  ]

The Incoming_Post is initialized with a post payload ($outgoing_post->get_payload()) and queries the existing linked post on instantiation:

$incoming_post = new Newspack_Network\Content_Distribution\Incoming_Post( $payload );

// Insert the post with the object payload.
$post_id = $incoming_post->insert();

The methods and unit tests had most of their logic preserved with some adjustments to fit the new approach. The Content_Distribution class is still responsible for implementing the hooks that make the distribution possible, connecting the new objects.

This PR also implements the support for syncing taxonomies (merged from #158). Testing this new feature is a good way to ensure that this refactor didn't introduce any regression to the core elements of the project.

Testing

Make sure you have a network setup with at least 1 hub and 1 node
On the hub, create a new post with a featured image, categories, and tags
Grab the post ID, and use the WP shell to distribute to the node:

$post_id = 0; // Change to your post ID.
$outgoing_post = new Newspack_Network\Content_Distribution\Outgoing_Post( $post_id );
$outgoing_post->set_config( [ 'http://node1.local' ] ); // Change to match your node URL.
Newspack_Network\Content_Distribution::distribute_post( $outgoing_post );

Once the events are handled, visit the node's drafts and confirm the post is listed with the correct taxonomy and featured image
Back in the hub, change the categories and terms and save
In the node, confirm the taxonomy changes were applied

…on-modified-date

…on-featured-image

naxoc

This looks good! Much better separation.

I think I'm still learning, so I added some questions that might be silly about the Linked post. I don't understand what a linked post that doesn't have a post is or should do.

naxoc · 2024-12-10T13:49:06Z

includes/content-distribution/class-distributed-post.php

+	 *
+	 * @var int
+	 */
+	public $ID = 0;


Is this necessary? we have $this->post->ID

It's a public var, whereas $post is protected. To avoid the duplication I changed the strategy to deprecate this var and add a public get_post() in f755a01. WDYT?

I like it! 👍

naxoc · 2024-12-10T13:51:05Z

includes/content-distribution/class-distributed-post.php

+		if ( empty( $config['network_post_id'] ) ) {
+			$config['network_post_id'] = md5( $this->ID . get_bloginfo( 'url' ) );
+		}
+		$config['enabled']   = empty( $site_urls ) ? false : true;


I struggle understandingenabled a bit. Why would we have a disabled distributed post? Is this the same as linked?

It's mostly a proposal to be able to deactivate the distribution of a given post without removing the configured site list. I can see this being a toggle in the post distribution configuration, but that's not really scoped.

So it means "distribution is enabled"?

to be able to deactivate the distribution of a given post without removing the configured site list.

But here it is based on not having sites in the list.

Would it be easier to understand if it was a count of the site urls? (and a different name)?

Currently it's an automated way to set enabled given the site list.

There can be a deactivate_distribution() method that just toggles that prop. There's no need for it right now. I can also just remove the prop entirely and we implement once there's a case for it.

Updated in 22c6e8f

naxoc · 2024-12-10T13:51:49Z

includes/content-distribution/class-distributed-post.php

+	 * Whether the post is distributed. Optionally provide a $site_url to check if
+	 * the post is distributed to that site.
+	 *
+	 * @param int|null $site_url Optional site ID.


Should that not be string? And url? Not ID?

Thanks for catching that! Fixed in 2bb94b6

naxoc · 2024-12-10T13:52:30Z

includes/content-distribution/class-distributed-post.php

+	 * @return bool
+	 */
+	public function is_distributed( $site_url = null ) {
+		if ( ! $this->ID ) {


This would never happen I think? It's set in the constructor. This goes for all the checks.

You're right! Removed in f755a01

Missed one: 8bafa68

naxoc · 2024-12-10T13:56:23Z

includes/content-distribution/class-linked-post.php

+		}
+
+		$config    = $payload['config'];
+		$post_data = $payload['post_data'];


This var is unused

Whoops! Inherited from its previous home. Fixed in 086ec9c

naxoc · 2024-12-10T13:57:31Z

includes/content-distribution/class-linked-post.php

+		$this->network_post_id = $payload['config']['network_post_id'];
+
+		$post = $this->query_linked_post();
+		if ( $post ) {


Would it be better to throw an error here or is there a use case for having no post?

The Linked_Post can - and will - be instantiated without the post existing. It will exist once ->insert() is called.

…ublic method

naxoc · 2024-12-10T14:17:22Z

I am getting an error when clicking "Add New Post" in wp-admin/edit.php

PHP Fatal error:  Uncaught Error: Call to a member function is_distributed() on null in /Users/ckj/Dev/_code/re│Stack trace:
│#0 [internal function]: Newspack_Network\Content_Distribution::handle_post_updated(NULL, Object(WP_Post), false, NULL)
│#1 /Users/ckj/Dev/_code/repos/newspack-plugin/includes/data-events/class-data-events.php(192): call_user_func_array(Array, Array)
│#2 /Users/ckj/Dev/sites/hub/wp-includes/class-wp-hook.php(324): Newspack\Data_Events::Newspack\{closure}(140, Object(WP_Post), false, NULL)
│#3 /Users/ckj/Dev/sites/hub/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters(NULL, Array)
│#4 /Users/ckj/Dev/sites/hub/wp-includes/plugin.php(517): WP_Hook->do_action(Array)
│#5 /Users/ckj/Dev/sites/hub/wp-includes/post.php(5781): do_action('wp_after_insert...', 140, Object(WP_Post), false, NULL)
│#6 /Users/ckj/Dev/sites/hub/wp-admin/includes/post.php(772): wp_after_insert_post(Object(WP_Post), false, NULL)
│#7 /Users/ckj/Dev/sites/hub/wp-admin/post-new.php(66): get_default_post_to_edit('post', true)
│#8 /Users/ckj/.composer/vendor/laravel/valet/server.php(110): require('/Users/ckj/Dev/...')
│#9 {main}
│  thrown in /Users/ckj/Dev/_code/repos/newspack-network/includes/class-content-distribution.php on line 69

miguelpeixe · 2024-12-10T14:28:47Z

I am getting an error when clicking "Add New Post" in wp-admin/edit.php

Caused by a last-minute change in the Content_Distribution::handle_post_updated() method 🤦

Fixed in dddda16

miguelpeixe · 2024-12-10T15:40:43Z

As discussed in p1733836852714589-slack-C055SRT7WN4, the classes have been renamed to Outgoing_Post and Incoming_Post in ef68219

naxoc

I think this looks good now! Thank you for your patience with all my questions, Miguel 🙌

miguelpeixe · 2024-12-10T15:56:34Z

Thank you for the thorough review and great questions 🙌

* feat: content distribution class * feat: insert linked post * refactor: use site url instead of ID * chore: lint * feat: return errors on post insert * chore: lint * feat: support webhooks request priority * feat: unlink functionality and unit tests * chore: lint * feat: introduce 'linked_post_inserted' hook and listener * chore: lint * fix: listener hook name * fix: typo * chore: better docblocks * chore: remove redundant code * test: persist post hash * test: remove unnecessary assertion * refactor: distribute_post() to use handle_post_updated() method * refactor: post hash is now network post id * chore: update comment * chore: Add CLI command for distribute * feat: content distribution class (#155) * feat: content distribution class * feat: insert linked post * refactor: use site url instead of ID * chore: lint * feat: return errors on post insert * chore: lint * feat: support webhooks request priority * feat: unlink functionality and unit tests * chore: lint * feat: introduce 'linked_post_inserted' hook and listener * chore: lint * fix: listener hook name * fix: typo * chore: better docblocks * chore: remove redundant code * test: persist post hash * test: remove unnecessary assertion * refactor: distribute_post() to use handle_post_updated() method * refactor: post hash is now network post id * chore: update comment * feat(content-distribution): prevent older content updating linked posts (#156) * feat(content-distribution): handle post thumbnail (#157) * Add network util class * Almost done except for a few todos * refactor: OOP for distributed and linked posts (#160) * chore: Require php 8.1 * chore: Update to use util class Also use new incoming/outgoing classes * fix: Include correct classname The old classname "Linked_Post" was still used instead of "Incoming_Post" causing sync to fail. ## How to test Probably eyeballing is enough * chore: Add validation of outgoing post Also move inclusion of the content distribution CLI class to better respect the flag we are introdocuing * chore: phpcs * fix(content-distribution): debug post update and remove deprecated config (#165) * fix: Don't return payload on all posts * fix: post handling * chore: move try-catch * chore: Move check for networked urls * Move more * Move things again * chore: Add a mock networked node * chore: add one more node to the mock nodes * Add a test --------- Co-authored-by: Miguel Peixe <miguel.peixe@automattic.com>

# [2.4.0-alpha.1](v2.3.4...v2.4.0-alpha.1) (2025-01-10) ### Bug Fixes * **content-distribution:** post insertion hook and additional meta for incoming post event ([#173](#173)) ([48df13c](48df13c)) * load text domain on init hook ([#171](#171)) ([01fb89c](01fb89c)) ### Features * content distribution - experimental ([#168](#168)) ([dc837d8](dc837d8)) * **content-distribution:** add CLI command for distribute post ([#159](#159)) ([7a43b86](7a43b86)), closes [#155](#155) [#156](#156) [#157](#157) [#160](#160) [#165](#165) * **content-distribution:** canonical url ([#177](#177)) ([5ca60ce](5ca60ce)) * **content-distribution:** capability and admin page ([#176](#176)) ([5285285](5285285)) * **content-distribution:** control distribution meta and prevent multiple dispatches ([#170](#170)) ([e76a2dc](e76a2dc)) * **content-distribution:** editor plugin for distribution ([#167](#167)) ([e10aef4](e10aef4)) * **content-distribution:** handle status changes ([#166](#166)) ([4af5da1](4af5da1)) * **content-distribution:** log incoming post errors ([#182](#182)) ([74c9119](74c9119)) * **content-distribution:** posts column ([#178](#178)) ([8e07640](8e07640)) * **content-distribution:** reserved taxonomies ([#174](#174)) ([a2c54d2](a2c54d2)) * **content-distribution:** sync comment and ping statuses ([#179](#179)) ([90c5425](90c5425)) * **content-distribution:** sync post meta ([#163](#163)) ([353a3d8](353a3d8)) * **event-log:** collapse data ([#180](#180)) ([956219d](956219d)) * limit purchase of a network membership ([#169](#169)) ([deb2683](deb2683))

# [2.4.0](v2.3.4...v2.4.0) (2025-01-20) ### Bug Fixes * **content-distribution:** post insertion hook and additional meta for incoming post event ([#173](#173)) ([48df13c](48df13c)) * load text domain on init hook ([#171](#171)) ([01fb89c](01fb89c)) ### Features * content distribution - experimental ([#168](#168)) ([dc837d8](dc837d8)) * **content-distribution:** add CLI command for distribute post ([#159](#159)) ([7a43b86](7a43b86)), closes [#155](#155) [#156](#156) [#157](#157) [#160](#160) [#165](#165) * **content-distribution:** canonical url ([#177](#177)) ([5ca60ce](5ca60ce)) * **content-distribution:** capability and admin page ([#176](#176)) ([5285285](5285285)) * **content-distribution:** control distribution meta and prevent multiple dispatches ([#170](#170)) ([e76a2dc](e76a2dc)) * **content-distribution:** editor plugin for distribution ([#167](#167)) ([e10aef4](e10aef4)) * **content-distribution:** handle status changes ([#166](#166)) ([4af5da1](4af5da1)) * **content-distribution:** log incoming post errors ([#182](#182)) ([74c9119](74c9119)) * **content-distribution:** posts column ([#178](#178)) ([8e07640](8e07640)) * **content-distribution:** reserved taxonomies ([#174](#174)) ([a2c54d2](a2c54d2)) * **content-distribution:** sync comment and ping statuses ([#179](#179)) ([90c5425](90c5425)) * **content-distribution:** sync post meta ([#163](#163)) ([353a3d8](353a3d8)) * **event-log:** collapse data ([#180](#180)) ([956219d](956219d)) * limit purchase of a network membership ([#169](#169)) ([deb2683](deb2683))

miguelpeixe added 30 commits November 28, 2024 12:24

feat: content distribution class

9997fd5

feat: insert linked post

7a5153d

refactor: use site url instead of ID

4230692

chore: lint

0561d77

feat: return errors on post insert

e1aee3f

chore: lint

6d5e931

feat: support webhooks request priority

34dcbf3

feat: unlink functionality and unit tests

5e47bbe

chore: lint

ca10658

feat: introduce 'linked_post_inserted' hook and listener

136ed6a

chore: lint

7738de3

fix: listener hook name

6095800

fix: typo

699228a

chore: better docblocks

0bf9c37

chore: remove redundant code

20d5dc0

test: persist post hash

fb501d4

test: remove unnecessary assertion

548a571

refactor: distribute_post() to use handle_post_updated() method

b0c0a0d

refactor: post hash is now network post id

2d3f9ca

chore: update comment

78c7f01

feat(content-distribution): prevent older content updating linked posts

d7ab065

fix: return error

63cb21e

feat(content-distribution): handle post thumbnail

c05d5fa

refactor: use media_sideload_image function

4741f7a

test: thumbnail insertion, update and removal

90745b6

test: simplify tests

b04fcfa

feat(content-distribution): sync taxonomy on distribution

168e38b

test: assert taxonomy insertion

9f28cf4

Merge branch 'epic/content-distribution' into feat/content-distributi…

cabcf73

…on-modified-date

Merge branch 'epic/content-distribution' into feat/content-distributi…

cb072cf

…on-featured-image

feat: support Distributed_Post as method param

3ab76a5

miguelpeixe marked this pull request as ready for review December 10, 2024 13:18

miguelpeixe requested a review from a team as a code owner December 10, 2024 13:18

miguelpeixe self-assigned this Dec 10, 2024

miguelpeixe requested review from leogermani and naxoc December 10, 2024 13:18

miguelpeixe added the [Status] Needs Review label Dec 10, 2024

naxoc reviewed Dec 10, 2024

View reviewed changes

miguelpeixe added 3 commits December 10, 2024 11:07

chore: remove unused var

086ec9c

chore: fix docblock

2bb94b6

fix: remove unnecessary checks, deprecate ID var and add get_post() p…

f755a01

…ublic method

fix: handle_post_updated

dddda16

miguelpeixe requested a review from naxoc December 10, 2024 14:28

miguelpeixe added 3 commits December 10, 2024 11:30

fix: remove unnecessary check

8bafa68

fix: remove enabled prop from distributed post

22c6e8f

chore: rename Incoming and Outgoing post classes

ef68219

miguelpeixe added 3 commits December 10, 2024 12:41

fix: rename action

e3f16d8

chore: update docblock

eb0fa32

test: instantiation with post id

e0d99d2

naxoc approved these changes Dec 10, 2024

View reviewed changes

miguelpeixe merged commit 31d6744 into epic/content-distribution Dec 10, 2024

miguelpeixe deleted the refactor/content-distribution-oop branch December 10, 2024 15:57

miguelpeixe mentioned this pull request Dec 10, 2024

feat(content-distribution): sync post meta #163

Merged

refactor: OOP for distributed and linked posts #160

refactor: OOP for distributed and linked posts #160

Uh oh!

Conversation

miguelpeixe commented Dec 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Uh oh!

naxoc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naxoc commented Dec 10, 2024

Uh oh!

miguelpeixe commented Dec 10, 2024

Uh oh!

miguelpeixe commented Dec 10, 2024

Uh oh!

naxoc left a comment

Choose a reason for hiding this comment

Uh oh!

miguelpeixe commented Dec 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

miguelpeixe commented Dec 9, 2024 •

edited

Loading