WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 8 months ago

#46294 new defect (bug)

wp rest api fails to paginate page requests correctly when ordering on menu_order

Reported by: hobzhobz Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.4.1
Component: Query Keywords: needs-patch
Focuses: rest-api Cc:

Description

Describe the bug
Gutenberg and wordpress fails to render the parent page dropdown correctly when there are more than 100 pages and the pages have various menu_order values set.

The problem is that gutenberg calls the wordpress rest api like this, getting 100 items at a time.

http://server/wp-json/wp/v2/pages?per_page=100&exclude=890&parent_exclude=890&orderby=menu_order&order=asc&context=edit&_locale=user
http://server/wp-json/wp/v2/pages?per_page=100&exclude%5B0%5D=890&parent_exclude%5B0%5D=890&orderby=menu_order&order=asc&context=edit&_locale=user&page=2

This results in the following 2 sql queries:
SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.ID NOT IN (890) AND wp_posts.post_parent NOT IN (890) AND wp_posts.post_type = 'page' AND ((wp_posts.post_status = 'publish')) ORDER BY wp_posts.menu_order ASC LIMIT 0, 100
SELECT SQL_CALC_FOUND_ROWS wp_posts.ID FROM wp_posts WHERE 1=1 AND wp_posts.ID NOT IN (890) AND wp_posts.post_parent NOT IN (890) AND wp_posts.post_type = 'page' AND ((wp_posts.post_status = 'publish')) ORDER BY wp_posts.menu_order ASC LIMIT 100, 100

When ordering on a non unique field like menu_order you need to also add a second unique ordering field like ID See https://bugs.mysql.com/bug.php?id=72076 See the answer from Tor Didriksen.

This is very critical bug because it means that if the current pages parent is missing in the parent page dropdown and you click update it means that the menu order for that page will be reset and this means i cant use gutenberg.

To Reproduce
Steps to reproduce the behavior:

  1. Create around 200 pages.
  2. Set menu_order != on some of the pages.
  3. edit any of the pages.
  4. the parent page dropdown is missing pages

Expected behavior
the rest api should add some unique order by field to prevent the same items from coming on page 1 and 2 of the requests.

Change History (22)

#1 @mukesh27
3 years ago

  • Component changed from General to REST API
  • Focuses rest-api added

#2 @hobzhobz
3 years ago

  • Component changed from REST API to General
  • Focuses rest-api removed

The old hack no longer works due to a new bug.

Here is a new workaround: https://core.trac.wordpress.org/ticket/52626#comment:4

This is the old hack, it no longer works, but it still demonstrates the issue with menu_order bug:

In the file /wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php:290

around line 290.
change from:

$posts_query  = new WP_Query();
$query_result = $posts_query->query( $query_args );

to

$posts_query  = new WP_Query();
if (isset($query_args['orderby']) && is_string($query_args['orderby']) && $query_args['orderby'] == 'menu_order') {
    $query_args['orderby'] = ['menu_order' => 'ASC', 'ID' => 'ASC'];
}
$query_result = $posts_query->query( $query_args );

now the query will be deterministic between pages.

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

#3 @hobzhobz
3 years ago

Note, this is hard to reproduce unless you set menu_order != 0 on many pages.

If menu_order == 0 on all pages this issue doesnt seem to happen, but i bet it has to do with mysql internals as its "undefined behavior" how mysql sorts when sorting only on a non unique field.

#4 @swissspidy
3 years ago

  • Component changed from General to REST API

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


2 years ago

#6 follow-up: @TimothyBlynJacobs
2 years ago

  • Component changed from REST API to Query
  • Focuses rest-api added

This seems like a bug in the query component. I’d think paginating over a new WP_Query([ 'orderby' => 'menu_order' ]) should work as expected.

#7 in reply to: ↑ 6 @hobzhobz
2 years ago

Replying to TimothyBlynJacobs:

This seems like a bug in the query component. I’d think paginating over a new WP_Query([ 'orderby' => 'menu_order' ]) should work as expected.

I agree. Wp_query should automatically append order by id asc or something when the original order by clause contains only non-unique fields

#8 follow-up: @danpw
2 years ago

Good afternoon, any updates on this issue? We're noticing it on some of our client sites as well.

#9 in reply to: ↑ 8 @secretsurfer
2 years ago

Replying to danpw:

Good afternoon, any updates on this issue? We're noticing it on some of our client sites as well.

I see this issue in one of my WooCommerce sites, too. I realized that it fails when activating Barn2Media Product Table or WooCommerce Blocks (from Automattic!). I think that the problem appeared since WP 5.3.

Any help?

Last edited 2 years ago by secretsurfer (previous) (diff)

#10 @hobzhobz
2 years ago

Bump, this issue is still live and still easy to fix and still causing problems for ALL sites with gutenberg and more than 100 pages.

Last edited 2 years ago by hobzhobz (previous) (diff)

#11 follow-up: @jdorner
19 months ago

  • Severity changed from normal to major
  • Version set to 5.4.1

I'm experiencing problems as well. Is there a fix for this issue?

#12 in reply to: ↑ 11 ; follow-up: @hobzhobz
19 months ago

  • Severity changed from major to critical

Replying to jdorner:

I'm experiencing problems as well. Is there a fix for this issue?

There's a hacky patch in reply #2 .

You need to apply it after every WordPress upgrade.

Version 0, edited 19 months ago by hobzhobz (next)

#13 in reply to: ↑ 12 @photosjob
18 months ago

Experiencing the same issue on a staging site with a CPT and 180+ articles.
Applied the hacky patch above for now (as more content needs to be added). Would be nice if a permanent solution could be applied soon...

Thanks in advance!!

Kind regards,
Job

Replying to hobzhobz:

Replying to jdorner:

I'm experiencing problems as well. Is there a fix for this issue?

There's a hacky patch in reply no2.

You need to apply it after every WordPress upgrade.

#14 @Colter
17 months ago

I would like to confirm this is still an issue on New Installs and Upgraded Sites.

#15 @emrikol
16 months ago

IF the patch in reply 2 works for you, I believe you can do it in a filter, rather than hacking core after every update:

<?php
add_filter( 'rest_page_query', function( $args, $request ) {
        if (
                        isset( $args['orderby'] ) &&
                        is_string( $args['orderby'] ) &&
                        'menu_order' === $args['orderby']
        ) {
                $args['orderby'] = [ 'menu_order' => 'ASC', 'ID' => 'ASC' ];
        }
        return $args;
}, 10, 2 );

In my testing when I ran into this issue, I also needed to bump up posts_per_page as well since it was still very low compared to the number of pages on the site experiencing the issue.

#16 follow-up: @andreasgregor
15 months ago

I'm still having this issue on a 5.5.1 site with 200+ pages.

The patch in reply 2 is not working for me, neither is the filter variant.
Any other ideas?

#17 @swissspidy
14 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release
  • Severity changed from critical to normal

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


12 months ago

#19 in reply to: ↑ 16 @hobzhobz
9 months ago

Replying to andreasgregor:

I'm still having this issue on a 5.5.1 site with 200+ pages.

The patch in reply 2 is not working for me, neither is the filter variant.
Any other ideas?

The patch has indeed stopped working. For some reason we only get 100 pages now.. Which means the javascript cannot build the tree correctly and produces bad results.

Bumping posts_per_page doesn't work for me. Yes, the ajax response contains all the pages, but the parent page select tree does still not get built correctly.

I have created a new ticket: https://core.trac.wordpress.org/ticket/52626/

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

#20 @hobzhobz
9 months ago

There's a new workaround/hack/patch here: https://core.trac.wordpress.org/ticket/52626#comment:4

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


8 months ago

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


8 months ago

Note: See TracTickets for help on using tickets.