Reduce duplication pulling posts #1017

peterwilsoncc · 2023-02-20T03:15:05Z

Description of the Change

Closes #1016

How to test the Change

Changelog Entry

Added - Reduce duplication when pulling posts.

Credits

Props @peterwilsoncc

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.

peterwilsoncc · 2023-02-20T03:20:13Z

includes/classes/InternalConnections/NetworkSiteConnection.php

-				$formatted_post = Utils\prepare_post( $post );
+				$dt_post        = new DistributorPost( $post );
+				$formatted_post = $dt_post->to_insert( $new_post_args );


This causes a BC change on this line below.

distributor/includes/classes/InternalConnections/NetworkSiteConnection.php

Line 562 in aea7570

return apply_filters( 'dt_remote_get', $formatted_post, $args, $this );

The $formatted_post was passed as an object in the past but is now passed as an array.

However, the filter is conflicted anyways as demonstrated by this code, it can sometimes be an object, sometimes an array.

peterwilsoncc · 2023-03-15T05:00:07Z

includes/classes/Authentication.php

Todo: Make sure this doesn't affect the wp.com oauth flow.

peterwilsoncc · 2023-03-24T02:36:36Z

So it seems network pulls are broken.

For some reason, it appears the posts are going straight to the skipped column on network connections but I am unable to figure out why at the moment.

While testing, I suspect it is intermittent too.

The return type of NetworkSiteConnection::remote_get() has changed from a WP_Post to an array when getting a single post object, this is formatted by DistributorPost to be ready for inserting/updating the existing post entry

peterwilsoncc · 2023-03-29T02:26:49Z

tests/php/NetworkSiteConnectionsTest.php

-				'args'   => [ \WP_Mock\Functions::type( 'int' ), 'dt_original_post_id', 2 ],
+				'args'   => [ \WP_Mock\Functions::type( 'int' ), 'dt_original_post_id', 111 ],


I can not figure out why this was passing. The get_post() mock above is using 111 so that's what should have been passed.

…on of the plugin.

…ens.

peterwilsoncc · 2023-04-19T05:09:00Z

@dkotter @ravinderk

This is Big.

Changes:

External pulls always use the custom REST API endpoint, including for
- Pulling a single post (was using WP Core default endpoint)
- Populating the pull list screen prior to pulling any posts (was using WP Core default endpoint)
- Populating the pull list screen after pulling posts (unchanged)
- When pulling multiple posts, all posts are obtained in a single HTTP request
The custom REST API endpoint for pulling content has been refactored substantially
- An error is returned if the request is from Distributor V1
- No change is required to the pull screen to catch this, the code was already in place
- Accepts additional parameters
  - include
  - order
  - orderby
- Additional sanitization of values added to post type and status parameters
- Permission check modified to account for requesting multiple post types

Testing notes

These are the situations I cna think of:

Network pull
Network pull, multiple posts
External pull
External pull, multiple posts
Update previously distributed posts (network, external)
Unlinking posts to ensure they do not update
Canonical links work as expected (ie, show source site): with and without Yoast
Confirm meta data matches between old and new

Feel free to add anything to this list you think of for testing

WordPressExternalConnection::remote_get() is now a wrapper for the ::remote_post() method
Distributor HTTP requests now include X-Distributor-Version as a header
To avoid messing around with fixing date values, pulling now uses wp_update_post() for updates

includes/classes/InternalConnections/NetworkSiteConnection.php

includes/rest-api.php

includes/utils.php

peterwilsoncc · 2023-04-26T00:50:59Z

Thanks for the review @dkotter, I've addressed each of your suggestions.

ravinderk

@peterwilsoncc I left minor suggestions. Let me know if you have questions.

ravinderk · 2023-04-27T13:20:49Z

includes/bootstrap.php

@@ -54,6 +54,7 @@ function() {
 	'rest_post_dispatch',
 	function( $response ) {
 		$response->header( 'X-Distributor', 'yes' );
+		$response->header( 'X-Distributor-Version', DT_VERSION );


@peterwilsoncc I am not aware why we add distributor-related information to all API requests. Is it required? Can we limit it to distributor-specific rest API requests?

I am not aware why we add distributor-related information to all API requests.

Not sure of the design decision here, let's open a ticket as a maybe later.

ravinderk · 2023-04-27T13:23:18Z

includes/classes/Authentication.php

+		if ( ! isset( $args['headers'] ) ) {
+			$args['headers'] = array();
+		}
+		$args['headers']['X-Distributor-Version'] = DT_VERSION;


@peterwilsoncc I see we are repeating this multiple times in the code base. I am okay with explicitly adding this in the header, but I want to check if we can add this header automatically with a filter hook to any distributor-related call.

ravinderk · 2023-04-27T13:23:51Z

includes/classes/DistributorPost.php

@@ -816,6 +817,38 @@ protected function to_insert( $args = array() ) {
 		return $insert;
 	}

+	/**


@peterwilsoncc Can we add since tag?

I've created #1051 to add the annotations as I noticed I missed them for all the methods in the new class. I've added it to this method as it's introduced in this PR, see 671837f

ravinderk · 2023-04-27T13:30:03Z

includes/classes/InternalConnections/NetworkSiteConnection.php

 			add_filter( 'wp_insert_post_data', array( '\Distributor\InternalConnections\NetworkSiteConnection', 'maybe_set_modified_date' ), 10, 2 );

 			// Filter documented in includes/classes/ExternalConnections/WordPressExternalConnection.php
 			$new_post_args = Utils\post_args_allow_list( apply_filters( 'dt_pull_post_args', $post_array, $item_array['remote_post_id'], $post, $this ) );
-			$new_post_id   = wp_insert_post( wp_slash( $new_post_args ) );
-
+			if ( $update ) {


@peterwilsoncc wp_insert_post handles post update and insert. Can we use it? instead of if...else

wp_insert_post handles post update and insert. Can we use it? instead of if...else

wp_update_post includes a number of date, status and taxonomy checks that the previous code was either missing or doubling up, so I wanted to switch to the function to avoid the need to duplicate it.

includes/classes/InternalConnections/NetworkSiteConnection.php

includes/rest-api.php

ravinderk · 2023-04-27T13:37:50Z

@peterwilsoncc I am facing a weird issue when using this pull request. Check the screencast and let me know if You have questions.

Screen.Recording.2023-04-27.at.6.42.35.PM.mov

peterwilsoncc · 2023-04-28T00:57:47Z

Check the screencast and let me know if You have questions.

I'm unable to reproduce the issue, but will continue testing.

A few questions:

do you have a persistent cache installed
was it a fresh install from the branch or from develop
had you pulled on develop prior to switching branch
is it an internal connection or external
if external, is it MS - MS, Single Site - Single Site or Single - Multi-site
Do you have a bi-directional set up, IE both sites are connected as external connections or just single direction?

If I continue to have problems reproducing, I'll reach our via slack

ravinderk · 2023-05-24T07:00:45Z

@peterwilsoncc I am facing a weird issue when using this pull request. Check the screencast and let me know if You have questions.

Screen.Recording.2023-04-27.at.6.42.35.PM.mov

@peterwilsoncc I was able to reproduce the issue ☝️ with the following steps 👇

Step to reproduce the issue

Install two different versions on the main site and external connection site
Main Site: fix/1016-reduce-duplication-pulling-posts
External Site: develop
Setup external connection to External Site on Main Site
Visit Distributor -> Pull Content page

As I understand it, we are changing the data format without backward compatibility. There is the possibility that two connected websites can have two different versions of the distributor plugin. Any new change should be compatible with the following scenario. Feel free to ask any questions.

peterwilsoncc · 2023-05-24T23:12:16Z

@ravinderk that's covered by these lines

distributor/includes/rest-api.php

Lines 367 to 391 in 0efefda

    
           	/* 
        
           	 * Ensure Distributor requests are coming from a supported version. 
        
           	 * 
        
           	 * Changes to this endpoint in Distributor 2.0.0 require both the source and remote 
        
           	 * sites use a 2.x release of Distributor. This check ensures that the remote site 
        
           	 * is running a version of Distributor that supports the new endpoint. 
        
           	 * 
        
           	 * Development versions of the plugin and Non-Distributor requests are allowed 
        
           	 * to pass through this check. 
        
           	 */ 
        
           	if ( 
        
           		false === Utils\is_development_version() 
        
           		&& null !== $request->get_param( 'distributor_request' ) 
        
           		&& ( 
        
           			null === $request->get_header( 'X-Distributor-Version' ) 
        
           			|| version_compare( $request->get_header( 'X-Distributor-Version' ), '2.0.0', '<' ) 
        
           		) 
        
           	) { 
        
           		return new \WP_Error( 
        
           			'distributor_pull_content_permissions', 
        
           			esc_html__( 'Pulling content from external connections requires Distributor version 2.0.0 or later.', 'distributor' ), 
        
           			array( 'status' => 403 ) 
        
           		); 
        
           	}

As the version on develop is bumped during the release process, it's not possible to include the version compare check when running a development version of the plugin.

peterwilsoncc · 2023-05-25T03:02:17Z

Install two different versions on the main site and external connection site
Main Site: fix/1016-reduce-duplication-pulling-posts
External Site: develop

Sorry, I had this reversed. Taking a look.

peterwilsoncc · 2023-05-25T03:59:59Z

@ravinderk In c83fde1 I've pushed an additional check for the external version of Distributor to the pull screen.

As we bump DT_VERSION during the release process, the version check is ignored for development versions. You can remove composer.lock to see the error.

However, removing composer.lock will always cause the error to show as Distributor will think it's working with version 1.9.1.

I can jump on a call if you want me to clarify over a screen share.

peterwilsoncc self-assigned this Feb 20, 2023

peterwilsoncc added this to the 2.0.0 milestone Feb 20, 2023

peterwilsoncc commented Feb 20, 2023

View reviewed changes

peterwilsoncc force-pushed the fix/1016-reduce-duplication-pulling-posts branch from aea7570 to a26027a Compare February 22, 2023 23:33

Base automatically changed from fix/997-reduce-push-duplication to develop February 24, 2023 01:09

peterwilsoncc force-pushed the fix/1016-reduce-duplication-pulling-posts branch 2 times, most recently from 3c71b50 to 5c4f891 Compare March 15, 2023 04:27

peterwilsoncc commented Mar 15, 2023

View reviewed changes

includes/classes/Authentication.php

Copy link

Collaborator Author

peterwilsoncc Mar 15, 2023

Choose a reason for hiding this comment

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

Todo: Make sure this doesn't affect the wp.com oauth flow.

peterwilsoncc force-pushed the fix/1016-reduce-duplication-pulling-posts branch 3 times, most recently from d568542 to a9ce06e Compare March 17, 2023 01:29

jeffpaul requested review from a team and Sidsector9 and removed request for a team March 23, 2023 21:59

peterwilsoncc force-pushed the fix/1016-reduce-duplication-pulling-posts branch from d3d5e75 to 65cd23f Compare March 29, 2023 02:25

peterwilsoncc commented Mar 29, 2023

View reviewed changes

peterwilsoncc force-pushed the fix/1016-reduce-duplication-pulling-posts branch from 051325c to 2484a94 Compare March 29, 2023 02:59

peterwilsoncc force-pushed the fix/1016-reduce-duplication-pulling-posts branch from 337a70c to c4f49f1 Compare April 18, 2023 05:45

peterwilsoncc added 11 commits April 19, 2023 12:16

Use post abstraction for pulling network posts.

84c3f03

Rename get_pull_content to get_pull_content_list.

77080f6

Expose Distributor version in REST headers.

7ab2dad

Include X-Distributor-Version header in requests.

af11b17

Introduce helper function to determine if running a development versi…

4db4780

…on of the plugin.

Ensure compatible version of Distributor is used.

df37d77

Use post abstraction in pull list endpoint.

a844ea3

Add method to include additional data required for external pull scre…

12c2831

…ens.

Use new method for pull screen lists.

70a894c

Accept arrays of post types and statuses.

f0f7677

Introduce include parameter.

a66607d

Merge branch 'develop' into fix/1016-reduce-duplication-pulling-posts

05dcce6

dkotter requested changes Apr 25, 2023

View reviewed changes

peterwilsoncc added 4 commits April 26, 2023 10:35

Sanitize and slash post meta as appropriate.

fa68246

Replace placeholder versions with 2.0.0.

9dd5746

Fix post status argument sanitization.

827d4d6

Mock sanitize_url() as passthrough function.

e747fb6

dkotter previously approved these changes Apr 26, 2023

View reviewed changes

peterwilsoncc mentioned this pull request Apr 27, 2023

Added ability to view all post types when Pulling from an External Connection #1002

Merged

4 tasks

ravinderk suggested changes Apr 27, 2023

View reviewed changes

Use absint for positive integers.

27b729c

peterwilsoncc dismissed dkotter’s stale review via 27b729c April 28, 2023 00:18

peterwilsoncc added 4 commits April 28, 2023 10:20

Escape \ in callback.

62d165f

Remove duplicate validate_callback entry.

0efefda

Add since tag annotation.

671837f

Reproduce absint for test suite.

18eb1e4

jeffpaul requested review from ravinderk and removed request for jeffpaul May 22, 2023 14:13

Merge branch 'develop' into fix/1016-reduce-duplication-pulling-posts

442113f

Version check external version of Distributor on pull screen.

c83fde1

ravinderk approved these changes May 26, 2023

View reviewed changes

peterwilsoncc merged commit 3cd758f into develop May 26, 2023

peterwilsoncc deleted the fix/1016-reduce-duplication-pulling-posts branch May 26, 2023 04:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce duplication pulling posts #1017

Reduce duplication pulling posts #1017

peterwilsoncc commented Feb 20, 2023 •

edited

Loading

peterwilsoncc Feb 20, 2023

peterwilsoncc Mar 15, 2023

peterwilsoncc commented Mar 24, 2023

peterwilsoncc Mar 29, 2023

peterwilsoncc commented Apr 19, 2023

peterwilsoncc commented Apr 26, 2023

ravinderk left a comment

ravinderk Apr 27, 2023

peterwilsoncc Apr 27, 2023

ravinderk Apr 27, 2023

ravinderk Apr 27, 2023

peterwilsoncc Apr 28, 2023

ravinderk Apr 27, 2023

peterwilsoncc Apr 27, 2023

ravinderk commented Apr 27, 2023

peterwilsoncc commented Apr 28, 2023 •

edited

Loading

ravinderk commented May 24, 2023

peterwilsoncc commented May 24, 2023

peterwilsoncc commented May 25, 2023

peterwilsoncc commented May 25, 2023 •

edited

Loading

		'args' => [ \WP_Mock\Functions::type( 'int' ), 'dt_original_post_id', 2 ],
		'args' => [ \WP_Mock\Functions::type( 'int' ), 'dt_original_post_id', 111 ],

Reduce duplication pulling posts #1017

Reduce duplication pulling posts #1017

Conversation

peterwilsoncc commented Feb 20, 2023 • edited Loading

Description of the Change

How to test the Change

Changelog Entry

Credits

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterwilsoncc commented Mar 24, 2023

Choose a reason for hiding this comment

peterwilsoncc commented Apr 19, 2023

peterwilsoncc commented Apr 26, 2023

ravinderk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravinderk commented Apr 27, 2023

peterwilsoncc commented Apr 28, 2023 • edited Loading

ravinderk commented May 24, 2023

Step to reproduce the issue

peterwilsoncc commented May 24, 2023

peterwilsoncc commented May 25, 2023

peterwilsoncc commented May 25, 2023 • edited Loading

peterwilsoncc commented Feb 20, 2023 •

edited

Loading

peterwilsoncc commented Apr 28, 2023 •

edited

Loading

peterwilsoncc commented May 25, 2023 •

edited

Loading