Distributor: NetworkSiteConnection::update_syndicated running too early in a Gutenberg/REST context

[lakrisgubben]

Describe the bug
When updating a post that has been distributed to another site in a multisite (internal networkconnection) using Gutenberg, updates to meta, terms, etc are not pushed. The issue here seems to be that the update_syndicated method in the NetworkSiteConnection class is hooked to the save_post action. This works fine when using the classic editor, but when using Gutenberg and posts are saved through the REST-api, the save_post hook seems to fire earlier. Looking at the update_item method of the WP_REST_Posts_Controller class one can see that wp_update_postthat triggers the save_post action is called on line 697, and only after that are meta, terms etc handled by the REST-api. This means that when Distributor is running the update_item method, meta, terms etc haven't been updated/saved yet.

As a work around I've added the following code to an mu-plugin:

add_action(
    'rest_after_insert_post',
    function( WP_Post $post ): void {
        \Distributor\InternalConnections\NetworkSiteConnection::update_syndicated( $post->ID );
    }
);

This is not optimal since it means that the post will be distributed twice on save, it's no biggie either, but it makes saving a bit slower since update_syndicated is run twice.

I'm not sure what the best solution to this problem is to make it work with both Gutenberg and the classic editor. One could of course hook the update_syndicated method (with some minor modifications to account for the different values sent through the save_post and rest_after_insert_{$post_type} actions) to the rest_after_insert_{$post_type} action for all post_types where show_in_rest = true. This would mean that the update_syndicated method would be run twice, but that could maybe be handled by aborting that method if something like ( ( defined( 'REST_REQUEST' ) && REST_REQUEST ) && doing_action( 'save_post' ) ) was true. But I feel that there are probably smarter ways than that. ;)

Anyways, thanks for a great plugin and I'd be happy to help out with this issue if y'all are interested.

Steps to Reproduce

1. On a WordPress multisite create a post using Gutenberg with at least a title, category and a featured image. (Important, do not use any legacy metaboxes in Gutenberg because that triggers another save in WP to handle back-compat, which makes this issue go away)
2. Distribute this post to another site in the network and check that it looks good.
3. Change the title, category and featured image and update the post.
4. Check the distributed post on the other site and see that the title has changed but not the category or the featured image.
5. Update only the title of the post.
6. Check the distributed post on the other site and see that the title has changed and the category and featured image saved in step 3 has been distributed (since they where already saved when step 5 was run).

Expected behavior

Meta, terms etc should be distributed on update.

Environment information

• Distributor version: Latest from develop branch.
• Theme and version: Twenty Nineteen 1.4
• Other installed plugin(s) and version(s): none.
• WordPress 5.2.3

[jeffpaul]

@lakrisgubben thanks for the very detailed issue and research on this, it's greatly appreciated! I'm pulling this into our next milestoned release to see if we can get this resolved, though if you're able to work up a PR then I'd gladly get it through review an into an upcoming release. Thanks again!

[lakrisgubben]

Thanks for the reply @jeffpaul! Glad to help out with a PR on this, but to do that I'd like some input from someone more well-versed in the codebase to make sure the fix is made in a way that y'all are happy with. :)

[dkotter]

@lakrisgubben Thanks for bringing this to our attention. I've run across a slightly similar Gutenberg issue before but had not encountered this particular one yet.

Honestly, not sure there is a great approach here, as we're mostly reliant on how Gutenberg works. This is a known issue (see here for one thread about it: https://github.com/WordPress/gutenberg/issues/12903) and it sounds like they're trying to figure out a good approach for similar scenarios.

Here's what I would suggest, which is pretty similar to your approach:

• We have a is_using_gutenberg helper method that will tell you if a particular post is written using Gutenberg. I think we can use this, within the update_syndicate method, to determine if the post is using Gutenberg or not
• If it is, we can add a new action, hooked to rest_after_insert_{$post_type}, that calls that same update_syndicate method. And then return early, so the rest of the code doesn't run (and we don't get double updates)
• We also need to check the action that is running, so when the update_syndicate method is called by the rest_after_insert_{$post_type} hook, it knows to actually run all the code at that point. So something like \Distributor\Utils\is_using_gutenberg( $post ) && doing_action( 'save_post' )
• And since these hooks use different arguments (one is post ID, one is the post object), we'll need to handle that. I think it should be as simple as calling get_post( $post_id ), as get_post accepts either an ID or object

Let me know if you have any questions on this approach (or any concerns). I haven't tested this yet but I think, in theory, this should work and should prevent double updates from happening.

[lakrisgubben]

@dkotter Thanks for the detailed reply! I took a stab at implementing a fix based on your suggestions and found another problem that I hadn't thought of before. :)

Adding something like

if ( \Distributor\Utils\is_using_gutenberg( $post ) && doing_action( 'save_post' ) ) {
    add_action( "rest_after_insert_{$post->post_type}", array( '\Distributor\InternalConnections\NetworkSiteConnection', 'update_syndicated' ) );
    return;
}

makes the updating of a syndicated article work both with the classic editor and with Gutenberg using no legacy metaboxes. But it breaks when using Gutenberg with legacy metaboxes because those are saved in a separate post request run after the rest request that does not trigger the rest_after_insert_{$post->post_type} hook, meaning the metadata from legacy metaboxes are not syndicated on update.

As you say there doesn't seem to be a great way to handle this situation, but adding ! isset( $_GET['meta-box-loader'] ) to the if-statement above at least makes it work. This will mean that the updated post is syndicated twice if using Gutenberg and legacy metaboxes, but I guess that's better than beeing partially syndicated and that's also how it's currently working.

So adding this to the update_syndicated method seems to me (as far as I've been able to test it) to make this work great with both classic editor and Gutenberg and "okay" with Gutenberg using legacy metaboxes:

if ( \Distributor\Utils\is_using_gutenberg( $post ) && doing_action( 'save_post' ) && ! isset( $_GET['meta-box-loader'] ) ) {
    add_action( "rest_after_insert_{$post->post_type}", array( '\Distributor\InternalConnections\NetworkSiteConnection', 'update_syndicated' ) );
    return;
}

@dkotter let me know if you think this solution seems reasonable or if you can think of another way to deal with this problem and then I can make a PR for it.

[dkotter]

@lakrisgubben Thanks for testing that out. I knew Gutenberg made two requests if you have a custom meta box (which has caused issues on other projects) but I guess I didn't realize the rest hooks don't fire in those cases (I think they only fire if the meta is registered using register_post_meta and the show_in_rest value is true).

It seems like that meta-box-loader variable should always be set if we're processing custom meta, though does seem non-ideal to rely on that. But there doesn't seem to be a core way to solve this and all other approaches I've seen mentioned seem even more non-ideal, so I think this is a decent way forward.

Happy to test a PR if you have time to put that together. Thanks for all the effort here!

[lakrisgubben]

@dkotter first pass at PR here: https://github.com/10up/distributor/pull/518