Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

REST API: new endpoint to fetch post counts by post status #7773

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Nov 12, 2024

TODO

What?

Syncs WordPress/gutenberg#67719

The PR proposes to extend WP_REST_Posts_Controller with a new route /count to fetch page counts, e.g., wp/v2/pages/count.

Why?

The need for a performant post counts check for the block editor. See: WordPress/gutenberg#65223 (comment)

Contrast this with $query->found_posts which returns the total count for a specific query only and would degrade performance given n posts.

How?

Via wp/v2/pages/count, the editor can fetch the total number of post counts.

This is done using wp_count_posts.

Testing Instructions

Examples to run in the browser console (in the editor):

await wp.apiFetch( { path: '/wp/v2/pages/count' } )

/*
{
    "publish": 1,
    "future": 0,
    "draft": 2,
    "pending": 0,
    "private": 0,
    "trash": 0,
    "auto-draft": 0
}
*/

Also add a custom post status and ensure it appears in the response:

/**
 * Create initial post types.
 */
function create_initial_post_status_test() {
	register_post_status( 'ridgy_didge', array( 'public' => true ) );
}

add_action( 'init', 'create_initial_post_status_test', 0 ); // Highest priority
await wp.apiFetch( { path: '/wp/v2/pages/count' } )

/*
{
    "publish": 1,
    "future": 0,
    "draft": 2,
    "pending": 0,
    "private": 0,
    "trash": 0,
    "ridgy_didge": 0,
    "auto-draft": 0
}
*/

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


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

@ramonjd ramonjd force-pushed the add/rest-api-posts-counts-controller branch from 202b1e6 to 95db722 Compare November 12, 2024 10:49
@ramonjd ramonjd self-assigned this Nov 12, 2024
@ramonjd ramonjd marked this pull request as ready for review November 12, 2024 11:29
Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props ramonopoly.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@ramonjd ramonjd force-pushed the add/rest-api-posts-counts-controller branch from 8ab940f to d67a6c1 Compare December 9, 2024 04:38
@ramonjd ramonjd marked this pull request as draft December 9, 2024 06:46
@ramonjd ramonjd force-pushed the add/rest-api-posts-counts-controller branch from 24e7570 to b27145f Compare December 9, 2024 07:06
Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new endpoint needs tests.

Comment on lines +3374 to +3392
return array(
'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => 'post-counts',
'type' => 'object',
/*
* Use a pattern matcher for post status keys.
* This allows for custom post statuses to be included,
* which can be registered after the schema is generated.
*/
'patternProperties' => array(
'^\w+$' => array(
'description' => __( 'The number of posts for a given status.' ),
'type' => 'integer',
'context' => array( 'view', 'edit', 'embed' ),
'readonly' => true,
),
),
'additionalProperties' => false,
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return array(
'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => 'post-counts',
'type' => 'object',
/*
* Use a pattern matcher for post status keys.
* This allows for custom post statuses to be included,
* which can be registered after the schema is generated.
*/
'patternProperties' => array(
'^\w+$' => array(
'description' => __( 'The number of posts for a given status.' ),
'type' => 'integer',
'context' => array( 'view', 'edit', 'embed' ),
'readonly' => true,
),
),
'additionalProperties' => false,
);
$schema = array(
'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => 'post-counts',
'type' => 'object',
'properties' => [];
);
$post_stati = get_post_stati( array( 'internal' => false ) );
if ( get_post_status_object( 'trash' ) ) {
$post_stati[] = 'trash';
}
foreach( $post_stati as $post_status ) {
$schema['properties'] = array(
'description' => __( 'The number of posts for a given status.' ),
'type' => 'integer',
'context' => array( 'view', 'edit', 'embed' ),
'readonly' => true,
);
}
return $schema;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using get_post_stati was the original implementation WordPress/gutenberg@3895345 (#66294)

We can use it, but the caveat is that all custom post statuses must be registered at the highest priority, otherwise the endpoint will not return them.

A comment here is probably enough for now.

Comment on lines +3374 to +3393
return array(
'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => 'post-counts',
'type' => 'object',
/*
* Use a pattern matcher for post status keys.
* This allows for custom post statuses to be included,
* which can be registered after the schema is generated.
*/
'patternProperties' => array(
'^\w+$' => array(
'description' => __( 'The number of posts for a given status.' ),
'type' => 'integer',
'context' => array( 'view', 'edit', 'embed' ),
'readonly' => true,
),
),
'additionalProperties' => false,
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return array(
'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => 'post-counts',
'type' => 'object',
/*
* Use a pattern matcher for post status keys.
* This allows for custom post statuses to be included,
* which can be registered after the schema is generated.
*/
'patternProperties' => array(
'^\w+$' => array(
'description' => __( 'The number of posts for a given status.' ),
'type' => 'integer',
'context' => array( 'view', 'edit', 'embed' ),
'readonly' => true,
),
),
'additionalProperties' => false,
);
}
if ( $this->schema ) {
return $this->add_additional_fields_schema( $this->schema );
}
$schema = array(
'$schema' => 'http://json-schema.org/draft-04/schema#',
'title' => 'post-counts',
'type' => 'object',
/*
* Use a pattern matcher for post status keys.
* This allows for custom post statuses to be included,
* which can be registered after the schema is generated.
*/
'patternProperties' => array(
'^\w+$' => array(
'description' => __( 'The number of posts for a given status.' ),
'type' => 'integer',
'context' => array( 'view', 'edit', 'embed' ),
'readonly' => true,
),
),
'additionalProperties' => false,
);
$this->schema = $schema;
return $this->add_additional_fields_schema( $this->schema );
}

It is important to add the add fields and this caching.

*/
public function get_count_permissions_check() {
$post_type = get_post_type_object( $this->post_type );

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( ! $this->check_is_post_type_allowed( $post_type ) ) {
return false;
}

$counts = wp_count_posts( $this->post_type );
$data = array();

if ( ! empty( $counts ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is empty, I think this should return a 404 error ( wp_error here ).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about that for the following reasons:

  • The wp_count_posts function always returns an stdClass object, even when no posts exist, so an empty count is still a valid count result.
  • A 404 status code only indicates that the resource is missing. The resource exists, it performs a count, but returns zero values.
  • The endpoint should maintain consistent response structure regardless of count values.

Unless I'm missing something in wp_count_posts, 200 OK, therefore, is the most appropriate response code for this case as it's still a structured response.

Comment on lines +3320 to +3323
public function get_count() {
$counts = wp_count_posts( $this->post_type );
$data = array();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function get_count() {
$counts = wp_count_posts( $this->post_type );
$data = array();
public function get_count( $request ) {
$counts = wp_count_posts( $this->post_type );
$data = array();
$fields = $this->get_fields_for_response( $request );

}
// Include all public statuses in the response if there is a count.
foreach ( $post_stati as $status ) {
if ( isset( $counts->$status ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if ( isset( $counts->$status ) ) {
if ( rest_is_field_included( $status, $fields ) ) && isset( $counts->$status ) ) {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above suggestion had an extra parenthesis:

if ( rest_is_field_included( $status, $fields ) ) && isset( $counts->$status ) ) {

As I understand it, this will check against $this->get_item_schema() so the fields won't exist.

/counts requires a unique schema.

I'm finding the problem is that the schema callback when registering a route is never fired, unless it's an OPTIONS request (the context is 'help'). So I'm not sure the REST API is set up to allow a response to have two separate schemas.

I guess this is a good thing.

It's part of the reason why I chose to add a separate endpoint in the first place WordPress/gutenberg#66294

There is the option to add a counts field to the existing schema. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is the option to add a counts field to the existing schema

Integrating it into the current item schema isn't appropriate as it's not an item, it's related to the collection

@ramonjd
Copy link
Member Author

ramonjd commented Dec 9, 2024

I appreciate the quick review and suggestions.

This new endpoint needs tests.

I am aware. I had already written a prominent TODO in the description to add them.

@ramonjd
Copy link
Member Author

ramonjd commented Dec 10, 2024

Thanks for helping me think this over @spacedmonkey

Given everything I've tested I have a mind to revert back to https://github.com/WordPress/wordpress-develop/blob/95db722a2f73fd238312ed026f442c88363b3cd6/src/wp-includes/rest-api/endpoints/class-wp-rest-post-counts-controller.php, which introduced an embeddable, separate endpoint with a dedicated schema.

So far the only arguments to extend WP_REST_Posts_Controller has been because it seems like the natural thing to do.

Is it worth trying add a custom schema for the /counts route?

'callback' => array( $this, 'get_count' ),
'permission_callback' => array( $this, 'get_count_permissions_check' ),
),
'schema' => array( $this, 'get_count_schema' ),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does absolutely nothing in GET responses. It's only available in the 'help' context, which is used for OPTIONS requests.

@TimothyBJacobs do you know if it's advisable, or even possible, to have a custom schema for a route here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants