Make WordPress Core

Opened 2 years ago

Closed 7 months ago

Last modified 7 months ago

#56841 closed defect (bug) (fixed)

WordPress pagination broken after updating to 6.0

Reported by: wpfed's profile wpfed Owned by: swissspidy's profile swissspidy
Milestone: 6.5 Priority: normal
Severity: normal Version: 6.0
Component: Query Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

The pagination is broken in the admin and front-end.

In the admin it's returning incorrect amount of items (E.g., 10 posts are visible in the admin table but pagination is missing and shows there is 4 results only(4 items) but I have over 300 posts. The buttons/input to navigate are missing.

This only appears to happen with posts, pages and other list items are displaying correct results with pagination present.

I tried switching themes, deactivating all plugins, resetting permalinks, repaired the DB with WP auto repair but still no pagination was present.

Rolling back to core 5.9.4 restored pagination.

Updating to latest version 6.0.2 also did not help.

Server setup:
nginx/1.19.10
mysqli 5.7.18-15-57-log
PHP version 7.4.23 (Supports 64bit values)

I'm waiting for my host to update mysql, but it may be a while. I'm in the process of updating PHP to a higher version to see if that could help.

Also I took a deeper dive into the DB, saw that it's using latin1 charset(it's a older site), ENGINE=InnoDB and SET sql_mode='NO_AUTO_VALUE_ON_ZERO'; but nothing else of significance.

Also checked the error debug log and found this but not sure if that would have a impact. [05-Oct-2022 17:38:12 UTC] WordPress database error Incorrect string value: '\xEF\xBB\xBF</p...' for column 'post_content' at row 1 for query UPDATE wp_posts...

Any ideas what is causing this?

Attachments (1)

pagination-broken.png (60.6 KB) - added by wpfed 2 years ago.

Download all attachments as: .zip

Change History (43)

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


2 years ago

#2 follow-up: @cu121
2 years ago

  • Keywords has-screenshots needs-testing-info added

#3 in reply to: ↑ 2 @cu121
2 years ago

Thank you @wpfed, for the report! We really appreciate the time you have taken to report this issue. However, the mentioned issue seems very unique according to your local environment. I would request you kindly provide more required details about your local environment so that we can replicate your issue properly from our end.

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

#4 @ironprogrammer
2 years ago

  • Keywords reporter-feedback added

Hi, @wpfed -- Would you please provide reproduction information for this issue? The screenshot supplied suggests you may have reproduced this locally, but to be sure, sharing those steps with other reviewers will help this get more attention 🙌🏻

Please see the following link for guidance on Reproduction Testing instructions: https://make.wordpress.org/test/handbook/test-reports/#bug-defect-testing-instructions-template.

#5 @wpfed
2 years ago

Thank you all for getting back!

I wanted to add some more information in the meantime.

As mentioned by @cu121 this is very much looking like hosting environment issue since it's working on my localhost but not on my hosted server.

Another factor that is pointing to my hosting environment, I checked on another DEV site that is updated to 6.0.2 and the pagination is also missing! but once again, it's present on my localhost.

If I rollback to 5.9.4 the pagination returns. Something is not agreeing with WP 6.0+ and my hosting.

I have ticket open with my hosting company, I will post the solution once they figure it out. Thank you for your time, this can be closed.

#6 @ironprogrammer
2 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

Thank you for the update, @wpfed -- we'll close this for now.

You might also try checking out the dedicated support forums at https://wordpress.org/support/forums/. Good luck!

#7 @tadamarketing
15 months ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

We have similar problem. It is connected to changes in requests https://core.trac.wordpress.org/changeset/52973
After adding multiline strings - there are white signs in requests like \n\t\t\t which should be cleaned.

Last edited 15 months ago by tadamarketing (previous) (diff)

#8 follow-up: @wpfed
15 months ago

@tadamarketing @ironprogrammer

My managed hosting company has still not been able to resolve this issue, any site I updated from pre-6.0 runs into the same issue of pagination breaking, no matter what themes or plugins I disable. But as I mentioned before it only happens on my hosting company server, which is running NGINX and older version of mySQL. On my local I am running apache, mysql 5.7.40.

Since I opened the tickets, a few items were upgraded but it did not solve the issue:

Web server nginx/1.23.2
PHP version 7.4.33 (Supports 64bit values)

Mysql is still on older version:

Extension mysqli
Server version 5.7.18-15-57-log
Client version mysqlnd 7.4.33
Database host mysql_proxy
Table prefix wp_
Database charset utf8mb4
Database collation utf8mb4_unicode_ci
Max allowed packet size 67108864
Max connections number 1000

@tadamarketing which versions of everything are you running on?

#9 in reply to: ↑ 8 @tadamarketing
15 months ago

@wpfed @ironprogrammer

nginx: 1.25.1
PHP: 7.4.33
WP: 6.22
MariaDB: 10.4.30-MariaDB-log
Galera (wsrep provider version): 26.4.14(r06a0c285)
ProxySQL: 2.5.2-217-g7f727b3

Communication is going from WP (PHP) through ProxySQL to one of three MariaDB servers.

Can you try to run SQL query on your DB and share the result?
SHOW GLOBAL STATUS LIKE 'wsrep_provider_version';

Do you now if your hosting provider gives you direct connection to DB or is proxied through Load Balancer such as HAProxy, ProxySQL?

We diagnosed that the problem lies in that commit: https://github.com/WordPress/WordPress/commit/6f5f1875749a4425b3133ecddcc9366b432d06aa
The change is maybe better for readability but quite annoying for database load balancer which cannot match configured rules to incoming queries properly.
We also cannot trim white characters from queries.

Last edited 15 months ago by tadamarketing (previous) (diff)

#10 @wpfed
15 months ago

@tadamarketing

wsrep_provider_version: 3.20(r7e383f7)

It is proxy with load balancing.

I'm glad you guys found the issue! Are you using a workaround to the problem?

So it seems to impact specific server setups with load balancing, there is probably going to be more reports to this as users get of the 5.9 branch.

We need WP core member to look at this, if configuration needs to be updated on specific servers or if patch is needed in core. I worked with system admins who are unable to resolve this.

#11 @wpfed
12 months ago

  • Severity changed from normal to major

This just happened to another one of my sites, pagination/page navigation is not working when upgrading to 6.0+.

We really need someone to look into this.

#12 @wpfed
12 months ago

I did a quick test, as mentioned by @tadamarketing, the commit below is causing the issue (white signs in requests like \n\t\t\t)

https://github.com/WordPress/WordPress/commit/6f5f1875749a4425b3133ecddcc9366b432d06aa

I was able to fix the issue by restoring to the previous code used in wp-includes/class-wp-query.php OR using trim:

$this->request = trim($old_request);

So that means I would have to manually edit core files so it works properly.

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


11 months ago

#14 follow-up: @swissspidy
11 months ago

  • Keywords needs-patch added; has-screenshots reporter-feedback removed
  • Milestone set to Future Release
  • Severity changed from major to normal

Using trim() sounds reasonable. Would you be interested in submitting a PR with this suggestion? Looks like it needs to be done in multiple places.

#15 in reply to: ↑ 14 @wpfed
11 months ago

Replying to swissspidy:

Using trim() sounds reasonable. Would you be interested in submitting a PR with this suggestion? Looks like it needs to be done in multiple places.

Yes!, thank you, appreciate you getting back on this. It does require multiple places, I only did it one spot currently to fix it for my use case. I will get that submitted asap.

#16 @afercia
11 months ago

  • Component changed from Posts, Post Types to Query
  • Focuses coding-standards added
  • Keywords needs-testing-info removed

Cc: @SergeyBiryukov @jrf
It appears https://github.com/WordPress/WordPress/commit/6f5f1875749a4425b3133ecddcc9366b432d06aa impacted specific server setups with load balancing, where an sql string that includes line breaks and tab characters can be problematic. I'm guessing especially with new lines and tabs at the beginning of the query string, as in:

"
					SELECT  ...

as opposed to previous strings:

"SELECT  ...

#17 @SergeyBiryukov
11 months ago

  • Milestone changed from Future Release to 6.5

#18 @wpfed
11 months ago

Thanks for confirming @afercia That is how the queries look on the server when I saved them. I saved the queries on my server, one with current implementation and one with trim() or spaces removed:

Without space removal:

<?php
                                        SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID
                                        FROM wp_posts  LEFT JOIN wp_term_relationships ON (wp_posts.ID = wp_term_relationships.object_id)  LEFT JOIN wp_term_relationships AS tt1 ON (wp_posts.ID = tt1.object_id)
                                        WHERE 1=1  AND ( 
  wp_term_relationships.term_taxonomy_id IN (5) 
  AND 

With space removal:

<?php
SELECT SQL_CALC_FOUND_ROWS  wp_posts.ID
                                        FROM wp_posts  LEFT JOIN wp_term_relationships ON (wp_posts.ID = wp_term_relationships.object_id)  LEFT JOIN wp_term_relationships AS tt1 ON (wp_posts.ID = tt1.object_id)
                                        WHERE 1=1  AND ( 
  wp_term_relationships.term_taxonomy_id IN (5) 
  AND 

trim() function doesn't even need to be used, just changing the formatting so it's more constent with the rest of the select queries:

<?php
$this->request = "
                                        SELECT $found_rows $distinct {$wpdb->posts}.ID
                                        FROM {$wpdb->posts} $join
                                        WHERE 1=1 $where
                                        $groupby
                                        $orderby
                                        $limits
                                ";

With new formatting:

<?php
$this->request = "SELECT $found_rows $distinct {$wpdb->posts}.ID
                                        FROM {$wpdb->posts} $join
                                        WHERE 1=1 $where
                                        $groupby
                                        $orderby
                                        $limits
                                ";

I'll try to work on this today but preference is to not use trim() function.

This ticket was mentioned in PR #5635 on WordPress/wordpress-develop by LukaszJaro.


11 months ago
#19

  • Keywords has-patch added; needs-patch removed

I removed the white space for any select query by formatting it to one line. This keeps it consistent with the other select queries which are also one line.

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

#21 @wpfed
11 months ago

Tested with WordPress Core PR previewer and found no issues with PR. https://playground.wordpress.net/wordpress.html

#22 @ironprogrammer
11 months ago

Thanks for the PR, @wpfed! This effectively reverts things to before [52973].

However, because both [52973] and the follow-up [52977] were updates to improve readability of the code, could there be a compromise here? Such as:

// Remove leading/trailing whitespace while avoiding trim().
$this->request =
        "{$this->sql_clauses['select']}
         {$this->sql_clauses['from']}
         ...
         {$this->sql_clauses['limits']}";

// ALTERNATIVE: Concat and remove extraneous `\n` and `\t` introduced by [52977].
$this->request =
        "{$this->sql_clauses['select']} " .
        "{$this->sql_clauses['from']} " .
        ...
        "{$this->sql_clauses['limits']}";

There is some precedence for multiline string concatenation in a few places, e.g.:

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


8 months ago

#24 @audrasjb
8 months ago

As per today's bugscrub: This ticket is still under discussion, let’s give it a week before moving it to Future release.

#25 @wpfed
8 months ago

Thanks @audrasjb I will look into this tomorrow, looks like @ironprogrammer has given alternative solution I have to look into.

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


8 months ago

#27 @rajinsharwar
8 months ago

Thanks @wpfed for taking charge! Let's keep this in the milestone for some more days, and hopefully get this landed in 6.5. Thanks again for your help.

#28 @wpfed
8 months ago

@audrasjb @rajinsharwar

Thank you for the patience! - I made the updates as per @ironprogrammer
https://github.com/WordPress/wordpress-develop/pull/5635

Ran it through playground and my own NGINX setup and it's working.

I hope it is OK and can be merged to 6.5 :)

@wpfed commented on PR #5635:


8 months ago
#29

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.

## Unlinked Accounts
The following contributors have not linked their GitHub and WordPress.org accounts: @LukaszJaro.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

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

Refresh Please!

@wpfed commented on PR #5635:


7 months ago
#30

Looks good, @LukaszJaro! I've left some suggestions below that maintain the readability intent from r52973.

Thanks @ironprogrammer for getting back, I did not get this email notification unfortunately(email sync issue), I just committed the changes.

#31 @wpfed
7 months ago

@ironprogrammer I had some email issues, but I did just make the changes now.

#32 @ironprogrammer
7 months ago

  • Keywords needs-testing added

Before marking this for 6.5 commit consideration, additional testing would be helpful (in addition to comment:28) to verify this fix functions as intended. CC @tadamarketing, possibly @afercia.

#33 follow-up: @swissspidy
7 months ago

Speaking of testing, is it possible to cover this change with unit tests, e.g. verifying that the resulting queries don't have any leading/trailing whitespace?

#34 in reply to: ↑ 33 @afercia
7 months ago

Replying to swissspidy:

Speaking of testing, is it possible to cover this change with unit tests

I'd agree it would be worth it to make sure this bug doesn't hapen again, maybe for some unintended code formatting change in the future.

Also, I'd consider to add some inline comments in the code with a reference to this ticket?

as per testing the patch: I'm not sure I can, as it requires specific server setups with load balancing.

#35 follow-up: @swissspidy
7 months ago

  • Keywords has-unit-tests added

I just added some unit tests to the existing PR at https://github.com/WordPress/wordpress-develop/pull/5635. That also helps as documentation of this issue to prevent regressions.

Note that due to how queries can be constructed, there might still be some trailing whitespace. Right now, only leading whitespace is eliminated and tested in unit tests.

So to clarify: is only the leading whitespace an issue, or also the trailing whitespace? For the latter we'd definitely need to use trim().

#36 in reply to: ↑ 35 @wpfed
7 months ago

Replying to swissspidy:

So to clarify: is only the leading whitespace an issue, or also the trailing whitespace? For the latter we'd definitely need to use trim().

Thank you @swissspidy!! I really appreciate those unit tests.

To confirm your question, the issue was leading white space. Trailing white space was cleaned up for consistency.

@ironprogrammer commented on PR #5635:


7 months ago
#37

As proposed by @afercia, should each of the updated strings include something along the lines of:

// Remove leading and trailing whitespace, but maintain readability.

#38 @swissspidy
7 months ago

  • Keywords commit added; needs-testing removed

#39 @swissspidy
7 months ago

  • Owner set to swissspidy
  • Status changed from reopened to accepted

#40 @swissspidy
7 months ago

  • Focuses coding-standards removed

#41 @swissspidy
7 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 57750:

Query: Remove leading whitespace from certain database queries.

Unintended leading whitespace at the beginning of a raw MySQL query led to unexpected behavior such as broken pagination. Eliminating said whitespace avoids that.

Adds unit tests to prevent regressions.

Props wpfed, swissspidy, ironprogrammer, tadamarketing, afercia.
Fixes #56841.

Note: See TracTickets for help on using tickets.