Add logic to sort posts by hierarchy

[oandregal]

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

See related Gutenberg issues/PRs:

• Issue Showcase hierarchy in table/list layout gutenberg#55957
• PR Proof of concept: visualize hierarchical data gutenberg#66479

What

This PR expands the post controller to allow returning the data by parent-child relationship (sort by hierarchy).

Why

We want to display data in a hierarchical way in the site editor, similarly to what we do in wp-admin:

wp-admin	site editor

How to test

Added a new test suite to test the logic.

Steps to test this manually:

(alternatively, you may want to generate a gutenberg zip from trunk, use the normal wordpress-develop setup and change the gutenberg's PHP code in the plugin directory)

• In this PR, execute npm install && npm run build:dev.
• Have a clone of the Gutenberg repository, switch to trunk, create a .wp-env.override.json with the following contents (core is the local path of this PR's repository, so change accordingly):

{
 "core": "../wordpress-develop/src"
}

• In the Gutenberg codebase, comment the 3 filters defined here.
• In the Gutenberg codebase, run npm run wp-env start. Then npm install && npm run dev.
• Visit localhost:8888, open the wp-admin, and go to "Tools > Import". Import this theme test data file, that comes with hierarchical pages (or create your own set of pages).
• Visit the Site Editor > Pages. Then switch to the table layout. You should see something like this:

[github-actions]

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 oandregal, mcsf, joemcgill, spacedmonkey.

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

[github-actions]

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.

[mcsf]

@oandregal: Correct me if I'm wrong, but this is the same problem that we had with static variables in the original Gutenberg PR. The concept of posts controller doesn't map 1:1 to the concept of a query, and so attaching the state of a query (levels) to a controller instance that can outlive it ($this) seems dangerous.

[joemcgill]

Modifying the query args with these params look similar to what is happening in the current WP Admin when hierarchical post types are queried without any order. See:

wordpress-develop/src/wp-admin/includes/post.php

Lines 1294 to 1300 in 9ad162e

	if ( is_post_type_hierarchical( $post_type ) && empty( $orderby ) ) {
	$query['orderby'] = 'menu_order title';
	$query['order'] = 'asc';
	$query['posts_per_page'] = -1;
	$query['posts_per_archive_page'] = -1;
	$query['fields'] = 'id=>parent';
	}

I suspect we might need to update the is_eligible() method with the same check for an empty orderby query. I also wonder if we need to continue supporting the menu_order property as the default sorting for these types of queries?

For sites with a lot of posts, doing a query that returns all posts, e.g. posts_per_page => -1 could be very slow, so there should be a way for this to be disabled.

[spacedmonkey]

+1

[spacedmonkey]

Added some thoughts. This is a good start but I think this PR needs work.

[spacedmonkey]

Why do any of the methods in this class need to be static? Static methods have a number of downside, including performance as static methods are not garbage collected in the same way that other methods are. For this one place where the class is used, we could just create an instance of the class and use public methods.

[spacedmonkey]

Why does this need to be another class? Why can't this be part of the WP_REST_Posts_Controller class?

[spacedmonkey]

+1

[spacedmonkey]

Could we not use get_post_parent or get_ancestors

[spacedmonkey]

PHP Docs required

[spacedmonkey]

Php docs required.

[spacedmonkey]

PHPdocs required.

[joemcgill]

Two additional questions I have about this PR after thinking about it:

1. Is there a reason this should be implemented as a new request param, rather than extending the existing orderby to handle this use case?
2. Is there a reason why it's important to do the sorting and include the levels property in the return value, rather than doing that sorting in the client based on the returned post IDs and parents? Returning a REST response shape solely for the purpose of mapping to the visual display in the UI seems like code smell to me unless that value is necessary for some other purpose.

[oandregal]

Why does this need to be another class? Why can't this be part of the WP_REST_Posts_Controller class?

I'm not married to this implementation, it's just an artifact of having been developed in Gutenberg first.

It was backported verbatim from there, where there's no WP_REST_Post_Controller class. I used the existing filters so the changes were isolated. Merging them as part of the controller class could work, having a single function that calculates it all is also an option, etc. — don't have a strong opinion. My only suggestion would be to 1) keep the changes centralized in a single place as opposed to scattered through the controller class and 2) keep them testable. This is an important domain piece that can degrade easily if it's goes unchecked.

[oandregal]

Is there a reason this should be implemented as a new request param, rather than extending the existing orderby to handle this use case?

orderby_hierarchy configures how orderby works: either normally or considering hierarchy. I didn't occur to me to overload orderby with this "new mode", and now that I think of it I don't see a clean way to do it either.

[oandregal]

Is there a reason why it's important to do the sorting and include the levels property in the return value, rather than doing that sorting in the client based on the returned post IDs and parents? Returning a REST response shape solely for the purpose of mapping to the visual display in the UI seems like code smell to me unless that value is necessary for some other purpose.

Levels need to be calculated absolutely (based on all the data) and not just relative to the page that is sent to the browser. Otherwise, we'll end up with the wrong levels.

For example, if this was the full dataset, and the size of each page sent to the client was 5 items:

1
- 2
-- 3
-- 4
- 5
-- 6
--- 7
--- 8
- 9
- 10

If the client only has access to the items of the page, the 1st batch (items 1 to 5) has all the information it needs to calculate the levels. The second batch only contains items 6 to 10, there's no any information about their ancestors, so it won't be able to calculate them properly.