WP_Query normalization can break plugin filters comparing query variables.

[peterwilsoncc]

The WP_Query normalization changes in #59516 to improve cache hits can break plugins applying filters to secondary/custom end point queries based on query var values.

As an example:

<?php
$query = new WP_Query( array( 'post_type' => array( 'post', 'page' ) ) );

add_filter( 'posts_where', function ( $where, $query ) {
        if ( array( 'post', 'page' ) === $query->get( 'post_type' ) ) {
                // Modify WHERE clause.
        }

        return $where;
}, 10, 2 );

In 6.7 the above filter would modify the WHERE clause, in 6.8 the clause would not be modified as the post type value has been normalized to array( 'page', 'post' ).

A real world example can be seen in ​bbpress's filtering of the WHERE clause generated by bbp_has_replies().

Props @vortfu, @jjj

If the decision is made to revert, the about page will need to be updated to delete the "Take a load off the database" section referencing these changes.

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

[joemcgill]

Thanks @peterwilsoncc, unless there is a very simple solution, I think reverting at this stage of the release cycle is the right call. Then we can reopen the previous ticket and ensure we have unit test to cover this case, if possible.

[peterwilsoncc]

I can think of a solution but not sure how simple it is...

For reverting, we can delete the "Take a load off the database" section of the about page but it looks like there will need to be two string changes:

"Behind the scenes, database optimizations improve performance, and a new security warning system helps prevent vulnerabilities before they become a problem."

If this is a specific reference to this change it becomes:

"Behind the scenes, a new security warning system helps prevent vulnerabilities before they become a problem."

AND

In the performance section the sentence:

"Beyond speculative loading, WordPress 6.8 pays special attention to the block editor, block type registration, and query caching. "

becomes

"Beyond speculative loading, WordPress 6.8 pays special attention to the block editor, and block type registration. "

[peterwilsoncc]

In the ​linked pull request:

• Revert normalization chages
• Delete WP_Query section from about page, moving perf string in to it's place
• Update apparent call outs to this specific change [string changes] from about page

This patch includes reverts of r60048, r59771, r59766. If applying the diff from the GitHub pull request, the reverts can be recorded with svn merge -c -60048,-59771,-59766 --record-only ^/trunk. Alternatively, you may wish to revert each commit and only apply the changes to the about page from the pull request.

Since adjusting the order of post types is being treated as a breaking change, I think that we should include an automated test so that the behavior is not accidentally changed in the future. Here is a quick one that has 3 fails on trunk and all passes in this branch.

	/**
	 * Test that the order of post types in the post_type query var is preserved.
	 *
	 * @ticket 63255
	 * @dataProvider data_provider_test_post_type_array
	 */
	 public function test_post_type_array_order( $post_type_array ) {
		 $query = new WP_Query( array( 'post_type' => $post_type_array ) );

		$this->assertEquals( $post_type_array, $query->get( 'post_type' ) );
		// assert that reversing the array does not equal the original array
		$this->assertNotEquals( array_reverse( $post_type_array ), $query->get( 'post_type' ) );
	 }

	public function data_provider_test_post_type_array() {
		return array(
			array( array( 'post', 'page' ) ),
			array( array( 'page', 'post' ) ),
			array( array( 'page', 'post', 'attachment' ) ),
			array( array( 'attachment', 'post', 'page' ) ),
		);
	}

@aaronjorbin I guess we are deciding that, although there is some normalization already. I think it would be good to test a few more query variables if we are going to do so. Adapted from your tests:

/**
         * Test that the order of post types in the post_type query var is preserved.
         *
         * @ticket 63255
         * @dataProvider data_provider_test_post_type_array
         */
        public function test_post_type_array_order( $query_var, $query_var_value ) {
                $query = new WP_Query( array( $query_var => $query_var_value ) );

                $this->assertSame( $query_var_value, $query->get( $query_var ), 'Query variable getter should reflect order in which it was passed' );
        }

        public function data_provider_test_post_type_array() {
                return array(
                        'post type, string'            => array( 'post_type', 'post' ),
                        'post type, DESC array'        => array( 'post_type', array( 'post', 'page' ) ),
                        'post type, ASC array'         => array( 'post_type', array( 'page', 'post' ) ),
                        'post type, duplicate array'   => array( 'post_type', array( 'post', 'post' ) ),
                        'post status, string'          => array( 'post_status', 'publish' ),
                        'post status, DESC array'      => array( 'post_status', array( 'publish', 'draft' ) ),
                        'post status, ASC array'       => array( 'post_status', array( 'draft', 'publish' ) ),
                        'post status, duplicate array' => array( 'post_status', array( 'draft', 'draft' ) ),

                        'post_name__in, string'        => array( 'post_name__in', 'elphaba' ),
                        'post_name__in, DESC array'    => array( 'post_name__in', array( 'the-wizard-of-oz', 'glinda', 'doctor-dillamond', 'elphaba' ) ),
                        'post_name__in, ASC array'     => array( 'post_name__in', array( 'elphaba', 'doctor-dillamond', 'glinda', 'the-wizard-of-oz' ) ),

                        'category__in, string'         => array( 'category__in', '1' ),
                        'category__in, string[] ASC'   => array( 'category__in', array( '1', '2' ) ), // Fails 6.7 -- normalized to int[]
                        'category__in, string[] DESC'  => array( 'category__in', array( '2', '1' ) ), // Fails 6.7 -- normalized to int[]
                        'category__in, int[] ASC'      => array( 'category__in', array( 1, 2 ) ),
                        'category__in, int[] DESC'     => array( 'category__in', array( 2, 1 ) ),
                        'category__in, int[] duplicate'     => array( 'category__in', array( 1, 1 ) ), // Fails 6.7 -- normalized to remove dupe
                );
        }
`

I've pushed a bunch of tests in d71c019cc43d2cd18db2fee559ff989cad7d1107 -- that includes a few cases in which the values are already standardized (type cast, duplicates removed). The results can be seen in ​https://github.com/WordPress/wordpress-develop/actions/runs/14349309534?pr=8666

Once the test run above completes, I'll push fdf49b6d6eaa5cdb22c0e7399d061a39820559bb to remove the tests that fail due to existing standardization.

[JeffPaul]

"Beyond speculative loading, WordPress 6.8 pays special attention to the block editor, and block type registration. "

Can likely drop the final comma to become:
"Beyond speculative loading, WordPress 6.8 pays special attention to the block editor and block type registration."

[JeffPaul]

Also noting on the About page that we'll need to adjust the sections such that the removal of the "Take a load off the database" section means the "Performance updates" section moves up into its space (crude diagram attached).

[swissspidy]

Am I the only one not convinced that this needs a revert?

What bbPress does seems brittle either way, plugins shouldn't rely on the order of array items like that. And this doesn't appear to be a widespread problem either.

My suggestion would be:

1. Fix bbPress
2. Ship 6.8 as-is, maybe make this more clear in the field guide if needed.
3. Keep an eye out for more cases, and then reassess for 6.8.1.

[joemcgill]

Thanks for the gut check, @swissspidy! I'll admit that I've been battling an illness this week and I did not fully grasp the concern that was being raised here when I first read through the ticket.

Looking again today, it seems that if we want to maintain support for filters to expect to receive post types (or other normalized properties) from the query in a specific order, then we would need to normalize the cache key generation without affecting the order in which these actually get processed in the query. This may be doable, but also adds an additional level of complexity to cache key generation that seems undesirable — particularly when affected plugins can address this by sorting the the query properties before comparing (or using less brittle approaches, as Pascal suggests).

Aa an example, using the use case @peterwilsoncc added to the issue description, this would be a simple way to fix broken filter callbacks that previously relied on specific array ordering:

<?php
$query = new WP_Query( array( 'post_type' => array( 'post', 'page' ) ) );

add_filter( 'posts_where', function ( $where, $query ) {
        if ( sort( array( 'post', 'page' ) ) === $query->get( 'post_type' ) ) {
                // Modify WHERE clause.
        }

        return $where;
}, 10, 2 );

Given that it's also possible for other filter callbacks to affect the order of these values before they're passed through to other callbacks, I don't really think we should force back-compat in this case.

Would love to hear feedback from @peterwilsoncc, @johnjamesjacoby, or @vortfu prior to finalizing this decision, but currently I think we should ship 6.8 as is and move this to 6.8.1 for visibility, in case there are other reports. If a revert is needed at that point we can ship it in 6.8.1, otherwise close this as a wontfix.

[peterwilsoncc]

@jeffpaul -- I've got that in the PR if it's decided to revert.

---

@swissspidy @joemcgill I think the main issue is that I didn't think to post a dev note as I figured the change was an internal caching matter and didn't realise it could effect plugins.

When I was writing tests to run against the reverted branch, I noticed there is a lot of standardization in the class already. As of 6.7 there isn't any sorting but there is plenty of type casting, removing of duplicates and setting values to their defaults. There is also a fair bit of one argument populating others (x populating x__in and x__not_in, for example).

Given the amount of standardization in the class already, my current inclination is to write up a dev note rather than revert. I agree with @swissspidy's comment above that the array comparison I've used in the ticket is not great to begin with.

[johnjamesjacoby]

bbPress is an easy fix, and I'll get that done.

I think the probability is high that other plugins are doing "loose" comparisons similar to bbPress for a few reasons:

• all plugins get inspiration from each other, and thousands of them compare things to WP_Query
• the order of array values in any _Query class has simply never mattered – it's totally off the radar of everyone
• arrays of arguments are allowed to be out-of-order everywhere else (into and out-of functions & filters, etc...)

Like @spacedmonkey said in the original ticket, all of the other _Query classes would benefit from the idea of normalizing the values that go into generating the cache key, so in that vein I think the most complete version of this code would include global public functions to encapsulate & DRY these (and other) specific repeated patterns:

• sort()
• array_unique()
• array_unique( array_map( 'absint', $thing ) )
• implode( ',', array_map( 'absint', $thing ) )
• array_map( 'absint', array_unique( (array) $thing ) );
• array_unique( array_map( 'intval', preg_split( '/[,\s]+/', $thing ) ) );

If this were my call, me personally, I'd err on the side of avoiding breakage related to the infinite unknown of the plugin ecosystem, revert for 6.8, create some global functions for plugins (and core) to use, ship WP_Query in 6.8.1 or 6.8.2, and the other _Query classes subsequently.

(Fwiw, I love this performance improvement overall, and already do something similar to this inside of BerlinDB, so I am excited to see it in WordPress too.)

[spacedmonkey]

Instead of change the query args, could we consider only going this normalisation as part of the cache key generation? Would not the benefit but would have some benefit. I

[joemcgill]

Thanks @johnjamesjacoby. I've done ​a quick search of the plugins directory for plugins that are directly accessing WP_Query::post_type and bbPress is the only one I've come across that is doing an array comparison with multiple values like this. Most plugins that do so are either checking for a single specific post type, or checking to see if a value is in the array of post types.

Regarding a few other points:

• the order of array values in any _Query class has simply never mattered – it's totally off the radar of everyone
• arrays of arguments are allowed to be out-of-order everywhere else (into and out-of functions & filters, etc...)

To my mind, these both are reasons not to revert the current change. The bbPress example is actually using a strict type check (!==) in ​the code that's been referenced, which is counter to the principles you pointed out.

I think the only way we include this normalization for Query caching without any potential for breakage is to separate the normalization for caching from the actual values used in the queries, as @spacedmonkey suggests. However, as I already mentioned:

This may be doable, but also adds an additional level of complexity to cache key generation that seems undesirable.

There very well may be unknown side effects of this change that we'll need to resolve, but I think the only way we'll know for sure is to ship it and wait for actual reports, since this has already been in trunk for quite a while and this is the first report I've seen about potential breakage.

I'm going to move this to 6.8.1 and we can revert quickly if this causes any major unreported breakage that can't easily be fixed in a follow-up release.

[johnjamesjacoby]

@joemcgill I checked out the link—while bbPress is the only plugin using that specific strict comparison, I noticed other plugins are doing similar get/set operations on post_type. That seems to support the broader point I was trying to make about plugins generally not accounting for array sort order.

For instance, Jetpack appends the comics post type without re-sorting:
​https://wpdirectory.net/search/01JRJGRVBBT7Y5FWV4ZP833G67

It looks like this could open the door to similar shared-state issues across other WP_Query arguments that involve order-sensitive arrays (like post_status, etc.)

A more fault-tolerant approach might be to handle normalization in some isolated way that is not directly exposed to hooks.

One option could even be normalizing a copy/stub of the main WP_Query object, so the original values remain untouched.

I get the concern about reacting to breakage reports for 6.8.1, and that’s totally valid. I just still have some reservations about whether this particular solution is the most robust in the long term.

[peterwilsoncc]

For instance, Jetpack appends the comics post type without re-sorting: ​https://wpdirectory.net/search/01JRJGRVBBT7Y5FWV4ZP833G67.

I think that's fine, there's never been any benefit to resorting. Standardization happens after pre_get_posts but any modifications on the SQL clauses' filters will not be standardized by core and will need to be handled by plugins.

Instead of change the query args, could we consider only going this normalisation as part of the cache key generation? Would not the benefit but would have some benefit.

I did a thought experiment on this and came to the view it would be very flaky. It would involve:

• keeping track of which query_vars were standardized prior to this change and continuing to do so
• cloning query_vars in to cache_vars or whatever
• modifying ::set() to update both
• magic methods to update both when the public property is updated directly
• standardizing the new property & using that to generate the SQL, requiring duplication of a lot of code in an already chunky method.

---

Given there is some standardization in the class already, I don't want to commit to retaining some values but not others. If it's decided to revet, I think it's best to drop the tests I've included for that. (Here's a ​gist of failing tests on 6.7).

If there is a revert, I think it can be done in the 6.8 branch to allow for an extended lead time whenever 6.9 is released.

Of course, the final decision remains with @desrosj @joemcgill and @Mamaduka as tech leads for 6.8.

[jorbin]

Unless it's documented or already in heavy use, I don't think that the order of array items is something that should be considered a part of the API that should be protected from a backcompat break. The fact that it took multiple RCs for this to be noticed makes me think that there isn't heavy use.

If it's decided to revet, I think it's best to drop the tests I've included for that.

If it's decided that revert, than the decision has been made that this is a backcompat break and tests will guard against an accidental breakage in the future.

I'm not opposed to revisiting this decision as a part of 6.8.1, but I think the bar should be fairly high for us to consider a revert then.

One pre 6.8 task that I think should be done is an addition to the field guide to help educate and also to document this decision. Here is a draft for that:

In order to improve the cache hit percentage for queries, WordPress 6.8 optimizes and normalizes the parameters of WP_Query. This is a step that will ensure that passing in a different order for parameters such as post type doesn't lead to multiple caches for the same data. You should not rely on the order of array items if you are checking values passed to WP_Query.

An alternative approach in which the query vars are only modified after the pre_get_posts filter.

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

[peterwilsoncc]

Standardization happens after pre_get_posts but any modifications on the SQL clauses' filters will not be standardized by core and will need to be handled by plugins.

This was incorrect, trunk has a few additional items standardized prior to pre_get_posts that doesn't exist in 6.7 or earlier. ​PR#8685 moves the new standardization until after the hook fires.

Items that had existing standardization are not modified, if it happened prior to pre_get_posts in 6.7 the behavior is retained.

There are changes for the form of $query passed to the SQL clauses but I think that's acceptable as they are very low level filters that require care when implementing.

[joemcgill]

Thanks for following up on this, @peterwilsoncc. I'm still of the mind that back-compat for array order in filters is not something we need to maintain based on what I've seen thus far and I'd prefer not to introduce an 11th hour change that might lead to more issues, so let's keep this in 6.8.1 and reassess if we see that either of these concerns cause large enough issues to require a revert.

[jorbin]

Thusfar there haven't been issues that have forced us to consider this revert. I'm moving it to 6.8.2 to keep it available, but I think that if there is still no need for it as that cycle happens, this should be closed.