Fire publishing related hooks after all data is saved via the REST API.

[peterwilsoncc]

In REST API requests, wp_insert_post publishes posts before terms and post meta has been saved.

This causes a number of hooks to fire before the meta data and terms are available in the database. As a follow up to [43737] it's possible to publish posts and fire these hooks after all the data has been saved.

On REST Requests (both via HTTP and internal) it would be good to move the following to the later action:

• edit_attachment
• attachment_updated
• add_attachment
• edit_post
• post_updated
• save_post_{$post->post_type}
• save_post
• wp_insert_post
• setting the status to publish, future, etc
• Related hooks I have missed

To handle both HTTP and internal requests would require a new function that normally returns false but is filtered to return true at the start of a REST request and removing the filter at the end of the request.

This new filter could be added at around the same location as rest_pre_dispatch is fired and removed at the point rest_request_after_callbacks fires.

[kadamwhite]

While discussing this in Slack, @peterwilsoncc proposed the pseudocode

function is_rest_request() {
   return apply_filters ('is_rest_request', false);
}

add_filter( 'rest_pre_dispatch', function(){
    add_filter( 'is_rest_request', '__return_true' );
} )

add_filter( 'rest_request_after_callbacks', function(){
    remove_filter( 'is_rest_request', '__return_true' );
} )

I wonder (without having done any actual research into this) when else we'd need to remove that filter so that is_rest_request returns false again: possibly situations where we encounter an error while fulfilling the request (in case the request is being dispatched from within the PHP layer and that error is handled somehow); or if there are permissions failures or other changes of state during a request lifecycle.

[peterwilsoncc]

wp_is_json_request() was added in [43730] for #44534.

It doesn't handle internal requests but it would be helpful if it did.

I've realised my pseudo code won't work either as an internally dispatched request from another REST request such as those in \WP_REST_Server::embed_links() would remove the filter.

[peterwilsoncc]

In 45114-do-not-merge.diff​, I've created a very, very rough POC for the post endpoint.

• Adds a new private function for firing the hooks after saving a post
• By default, wp_insert_post will fire these hooks
• The REST API will wait until all data is fired before firing the hooks
• Only includes the post endpoint, attachements and other end points need changes
• Ideally I would like to delay the post status change until after all data has been saved via the API but I am not sure of a good approach.
• This is the ultimate in hacky hacks, in no way should it be merged without change.

[danielbachhuber]

For this particular issue, I think [43737] is the realistic path forward at this point. Changing the hook execution order in REST requests seems like it would have other unintended consequences.

You can use defined( 'REST_REQUEST' ) && REST_REQUEST in your existing classic callbacks to bail early for REST requests, and then [43737] to re-hook your behavior into REST requests.

[peterwilsoncc]

I've thought about this further and continue to think firing the after post save hooks before all data has been updated will be more problematic than changing the hooks.

A cursory glance at Jetpack shows it expects post meta to be saved on one of these hooks (within wp_transition_post_status(), looking at Yoast SEO it expects term data to be saved on one of these hooks (save_post).

As @pento said in his ​recent post on merge proposals, the REST API currently has low level of adoption. From version 5.0, it will reach roughly 100% of sites.

I've attached 45114.diff​ which I think has problems, but substantially fewer than the original POC:

• only required data is passed to the new function, inferred data is calculated
• the bypass option is placed inside an args setting within the post array, following a similar pattern used for add_meta_box()'s block editor setting
• short array syntax removed to avoid fatals in old PHP

As REST_REQUEST is only defined within HTTP requests to the rest API but not internal requests, it's an unsuitable check for the purpose of moving these hooks.

Rather than start a status war, I am leaving this in a closed state for discussion to continue.

[danielbachhuber]

@peterwilsoncc I'm slightly terrified of making any chances to wp_insert_post() at the risk of the sky falling on our heads. Is it possible to solve for this problem within the REST API controllers instead?

As REST_REQUEST is only defined within HTTP requests to the rest API but not internal requests, it's an unsuitable check for the purpose of moving these hooks.

What are some real-world contexts where internal, not HTTP, requests would be used for write operations?

[danielbachhuber]

Re-opening per conversation in Slack ​https://wordpress.slack.com/archives/C02RQC26G/p1541289407107900

[peterwilsoncc]

@danielbachhuber renaming as requested during our Slack conversation with Pento.

Specific use cases I could see that hit this are:

• Yoast SEO: Accesses terms and write the primary term to meta data
• Jetpack: Uses the save_post for sending data required for their publisize module (a similar use case on my site sent me down this path)
• CMB2, ACF both make use of this group of hooks and have taxonomy/terms custom meta types - I haven't had a chance to dig into this too deeply but it hints toward expected data.

In terms of real worlds uses of internal write requests:

• Writing and running tests (ie, ensuring test code and prod code runs the same)
• WooCommerce has a batch API endpoint, although I think it's intended use case has REST_REQUEST set to true, it just rules out the filter option I shared with Kadam and he mentioned above)
• I've done it in prior projects because writing post data and term or meta data via the API internal request is nicer than using wp_insert_post

[peterwilsoncc]

#46441 was marked as a duplicate.

[peterwilsoncc]

#46566 was marked as a duplicate.

[peterwilsoncc]

#47908 was marked as a duplicate.

[TimothyBlynJacobs]

A year post Gutenberg launch, is this still something we're interested in doing?

[talldanwp]

#48790 was marked as a duplicate.

[peterwilsoncc]

@TimothyBlynJacobs

I'm in two minds as to whether to still do this.

It was introduced as an accidental back-compat break with the REST API but now the new editor has been in core for a while, I'm wondering if doing this now would also break back-compat on a bunch of sites and plugins that have worked around it.

[kraftbj]

I'd still expect these hooks to fire after the complete data has been saved and personally vote to still correct this.

The particular use case I'm running into (a separate Jetpack instance from what is mentioned above):

0. We fire off an action at wp_insert_post to handle post changes.
1. We have a filter for an array of categories that a post in any of those categories would be excluded from being e-mailed out (our Subscriptions feature).
2. We hook onto wp_transition_post_status to check if a post is being newly published to begin the process of flagging a post to be e-mailed out.
3. Within that, we check if that being-published post is in a category that matches one from the array in item 1 above.

In the Classic Editor or non-REST requests, wp_insert_post adds the categories/tags/terms at L3937 ​https://github.com/WordPress/WordPress/blob/5.3/wp-includes/post.php#L3937 and then L4136 ​https://github.com/WordPress/WordPress/blob/5.3/wp-includes/post.php#L4136 at the end of all of the work, actually fires off the function.

In the REST API, update_item fires wp_update_post (which through all of the steps fires wp_insert_post and that hook) at L692 ( ​https://github.com/WordPress/WordPress/blob/5.3/wp-includes/post.php#L4136 ) and then later handle_term for categories, etc at L730 ​https://github.com/WordPress/WordPress/blob/5.3/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L730 .

While I think this should be fixed so the behavior of wp_insert_post, et al matches the docs and previous behavior going back to 2.0, if nothing else, could we add a *new* hook that fires at the end of both processes.

I kid with the name, but a wp_insert_post_for_real that we can add both in the class-wp-rest-posts-controller.php before the response AND at the end of post.php where wp_insert_post is already.

The inline documentation of those hooks should be modified to clarify the different behaviors and that they're somewhat unreliable, to be frank.

[kraftbj]

Looking at this a bit closer, I'm interested in attempting the following approach:

1. It appears the primary reason create_item in the REST posts controller runs wp_insert_post so early is to get a post ID back for use in the rest of the insert method.

2. For non-updates, the logic in wp_insert_post is pretty simple to get an ID -- just $wpdb->insert into the posts table.

3. Create a new Core function to "reserve" a post ID. For BC with wp_insert_post, the status can be new to mimic what wp_insert_post is doing and have a default dummy/blank information.

4. The REST controller can call that function to get a post ID, do the work it is doing with the extra information and then call wp_insert_post to add everything (including the proper status) to the post.

An alternative could be to do wp_insert_post as it is done right now, except not pass the status, retaining it for later. Then, just prior to rest_after_insert_ hook, update the post with the proper status.

I'm thinking the second scenario may be safer and would in particular help with my case of a "published" post firing the wp_insert_post hook prematurely, but open to anyone's thoughts about it.

(Further note - the same shifting of wp_update_post needs to happen in update_item within the REST controller too).

[kraftbj]

Alterative approach of moving the firing order within the REST controller.

[kraftbj]

For the sake of conversation, 45114-alt.diff only modifies the REST controller.

For a new post, I'm somewhat mocking what wp_insert_post does in a similar case by setting a "new" post status. To keep intent, I'm storing the passed status and then wp_update_post at the end with the intended status.

wp_insert_post hooks will still fire early, but with a more "expected" status. wp_update_post at the conclusion of the method will fire the hooks again, but things like transitioning post status to publish will be accurate.

For the update, I just moved the call to wp_update_post to the end. Same thing in that now hooks that are firing on publish will have fully fleshed data available.

I don't expect this to be a complete solution and haven't audited the list of hooks mentioned in the OP to ensure this hits all those cases. Posting early to see if this direction seems feasible.

[Collizo4sky]

Any news on this?

[peterwilsoncc]

@kraftbj

Thanks for the patch.

For updates, I think moving wp_insert_post() ends up reversing the problem.

The post term and meta data functions' filters end up running before the post content is updated, the first thing that comes to mind is the potential for this to cause problems with wp_update_term_count() and family when the post status changes.

Similar is true for inserting but with auto-draft it's not as much of an issue.

FWIW, I have been experimenting with a plugin to handle this but once I started writing unit tests it proved to be broken. I haven't been able to find an approach that doesn't involve firing the after post updates in a seperate function.

[TimothyBlynJacobs]

Another thing is that if someone is updating things on the rest_insert_post or rest_after_insert_post hooks. Then save_post won't be accurate I don't think.

It was introduced as an accidental back-compat break with the REST API but now the new editor has been in core for a while, I'm wondering if doing this now would also break back-compat on a bunch of sites and plugins that have worked around it.

I definitely think this is a risk. And to me it seems conceptually simpler that for block editor post types, use rest_after_insert and for others use save_post and call it a day.

I haven't been able to find an approach that doesn't involve firing the after post updates in a seperate function.

Yeah I'm having a hard time seeing a solution other than this. But it also feels kinda fragile.

---

I wonder if there is a more resilient way of doing these kinds of checks that rely on save_post in the first place? Because while the REST API is the primary way people are having this issue, it would happen with any code that does a wp_insert_post() and then updates meta fields or uses wp_set_post_terms afterwords, which is pretty common for plugins to do.

My instinct is to write it something like this:

<?php
$queue = [];

add_action( 'save_post', function ( $post_id ) use ( &$queue ) {
        $queue[] = $post_id;
} );

add_action( 'shutdown', function () use ( $queue ) {
        foreach ( $queue as $post_id ) {
                // Get the latest copy of the post.
                $post = get_post( $post_id );
        }
} );

I think that'd work for both the REST API, the Classic Editor, or any code that creates post.

[kraftbj]

Replying to TimothyBlynJacobs:

I definitely think this is a risk. And to me it seems conceptually simpler that for block editor post types, use rest_after_insert and for others use save_post and call it a day.

That doesn't work because the REST API isn't the exclusive way to edit those post types. Ignoring the Classic Editor, something like someone using a XML-RPC client (or Press This) to save a post sometimes and using the block editor sometimes.

I wonder if there is a more resilient way of doing these kinds of checks that rely on save_post in the first place? Because while the REST API is the primary way people are having this issue, it would happen with any code that does a wp_insert_post() and then updates meta fields or uses wp_set_post_terms afterwords, which is pretty common for plugins to do.

My instinct is to write it something like this:

This feels suboptimal to me given the simplicity of being able to use save_post before, but probably the most practical solution.

[peterwilsoncc]

Moving this on to the 5.6 milestone for visibility.

My proposal is:

• split functions per 45114.diff​
• in REST requests, prevent wp_insert_post() from firing the hooks
• Fire the hooks after term and meta updates are processed in REST requests just prior to the existing hooks
• add some unit tests to the patch

Helen has suggested bringing this ticket up in a dev-chat which I shall do next week.

The key questions I have are:

• is it too late to fix this back-compat change?
• would a _fix_ end up breaking a bunch of plugins?
• rubber-ducking how this could break things.

[TimothyBlynJacobs]

My general thoughts are that using the save_post hook to fire those types of actions that require the full state of the post immediately isn't the best way to do it.

Any instance where all the post data can't be added via wp_insert_post arguments will be affected by this. Which is why in my mind when you need the absolute completed state of the object, and the action performed is final ( you aren't listening for hooks to sync things ), then save_post should be used as an indicator that a new post is coming and be queued for action, but not immediately processed following a pattern similar to what I outlined in comment:27.

This would allow the REST API publishing case to be fixed, but it means the underlying problem wouldn't be fixed.

---

Now, if we do want to make that change anyways.

is it too late to fix this back-compat change?

I don't think it is too late assuming we don't cause breakage of the plugins that have implemented alternatives like listening for rest_after_insert_.

would a _fix_ end up breaking a bunch of plugins?

I think this could be a possibility and the breakage might be subtle which would be particularly annoying.

rubber-ducking how this could break things.

One possibility would be if developers are expecting a post to have been created first ( and listening for it with save_post ), before their callbacks that are attached to meta or term updates are fired.

[peterwilsoncc]

Replying to TimothyBlynJacobs:

Any instance where all the post data can't be added via wp_insert_post arguments will be affected by this. Which is why in my mind when you need the absolute completed state of the object, and the action performed is final ( you aren't listening for hooks to sync things ), then save_post should be used as an indicator that a new post is coming and be queued for action, but not immediately processed following a pattern similar to what I outlined in comment:27.

This is true but the typical use case for the hooks at the end of wp_insert_post() has been to update or read post data entered via the edit screen. This was lost once the block editor switched to using the REST API.

Separating the hooks to a seperate function would also allow for plugins adding posts added programatically to make use of the hooks by calling them after terms and meta data has been added. In that sense, I think it does solve the underlying problem.

Re: the code sample above, it prevents the code running on such hooks from accessing both the old and new state on transitioning posts. It would be possible to pass that around but that would still miss having an up-to-date post when Core creates revisions, processing term counts and some other functionality. I've experimented with ​moving the actions to later hooks in a plugin and it causes other problems.

Using the shutdown hook also becomes problematic when adding multiple posts via wp-cli, as the shutdown hook only runs once rather than each time a post is updated.

---

I accept your concern it's risky and I regret not pushing harder for a fix back in 5.0 but having tried several approaches haven't been able to discover a less risky way of doing it.

[peterwilsoncc]

This was discussed at length in the 0500 UTC dev-chat on September 30, 2020.

Out of this discussion, three options were suggested for this ticket:

1. close wontfix
2. move the hooks per my suggestion above
3. add a new hook/s that fire after posts, their terms and meta data are all updated common to the classic editor, REST API, XML-RPC and other methods for updating a post.

At the conclusion of the discussion, attendees were asked for their preference and new hook/s was in the majority, followed by moving the existing hooks in the REST API.

Emoji reaction votes are not scientific and lack nuance, but it's worth noting the people expressing an opinion all had many years of experience contributing the WP Core. The poll can be found in Slack at ​https://wordpress.slack.com/archives/C02RQBWTW/p1601444099240400

Trac ticket: https://core.trac.wordpress.org/ticket/45114

[peterwilsoncc]

I've put together a pull request with a new hook and a new parameter on wp_insert_post() to determine whether it should be called.

In terms of naming things I am not sure the ones I have chosen are the best but they get the proof of concept out at the very least.

For the hook name, these are other items in my brain dump:

wp_save_post_final
wp_save_post_finalized
wp_save_post_complete
wp_save_post_terms_meta
wp_save_post_populated

[kraftbj]

I like the approach taken in the PR. Given that this has been out of alignment since 5.0, I can appreciate keeping the behavior in the same inconsistent (from my POV) state for the existing hooks and this is a nice solution to provide a new hook that is universal.

[peterwilsoncc]

Thanks @kraftbj -- are you happy with the name, wp_after_insert_post, or do you think it need further work.

Returning the keyword I somehow removed.

[kraftbj]

Re the name, I'm happy with it. wp_after_insert_post is similar enough to the existing hook that mental muscle memory still kicks in. "final", "finalized" imply things that I don't know they need to. "complete" would be okay too. "terms_meta" and "populated" feel off to me.

All of this is subjective and I don't have any other particular ideas. So, yes, happy with wp_after_insert_post. :)

[TimothyBlynJacobs]

@peterwilsoncc What do you think about introducing a function for this like wp_transition_post_status. We're calling it in a number of places already, and I think we'd want to encourage plugin authors to use it whenever they have to follow a pattern where there are more updates they have to do after calling wp_insert_post().

[peterwilsoncc]

@TimothyBlynJacobs Thanks for the review, I've updated the PR to do just that along with adding the call in two places I'd missed.

[TimothyBlynJacobs]

Looks good to me!

[peterwilsoncc]

45114.2.diff​ brings the pull request over to trac for archival purposes.

Along with those active on this ticket, props is due for some discussion in Slack: thewebprincess, mikeschroder, noisysocks, markparnell, aristath, joyously

[peterwilsoncc]

In 49172:

REST API, Posts: Add a hook to fire once a post, its terms and meta update.

Introduces the action wp_after_insert_post inside a wrapper function of the same name. This hook allows plugin developers to access a posts full data (including its terms and meta data) regardless of the workflow used to save it.

A new parameter is introduced to wp_insert_post() to indicate whether the hook should be fired within the function call or will be fired afterward.

Props aristath, Collizo4sky, danielbachhuber, joyously, kadamwhite, kraftbj, markparnell, mikeschroder, noisysocks, peterwilsoncc, SergeyBiryukov, talldanwp, thewebprincess, TimothyBlynJacobs.
Fixes #45114.

[Collizo4sky]

Hurray!

Many thanks to everyone who made this happen.

[SergeyBiryukov]

In 49173:

Docs: Add @since tags for the fire_after_hooks parameter of wp_insert_post(), wp_update_post(), and wp_insert_attachment().

Document the parameter as optional.

Follow-up to [49172].

See #45114.

[SergeyBiryukov]

In 49673:

Docs: Correct $fire_after_hooks parameter name in @since tags.

Follow-up to [49172], [49173].

See #45114.

[SergeyBiryukov]

Reopening for backporting [49673] to the 5.6 branch after a second committer's review.

[peterwilsoncc]

Approved for backporting to the 5.6 branch.

[SergeyBiryukov]

In 49684:

Docs: Correct $fire_after_hooks parameter name in @since tags.

Follow-up to [49172], [49173].

Merges [49673] to the 5.6 branch.
Reviewed by peterwilsoncc, SergeyBiryukov.
Fixes #45114.

[Collizo4sky]

I don't know if this is too late but can we get the post old status included in the wp_after_insert_post action?

This is important for one of my plugin where we trigger certain actions only if a post changes from draft to publish.

Currently, wp_transition_post_status() is called too early before all post meta data are saved.

[johnbillion]

Reopening based on the above. More context is always useful.

Any objections?

[TimothyBlynJacobs]

I think if we provide more context, it'd be better to provide the whole previous post object.

[Collizo4sky]

Very happy this is being considered. +1 to Timothy's suggestion.

Currently, my workaround feels hacky.

            add_action('transition_post_status', function ($new_status, $old_status, WP_Post $post) {

                 global $mo_old_post_status;

                 if (isset($post->ID)) {
                     $mo_old_post_status[$post->ID] = $old_status;
                  }

            }, 1, 3);


            add_action('wp_after_insert_post', function ($post_id, WP_Post $post, $update) {

                global $mo_old_post_status;

                $this->new_publish_post($post->post_status, $mo_old_post_status[$post_id], $post);

            }, 1, 3);
       

Would love to eliminate the use of the globals.

[peterwilsoncc]

I'll check the various locations the hook is called to see if it's possible (mainly to determine if a post is new or not).

If revisions are enabled it's possible to get the previous version with wp_get_post_revisions( $post_id, [ 'numberposts' => 1, 'offset' => 1 ] );. Granted that's not ideal for plugin authors who don't know if the site has revisions on or off but I wanted to note it for now.

[hellofromTonya]

@peterwilsoncc @johnbillion Is there work here that needs to be completed for 5.6 RC2 (tomorrow)?

[peterwilsoncc]

wp_insert_post() only sets the $post_before variable for posts that are been updated, only $previous_status is set for new posts.

For this to go in, a design decision is needed for what $post_before looks like for new posts.

• false (which is what get_post( /* non existent post */ ) returns)
• An almost empty post object: new WP_Post( [ 'post_status' => 'new', 'post_type' => 'blah' ] ); -- this would need an additional design decision for any other defaults.

@johnbillion @TimothyBlynJacobs Do you have thoughts? I am slightly tempted by the latter as it is a consistent type but it needs other decisions about the defaults (time stamps, etc) as it's a new way of using WP_Post in core.

The new way of doing things/inconsistent type was why I didn't include it earlier. The third option is to hold off until 5.7 as an enhancement.

[TimothyBlynJacobs]

I think it would be null not false right? ​https://github.com/WordPress/wordpress-develop/blob/0d53804ea3e8a9dc40f8b6c9c481aa7a8fecb77c/src/wp-includes/post.php#L784

That'd be my personal preference because we're clearly saying this post didn't exist before.

I think we could conceivably hold off to 5.7, but if plugins adopt calling the new action immediately, that could lead to more inconsistencies.

Trac ticket: https://core.trac.wordpress.org/ticket/45114#comment:53

[peterwilsoncc]

I've ​created a pull request to send the additional data and added some unit tests to make sure the post object in it's before state isn't a pointer to the edited state (ie, that the object has been cloned one way or another).

@TimothyBlynJacobs @johnbillion and others

As I wrote the code I don't wish to play either of the review roles this close to RC2. I think the tests are pretty thorougher.

[TimothyBlynJacobs]

The changes look good to me.

[SergeyBiryukov]

I think the PR does address the concern raised in comment:53, looks good to me.

[johnbillion]

This change looks good to me too and the tests are solid. Let's do it.

[johnbillion]

Spotted an issue here: ​https://github.com/WordPress/wordpress-develop/blob/17be19d0c4edcad9e4de0c70d877914ce65b7040/src/wp-admin/includes/post.php#L683-L696

wp_after_insert_post() gets called twice on the new post screen due to this. The $fire_after_hooks parameter of wp_insert_post() is set to true but wp_after_insert_post() is also manually called after setting the post format.

[johnbillion]

In 49731:

REST API: Pass the previous state of the post as a parameter to the wp_after_insert_post hook.

This enables, for example, the previous post status to be used by this hook without the need to first capture it on an earlier hook.

This also fixes the value of the $fire_after_hooks parameter in get_default_post_to_edit() so the wp_after_insert_post action correctly fires just once on the new post screen.

Props Collizo4sky, peterwilsoncc, hellofromTonya, TimothyBlynJacobs, SergeyBiryukov

Fixes #45114

[johnbillion]

In 49732:

REST API: Pass the previous state of the post as a parameter to the wp_after_insert_post hook.

This enables, for example, the previous post status to be used by this hook without the need to first capture it on an earlier hook.

This also fixes the value of the $fire_after_hooks parameter in get_default_post_to_edit() so the wp_after_insert_post action correctly fires just once on the new post screen.

This merges [45114] into the 5.6 branch

See #45114

Merged in changeset https://core.trac.wordpress.org/changeset/49731