WP_Query caching discards `posts_fields` and `posts_clauses['fields']` filters.

[peterwilsoncc]

When generating the cache key, WP_Query uses a consistent value for the fields requested in the SELECT clause.

In most cases, this is problem free as WP_Query uses the same cache regardless of the fields requested. When a plugin is filtering the fields returned, this can be problematic as the cache key could cause collisions in some circumstances.

When generating the cache key, WP_Query should consider the fields value if it has been modified by a plugin. This may also affect what needs to be cached.

I'm still investigating this so will add further notes to the ticket. I'm putting this on the 6.1.1 milestone for visibility.

[peterwilsoncc]

I've created a mini-plugin to modify the fields requested by WP_Query:

<?php
add_action( 'init', function() {
        add_filter( 'posts_fields', function( $fields ) {
                $fields .= ', 1 as test_post_fields';
                return $fields;
        } );

        add_filter( 'posts_clauses', function( $clauses ) {
                $clauses['fields'] .= ', 2 as test_post_clauses';
                return $clauses;
        } );

        $query2 = new WP_Query(
                array(
                        'post_type' => 'post',
                        'posts_per_page' => 1,
                        'fields' => 'id=>parent',
                        'cache_results' => false,
                )
        );

        var_dump( $query2->posts );

        exit;
}, 999 );

When initializing WP_Query( 'fields' => $value ) the following happens in 6.0:

• $value = 'ids': The posts property is an array if integers. The 6.1 behaviour matches.
• $value = 'all' /* or '' */: The posts property contains an array of WP_Post objects with the addition of test_post_fields and test_post_clauses. The 6.1 behaviour does not match.
• $vallue = 'id=>parent': The posts property contains an array of stdClass with the properties ID, post_parent, test_post_fields and test_post_clauses. The 6.1 behaviour does not match.

I see two options:

• bypass caching if the fields are not one of the three expected values WP_Query handles by default
• include the SQL fields in the cache key if the fields are not one of the three expected values

Either way, tests will need to be added to ensure changing the fields includes the additional values in the event the posts property is an object.

https://core.trac.wordpress.org/ticket/57012

First commit is some tests that pass on 6.0 but not on 6.1

[peterwilsoncc]

Props due @pypwalters, @claytoncollie, @johnwatkins0 for helping diagnose this issue.

[peterwilsoncc]

In the ​linked pull request:

• Added some tests to ensure modified SELECTs return the same values on the first and subsequent requests
• Bypass the main cache (ie, revert to 6.0 behavior) if the SELECT has been modified from an expected form

I considered caching modified SELECTs but figured some additional care would need to be taken to account for SELECTing many values that could blow up the size of the cached value.

This seemse like a sensible fix for a minor to me. 6.2 could explore generating a more comprehensive cache key.

Props due @costdev for reviewing names and tests via Slack DM.

[spacedmonkey]

Patch looks good to me. Adding commit, ahead of merge deadline in 6.1.1.

[peterwilsoncc]

In 54768:

Query: Bypass caching for filtered SELECTs.

Bypass caching within WP_Query when the SELECT clause has been modified via a filter. This prevents both cache key collisions and the returning of incomplete or unexpected results when the SELECT clause has been modified by an extender.

Props pypwalters, claytoncollie, johnwatkins0, TimothyBlynJacobs, costdev, spacedmonkey, peterwilsoncc.
Fixes #57012.

[peterwilsoncc]

Reopening merge for to the 6.1 branch.

Merged in https://core.trac.wordpress.org/changeset/54768

[SergeyBiryukov]

In 54777:

Coding Standards: Use consistent spelling for "cacheable" in WP_Query::get_posts().

Follow-up to [53941], [54768].

See #57012.

[desrosj]

In 54780:

Query: Bypass caching for filtered SELECTs.

Bypass caching within WP_Query when the SELECT clause has been modified via a filter. This prevents both cache key collisions and the returning of incomplete or unexpected results when the SELECT clause has been modified by an extender.

Props pypwalters, claytoncollie, johnwatkins0, TimothyBlynJacobs, costdev, spacedmonkey, peterwilsoncc.
Merges [54768] to the 6.1 branch.
Fixes #57012.

[desrosj]

In 54781:

Coding Standards: Use consistent spelling for "cacheable" in WP_Query::get_posts().

Follow-up to [53941], [54768].

Props SergeyBiryukov.
Merges [57012] to the 6.1 branch.
See #57012.

[SergeyBiryukov]

After [54768], seeing some test failures on MariaDB due to indeterminate sort order, see these test runs for example:

• ​greengeeksbot-r54824
• ​kdawsbot-r54824
• ​tuonettibot-r54824
• ​wpmudevbot-r54815

I can also reproduce these failures locally with MariaDB 10.6.8.

57012.diff​ aims to stabilize the tests by using assertEqualSets() instead of assertEquals(), since testing the order is out of their scope. Includes removing array_unshift() and array_reverse() calls as no longer needed.

[peterwilsoncc]

Thanks @SergeyBiryukov, the patch looks good based on a visual review.

As an educated guess, this is caused by hitting undefined behavior due to create_many creating several posts with the same date/time fields.

WP_Query is generating the DB Query below and each engine refines the order further using a different index.

SELECT SQL_CALC_FOUND_ROWS  wptests_posts.ID, wptests_posts.post_parent
FROM wptests_posts 
WHERE 1=1
        AND ((wptests_posts.post_type = 'wptests_pt'
        AND (wptests_posts.post_status = 'publish')))
ORDER BY wptests_posts.post_date DESC
LIMIT 0, 10

See also #42936, #56991.

[SergeyBiryukov]

In 54829:

Tests: Resolve WP_Query test failures on MariaDB due to indeterminate sort order.

[54768] added a few tests to verify that caching within WP_Query is bypassed when the SELECT clause has been modified via a filter, to avoid cache key collisions and the returning of incomplete or unexpected results.

However, creating several posts with the same date/time fields can result in inconsistent sort ordering between MySQL and MariaDB, as each engine refines the order further using a different index.

This commit aims to stabilize the tests by using assertEqualSets() instead of assertEquals(), since testing the order is out of their scope. Includes removing array_unshift() and array_reverse() calls as no longer needed.

This resolves a few test failures on MariaDB along the lines of:

Tests_Query_FieldsClause::test_should_limit_fields_to_id_and_parent_subset
Posts property for first query is not of expected form.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => stdClass Object (
-        'ID' => 36019
+        'ID' => 36015
         'post_parent' => 0
     )
     1 => stdClass Object (
-        'ID' => 36018
+        'ID' => 36016
         'post_parent' => 0
     )
     2 => stdClass Object (...)
     3 => stdClass Object (
-        'ID' => 36016
+        'ID' => 36018
         'post_parent' => 0
     )
     4 => stdClass Object (
-        'ID' => 36015
+        'ID' => 36019
         'post_parent' => 0
     )
 )

/tmp/wp-test-runner/tests/phpunit/tests/query/fieldsClause.php:67
/tmp/wp-test-runner/phpunit-5.7.phar:598

Follow-up to [54768].

Props peterwilsoncc, SergeyBiryukov.
See #57012.

[SergeyBiryukov]

In 54830:

Tests: Resolve WP_Query test failures on MariaDB due to indeterminate sort order.

[54768] added a few tests to verify that caching within WP_Query is bypassed when the SELECT clause has been modified via a filter, to avoid cache key collisions and the returning of incomplete or unexpected results.

However, creating several posts with the same date/time fields can result in inconsistent sort ordering between MySQL and MariaDB, as each engine refines the order further using a different index.

This commit aims to stabilize the tests by using assertEqualSets() instead of assertEquals(), since testing the order is out of their scope. Includes removing array_unshift() and array_reverse() calls as no longer needed.

This resolves a few test failures on MariaDB along the lines of:

Tests_Query_FieldsClause::test_should_limit_fields_to_id_and_parent_subset
Posts property for first query is not of expected form.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => stdClass Object (
-        'ID' => 36019
+        'ID' => 36015
         'post_parent' => 0
     )
     1 => stdClass Object (
-        'ID' => 36018
+        'ID' => 36016
         'post_parent' => 0
     )
     2 => stdClass Object (...)
     3 => stdClass Object (
-        'ID' => 36016
+        'ID' => 36018
         'post_parent' => 0
     )
     4 => stdClass Object (
-        'ID' => 36015
+        'ID' => 36019
         'post_parent' => 0
     )
 )

/tmp/wp-test-runner/tests/phpunit/tests/query/fieldsClause.php:67
/tmp/wp-test-runner/phpunit-5.7.phar:598

Follow-up to [54768].

Props peterwilsoncc, SergeyBiryukov.
Merges [54829] to the 6.1 branch.
Fixes #57012.