Fix/pull apply the content #498
Conversation
|
@dkotter as you have availability, it would be great to get this through review so we can make any necessary changes and get this to a merge (and potential Distributor minor release to resolve this). |
| @@ -267,6 +267,12 @@ public function pull( $items ) { | |||
|
|
|||
| restore_current_blog(); | |||
|
|
|||
| // Allow the sync'ed post to be updated via a REST request to | |||
There was a problem hiding this comment.
@petenelson If you could change these code comments into an actual DocBlock, similar to how most of the other actions are documented (and that should follow https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/php/#4-hooks-actions-and-filters), that would be great
|
|
||
| $request = apply_filters( 'dt_update_content_via_request_args', [], $new_post_id, $this ); | ||
|
|
||
| $response = wp_remote_get( $rest_url, $request ); |
There was a problem hiding this comment.
To support VIP, I think we'll need to enhance this a bit. Basically we need to check if we are on WordPress.com VIP and if so, use the vip_safe_wp_remote_get function. Can see examples of this here: https://github.com/10up/distributor/blob/develop/includes/classes/ExternalConnections/WordPressExternalConnection.php#L184
|
|
||
| $response = wp_remote_get( $rest_url, $request ); | ||
|
|
||
| $body = wp_remote_retrieve_body( $response ); |
There was a problem hiding this comment.
We'll want to add a few more checks here, just to be safe. Basically check the $response value isn't a WP Error and check that the response code (wp_remote_retrieve_response_code) matches what we want. Can see examples of that here: https://github.com/10up/distributor/blob/develop/includes/classes/ExternalConnections/WordPressExternalConnection.php#L200
There was a problem hiding this comment.
FYI, wp_remote_retrieve_body() already checks for WP_Error.
There was a problem hiding this comment.
Ah, didn't realize that. I still like the explicit check, so we can output a different error message in each instance, to better help with debugging issues.
| @@ -267,6 +267,12 @@ public function pull( $items ) { | |||
|
|
|||
| restore_current_blog(); | |||
|
|
|||
| // Allow the sync'ed post to be updated via a REST request to | |||
| // get the rendered content. | |||
| if ( apply_filters( 'dt_pull_post_apply_rendered_content', true, $new_post_id, $this, $post_array ) ) { | |||
There was a problem hiding this comment.
Not a CR comment but wondering if we want to default this to false? So sites that need the extra step of pulling in rendered content can opt-in but this won't impact other setups that don't need this? cc/ @jeffpaul
There was a problem hiding this comment.
I was thinking the same thing, with an eventual option in the settings to turn that off without a code change.
There was a problem hiding this comment.
My only concern is we're currently doing this for pushed posts so I imagine the perception is that we'd do this by default for pulled posts as well. I think I'd rather support the broader case and allow sites that know they don't need to render content first before pulling could adjust the default to false and gain back whatever performance the HTTP call is impacting. Unless I'm misunderstanding things or not fully thinking through other scenarios, if so please call those out.
|
@petenelson looks like there's a merge conflict on this, could you help resolve that so we can look to merge this in? |
|
Merge conflict has been resolved |
Description
Make a REST request for rendered post content after pulling a post from a network connection.
Benefits
Allows the pulled post to get rendered shortcodes and anything else updated by
the_contentfilter.Possible Drawbacks
The additional HTTP request has a small performance impact.
Checklist:
Applicable Issues
#497