Make WordPress Core

Opened 3 months ago

Last modified 3 weeks ago

#62701 new enhancement

Post controller: support for grouping results by parent-child relationship

Reported by: oandregal's profile oandregal Owned by:
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: REST API Keywords: has-patch has-unit-tests gutenberg-merge
Focuses: Cc:

Description

The current wp-admin displays hierarchical posts grouped/sorted by hierarchy. To implement the same behaviour in the site editor, we need to the REST API to support the same functionality.

See details in the related issues/PRs:

Change History (9)

This ticket was mentioned in PR #8014 on WordPress/wordpress-develop by @oandregal.


3 months ago
#1

  • Keywords has-unit-tests added

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

See related Gutenberg issues/PRs:

## 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
https://github.com/user-attachments/assets/6facad5b-acad-4671-a166-9ececeba5ccc https://github.com/user-attachments/assets/81f94e96-12c1-49f5-8c05-664f4ae27ddd

## How to test

Added a new test suite to test the logic. Without the related Gutenberg PR this is difficult to test.

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


7 weeks ago

@oandregal commented on PR #8014:


7 weeks ago
#3

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 commented on PR #8014:


7 weeks ago
#4

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 commented on PR #8014:


7 weeks ago
#5

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 we had the following dataset and each page sent to the client was of 5 items:

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

The levels of the items in the first page (1 to 5) would be calculated properly. For calculating the levels of the items in the second page (6 to 10) we wouldn't have any information about their ancestors.

#6 @joemcgill
4 weeks ago

  • Keywords gutenberg-merge added

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


3 weeks ago

#8 @audrasjb
3 weeks ago

  • Keywords changed from has-patch, has-unit-tests, gutenberg-merge to has-patch has-unit-tests gutenberg-merge

There are several change requests in the PR, so I'd be inclined to punt it to 6.9, but it was actually merge on Gutenberg' side (in version 20.0), so I assume we'll need to merge it in 6.8? Or maybe not?

What do you think @joemcgill @oandregal?

#9 @audrasjb
3 weeks ago

  • Milestone changed from 6.8 to 6.9

As per today's bug scrub: It appears this ticket still needs some work. As 6.8 beta 1 is very close, I'm moving it to 6.9. Feel free to move it back to 6.8 if it can be committed by Monday.

Note: See TracTickets for help on using tickets.