WordPress.org

Make WordPress Core

Opened 12 months ago

Last modified 2 months ago

#45114 reopened enhancement

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

Reported by: peterwilsoncc Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: REST API Keywords:
Focuses: Cc:
PR Number:

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 (2)

45114-do-not-merge.diff (4.2 KB) - added by peterwilsoncc 12 months ago.
45114.diff (7.6 KB) - added by peterwilsoncc 12 months ago.

Download all attachments as: .zip

Change History (19)

#1 @kadamwhite
12 months 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.


12 months ago

#3 @peterwilsoncc
12 months 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
12 months 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.


12 months ago

#6 @danielbachhuber
12 months 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.

#7 @peterwilsoncc
12 months 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
12 months 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.


12 months ago

#10 @danielbachhuber
12 months ago

  • Resolution maybelater deleted
  • Status changed from closed to reopened

#12 @peterwilsoncc
12 months 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.


7 months ago

#14 @peterwilsoncc
7 months ago

#46441 was marked as a duplicate.

#15 @peterwilsoncc
7 months ago

#46566 was marked as a duplicate.

#16 @SergeyBiryukov
7 months ago

  • Milestone set to Awaiting Review

#17 @peterwilsoncc
2 months ago

#47908 was marked as a duplicate.

Note: See TracTickets for help on using tickets.