WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 10 days ago

Last modified 6 days ago

#45114 closed defect (bug) (fixed)

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

Reported by: peterwilsoncc Owned by: peterwilsoncc
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: REST API Keywords: dev-feedback has-patch commit needs-dev-note
Focuses: Cc:

Description

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.

Attachments (4)

45114-do-not-merge.diff (4.2 KB) - added by peterwilsoncc 2 years ago.
45114.diff (7.6 KB) - added by peterwilsoncc 2 years ago.
45114-alt.diff (2.6 KB) - added by kraftbj 9 months ago.
Alterative approach of moving the firing order within the REST controller.
45114.2.diff (10.4 KB) - added by peterwilsoncc 10 days ago.

Download all attachments as: .zip

Change History (51)

#1 @kadamwhite
2 years ago

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.

This ticket was mentioned in Slack in #core-editor by peterwilsoncc. View the logs.


2 years ago

#3 @peterwilsoncc
2 years ago

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.

#4 @peterwilsoncc
2 years ago

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.

This ticket was mentioned in Slack in #core-editor by peterwilsoncc. View the logs.


2 years ago

#6 @danielbachhuber
2 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed

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
2 years ago

#7 @peterwilsoncc
2 years ago

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.

#8 @danielbachhuber
2 years ago

@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?

This ticket was mentioned in Slack in #core-restapi by danielbachhuber. View the logs.


2 years ago

#10 @danielbachhuber
2 years ago

  • Resolution maybelater deleted
  • Status changed from closed to reopened

#12 @peterwilsoncc
2 years ago

  • Summary changed from Publish posts and fire related hooks on `rest_after_insert_{$this->post_type}` for rest requests. to Fire publishing related hooks after all data is saved via the REST API.

@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

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


20 months ago

#14 @peterwilsoncc
20 months ago

#46441 was marked as a duplicate.

#15 @peterwilsoncc
20 months ago

#46566 was marked as a duplicate.

#16 @SergeyBiryukov
19 months ago

  • Milestone set to Awaiting Review

#17 @peterwilsoncc
14 months ago

#47908 was marked as a duplicate.

#18 @TimothyBlynJacobs
11 months ago

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

#19 @talldanwp
10 months ago

#48790 was marked as a duplicate.

#20 @peterwilsoncc
10 months ago

@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.

#21 @kraftbj
9 months ago

  • Type changed from enhancement to defect (bug)

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):

  1. We fire off an action at wp_insert_post to handle post changes.
  2. 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).
  3. 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.
  4. 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.

#22 @kraftbj
9 months ago

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.
  1. For non-updates, the logic in wp_insert_post is pretty simple to get an ID -- just $wpdb->insert into the posts table.
  1. 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.
  1. 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).

Last edited 9 months ago by kraftbj (previous) (diff)

@kraftbj
9 months ago

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

#23 @kraftbj
9 months ago

  • Keywords dev-feedback added

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.

#24 @Collizo4sky
5 months ago

Any news on this?

#25 @peterwilsoncc
3 months ago

@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.

This ticket was mentioned in Slack in #core-restapi by nickirongate. View the logs.


2 months ago

#27 follow-up: @TimothyBlynJacobs
2 months ago

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.

#28 in reply to: ↑ 27 @kraftbj
5 weeks ago

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.

#29 @peterwilsoncc
5 weeks ago

  • Milestone changed from Awaiting Review to 5.6

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.

#30 follow-up: @TimothyBlynJacobs
4 weeks ago

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.

#31 in reply to: ↑ 30 @peterwilsoncc
4 weeks ago

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.

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


4 weeks ago

This ticket was mentioned in Slack in #core by thelmachido. View the logs.


4 weeks ago

#34 @peterwilsoncc
4 weeks ago

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

This ticket was mentioned in PR #589 on WordPress/wordpress-develop by peterwilsoncc.


13 days ago

  • Keywords has-patch added

#36 @peterwilsoncc
13 days ago

  • Keywords has-patch removed

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

#37 @kraftbj
11 days ago

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.

#38 @peterwilsoncc
11 days ago

  • Keywords has-patch added

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.

#39 @kraftbj
11 days ago

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. :)

#40 @TimothyBlynJacobs
11 days ago

@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().

#41 @peterwilsoncc
11 days ago

@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.

#42 @TimothyBlynJacobs
10 days ago

  • Keywords commit added

Looks good to me!

#43 @peterwilsoncc
10 days ago

  • Keywords needs-dev-note added

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

#44 @peterwilsoncc
10 days ago

  • Owner set to peterwilsoncc
  • Resolution set to fixed
  • Status changed from reopened to closed

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.

#45 @Collizo4sky
10 days ago

Hurray!

Many thanks to everyone who made this happen.

#46 @SergeyBiryukov
10 days ago

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.

This ticket was mentioned in Slack in #core by sncoker. View the logs.


6 days ago

Note: See TracTickets for help on using tickets.