Make WordPress Core

Opened 5 years ago

Last modified 8 months ago

#47642 reviewing defect (bug)

Order by comment count - posts list tables

Reported by: alektabor's profile alektabor Owned by: johnbillion's profile johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Posts, Post Types Keywords: has-screenshots has-patch early early-like-actually-early has-unit-tests
Focuses: administration Cc:

Description

Results from posts list tables with enabled ordering by comment count are mixed (posts are missing and aren't unique). Pagination required.
Links:
wp-admin/edit.php?orderby=comment_count&order=asc
wp-admin/edit.php?orderby=comment_count&order=asc&paged=2
No matter which post type.
You need to set 'Number of items per page' in screen options in order to get a pagination.
https://i.imgur.com/HupwtnS.png
Set sorting by comment count in comments column.
https://i.imgur.com/zFDU7Oj.png
In lists of posts some of them are missing. Page number 1 and number 2 don't have unique posts.
I was testing it for some cases:
30+ posts and 'posts_per_page' = 20
30+ posts and 'posts_per_page' = 10
250+ posts and 'posts_per_page' = 20
24 pages and 'posts_per_page' = 20
Results in all cases were wrong.

The official queries are from wp-includes/class-wp-query.php (line: 2984) https://github.com/WordPress/WordPress/blob/master/wp-includes/class-wp-query.php#L2984, variable: $this->request. Below applied filter are generated ids for current posts.

https://i.imgur.com/tWevfpW.png

SQL queries from debugging (official queries):

Page number 1 (paged=1):

SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  WHERE 1=1  AND wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'future' OR wp_posts.post_status = 'draft' OR wp_posts.post_status = 'pending' OR wp_posts.post_status = 'private')  ORDER BY wp_posts.comment_count ASC LIMIT 0, 20

Result:
https://i.imgur.com/rFyTHjE.png

Page number 1 (paged=2):

SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID FROM wp_posts  WHERE 1=1  AND wp_posts.post_type = 'post' AND (wp_posts.post_status = 'publish' OR wp_posts.post_status = 'future' OR wp_posts.post_status = 'draft' OR wp_posts.post_status = 'pending' OR wp_posts.post_status = 'private')  ORDER BY wp_posts.comment_count ASC LIMIT 20, 20

Result:
https://i.imgur.com/THMM1Ri.png

What's funny if I added 'wp_posts.post_title' to query the problem is solved (unique posts):
https://i.imgur.com/nxn3tCy.png
https://i.imgur.com/whvDFaI.png

I think that problem is actually in the database queries/ settings what's caused the bad output in admin's panel.

For other sorting like title or date this problem doesn't occur.

Attachments (6)

patch.47642.20190920.1 (777 bytes) - added by ramon fincken 4 years ago.
patch.47642.20190920.1
patch.47642.20190923.1 (766 bytes) - added by ramon fincken 4 years ago.
patch.47642.20190923.1
47642.diff (590 bytes) - added by johnbillion 3 years ago.
patch.47642.20201030.1 (1.4 KB) - added by ramon fincken 3 years ago.
patch.47642.20201030.1
47642.2.diff (1.6 KB) - added by johnbillion 3 years ago.
patch.47642.20220204.1 (1.5 KB) - added by ramon fincken 2 years ago.
patch.47642.20220204.1

Download all attachments as: .zip

Change History (57)

#1 @ramon fincken
4 years ago

alltough I see that your ID resultset in mysql has duplicates I cannot reproduce this using:

posts with 0, 1, 2, 3, 3, 4 comments
paginated 2

I do not have overlapping or missing posts in the list.

Still this might me an error because of the post_status. Could you run the queries and do them only for the publish post status?

#2 @ramon fincken
4 years ago

OK, reproduced this .. it has to do with the fact that te results are ordered by a column that has the very same information. Thus is *might* be possible that de database engine spits the results to PHP in a (lets call it) *random* order. Thanks @herregroen for pointing that out

#3 @ramon fincken
4 years ago

so .. here is the fix. We need an extra unique (!) colum to sort by AFTER the comment count has been sorted. I used post_modified in this case.

@ramon fincken
4 years ago

patch.47642.20190920.1

#4 @ramon fincken
4 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#5 @ramon fincken
4 years ago

for the record: tested with pagination of 20 using 23 posts ( all had no comments )

#6 @johnbillion
4 years ago

Thanks for the patch @ramon-fincken. The post_modified field is also non-unique. It's possible that two posts could share the same comment count and modified date.

The secondary sort order should probably be the post ID instead.

#7 @ramon fincken
4 years ago

yes that would be totally unique. See attached new patch

@ramon fincken
4 years ago

patch.47642.20190923.1

#8 @ramon fincken
4 years ago

Hi @johnbillion any chance of checking the last patch?

#9 @JavierCasares
4 years ago

It works for me... also, to maintain consistency, should we check the same thing for the "modified" (default) option?

So, if we order and modified two posts at the same exact time... we may have the same problem, so it should be and option to order by the modified and ID field.

#10 @ramon fincken
4 years ago

Chances are that you collide with 2 very same (second) updates are less than having posts with zero comments if you ask me.

#11 @ramon fincken
4 years ago

If anything more needs to be done @johnbillion please let me know

#12 @johnbillion
4 years ago

  • Milestone changed from Awaiting Review to 5.6
  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Version 5.2.2 deleted

@johnbillion
3 years ago

#13 @johnbillion
3 years ago

  • Keywords needs-unit-tests added; needs-testing removed

Thanks for the patch @ramon-fincken . I think this adjustment needs to be made at a lower level, inside the WP_Query class. That way, any code that's ordering posts by comment count will have the order corrected, not just queries for that screen in the admin area.

47642.diff makes that change.

I'm working on some tests for this now.

In addition, this problem affects more than just the comment_count ordering, the main one being ordering by date. Let's tackle this first, then look at dates in a separate ticket.

#14 @ramon fincken
3 years ago

This covers more than just the admin posts, nice find :)

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


3 years ago

#16 @helen
3 years ago

  • Keywords early added
  • Milestone changed from 5.6 to 5.7

Punting from 5.6 and marking for 5.7-early, this is one of those things that is incredibly annoying and weird for users but we have to be quite careful with changes to this area.

Related: #44349

In particular:

Would there be a case when the orderby should not have ID?

I don't believe so, no. This change does have potential to break custom filters that are doing string replacements on the resulting SQL though.

#17 @SergeyBiryukov
3 years ago

#23923 / #26042 might also be related.

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


3 years ago

#19 @hellofromTonya
3 years ago

Per core scrub discussion today, Helen notes:

Should probably lump those together and see if somebody is interested in targeting a 5.7-early fix

Related: #42936, #44349, #23923, #26042

#20 @ramon fincken
3 years ago

here we go .. this patch tests for numeric values and datetime values.
order can happen on string OR array , so we need to test for both of them.

Also .. if it is an array, we do NOT need to add the ID if it is already present

see patch: patch.47642.20201030.1

@ramon fincken
3 years ago

patch.47642.20201030.1

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


3 years ago

#22 @johnbillion
3 years ago

Thanks for the updated patch Ramon, I'll take a look at this once work starts on 5.7.

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


3 years ago

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


3 years ago

#25 @ramon fincken
3 years ago

Any luck @johnbillion ?

Let me know if you need more info from my side.

@johnbillion
3 years ago

#26 @johnbillion
3 years ago

  • Keywords early-like-actually-early added
  • Milestone changed from 5.7 to 5.8

47642.2.diff is a start at writing some failing tests for this scenario. I'll work on more before 5.8 dev starts.

To run just these tests:

composer test -- --filter Tests_Query_Orderby

#27 @lukecarbis
3 years ago

@johnbillion Checking in to see if this has crossed your mind again?

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


3 years ago

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


3 years ago

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


3 years ago

#31 @gwwar
3 years ago

Another common one is the menu_order case. I think we want a secondary ID sort for all orderby values except for 'relevance' (which defers to any other orderby value if it's specified).

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


3 years ago

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


3 years ago

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


3 years ago

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


3 years ago

#36 @ramon fincken
3 years ago

@gwwar menu_order does not have to be unique so I would not recommend using that

#37 @msea55
3 years ago

Hello people working on this ticket,

I recently came across this issue in a slightly different context . I see that the patch submitted by @ramon-fincken only looks for specific order criteria which are likely to produce duplicates. It is my opinion that the fix of adding an ID sort is useful no matter the order criteria passed in. For example, while post_title is unlikely to produce duplicates, it is still possible, and I see no harm in adding ID to that as well, similarly @gwwar mentioned the case of menu_order which also may not be unique.

The context in which I came across this issue https://core.trac.wordpress.org/ticket/53201 was in the admin ui on the page that displays posts. On this page, if no sort is selected, a sort of "post_date" is assigned on what is here line 2328. This is inside the "if" block of if ( empty( $q['orderby'] ) ), for which @ramon-fincken 's patch edits the else block. Which is to say, the patch does not work for my case because it is too restrictive in where it applies.

I think adding ID is useful no matter what the passed in criteria, which is why I propose making the change a bit further down on what is here line 2393, after the order params, default order, and relevance order are all accounted for but before the filters at the end of the query, by adding the line $orderby = $orderby ? $orderby . ', ID' : ' ID';. I don't know how to attach a diff like you guys did but it'd look like:

...
                        if ( $search_orderby ) {
                                $orderby = $orderby ? $search_orderby . ', ' . $orderby : $search_orderby;
                        }
                }

+               //Add additional sort by order to break ties consistently for pagination https://core.trac.wordpress.org/ticket/47642
+               $orderby = $orderby ? $orderby . ', ID' : ' ID';

                if ( is_array( $post_type ) && count( $post_type ) > 1 ) {
                        $post_type_cap = 'multiple_post_type';
                } else {
...

This will resolve the issue for all sort orders except rand. For rand this addition doesn't hurt, but it also doesn't prevent this issue.

Last edited 3 years ago by msea55 (previous) (diff)

#38 @hellofromTonya
3 years ago

  • Milestone changed from 5.8 to 5.9

Today is 5.8 Beta 1. As tests are in progress and this is marked early (like really early), punting to 5.9.

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


3 years ago

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


2 years ago

#41 @ramon fincken
2 years ago

@msea55 sorry I read your reply only today with more care than before when you typed it

So .. yes always adding ID (if not present in the original query) would be better and perhaps even speed up the PHP thread compared to my original patch.

I will take a look at your suggestion.

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


2 years ago

This ticket was mentioned in Slack in #core-test by hellofromtonya. View the logs.


2 years ago

This ticket was mentioned in Slack in #core-test by costdev. View the logs.


2 years ago

This ticket was mentioned in PR #1762 on WordPress/wordpress-develop by pbearne.


2 years ago
#45

  • Keywords has-unit-tests added; needs-unit-tests removed

refreshed combined patch
Added .ID to the sorts for all

Trac ticket: https://core.trac.wordpress.org/attachment/ticket/47642

#46 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to Future Release

Query changes by design need careful consideration and plenty of soak time to ensure nothing breaks. With 5.9 feature freeze ~2 days away, it's too late in the release cycle for a change to query. Punting this out of 5.9. As the next milestone is not yet available, moving to Future Release. Once available, feel free to move into the next release.

#47 @ramon fincken
2 years ago

So .. I have rewritten this. Resulting in less lines of code and better maintainable.

The basic principle is that every query that actually HAS an order by request argument .. should have an ID.

We have 3 cases where we do not add an ID: ID, rand and relevance. All others will have an ID added by this code.

Unlike the loop in the last patches this will do a quick array_intersect.

@ramon fincken
2 years ago

patch.47642.20220204.1

#48 @msea55
2 years ago

@ramon-fincken , thanks for looking into this and taking my suggestion!

Looking at the patch you uploaded, it looks like it slots in on what is now line L2370 in master. The particular issue I came across (the automatic display of posts by post types in the admin ui) actually came up with the behavior that if no sort was provided, "date" became the default sort. At the time I said it was line 2328, though I think now it's line 2348 or 2383, I'm guessing the former but I don't recall the details. If your patch slots in where I think it does, it may not account for my case. The one on 2348 won't get the secondary sort. And the one on 2883 will get the secondary sort but won't get the intended default sort on date. I'd probably make the change on what is now line 2389 to cover more cases.

Anyway, I found a work around for my case, but figured I'd lend my two cents in case it helps. Hope I'm not inserting myself too much and thanks.

#49 @ramon fincken
18 months ago

How to proceed @johnbillion ?

#50 @juhise
12 months ago

#57914 was marked as a duplicate.

#51 @ramon fincken
8 months ago

@johnbillion what is best to proceed on this one?

Note: See TracTickets for help on using tickets.