Make WordPress Core

Opened 11 months ago

Closed 10 days ago

Last modified 10 hours ago

#64250 closed enhancement (fixed)

Refactor redirect_guess_404_permalink to use WP_Query instead of raw SQL

Reported by: spacedmonkey Owned by: spacedmonkey
Priority: normal Milestone: 7.2
Component: Permalinks Version: 2.3
Severity: normal Keywords: has-patch has-unit-tests needs-dev-note early
Cc: Focuses: performance

Description

The current implementation of the redirect_guess_404_permalink() function uses a direct SQL query to retrieve post IDs when attempting to guess the correct URL for a 404 request based on query variables. While this approach works, it bypasses WordPress’s built-in query abstraction layer and associated benefits.

This change proposes replacing the raw SQL with a WP_Query implementation. Doing so will improve maintainability, consistency, and performance through native WordPress features such as:

  • Query caching – Utilises WordPress’s object caching layer, reducing redundant database queries.
  • Filtering and extensibility – Enables the use of hooks and filters that developers may rely on to modify query behaviour.
  • Security and readability – Removes the need for manual SQL sanitisation and improves code clarity.
  • Future compatibility – Ensures the function remains aligned with core WordPress query handling and schema changes.

The refactor should maintain the same functional behaviour — guessing the most appropriate redirect target for a 404 request — while ensuring no regression in performance or accuracy.

Change History (44)

This ticket was mentioned in ​PR #10531 on ​WordPress/wordpress-develop by ​@altf4falt.


11 months ago
#1

  • Keywords has-patch added

The redirect_guess_404_permalink() function in src/wp-includes/canonical.php has been refactored to replace direct SQL queries with WordPress's native WP_Query abstraction layer.

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

#2 @altf4falt
11 months ago

I've refactored the function to use WP Query instead of direct SQL queries.

PR Link: ​https://github.com/WordPress/wordpress-develop/pull/10531

​@altf4falt commented on ​PR #10531:


11 months ago
#3

@westonruter I've resolved the comments. Seeking clarification on one comment and requesting for a re-review

#4 @westonruter
11 months ago

  • Keywords good-first-bug removed
  • Milestone Future Release → 7.0
  • Owner set to spacedmonkey
  • Status new → reviewing

This ticket was mentioned in ​PR #10590 on ​WordPress/wordpress-develop by ​@spacedmonkey.


10 months ago
#5

Trac ticket:

​@spacedmonkey commented on ​PR #10531:


10 months ago
#6

@westonruter @anandrajaram21 I have put together a little PR to use WP_Query without a filter. ​https://github.com/WordPress/wordpress-develop/pull/10590/commits/4c87073adeed8643d01d8a5934d7c8c5c00cf889. Completely untested, but this is how I think this should work.

​@westonruter commented on ​PR #10531:


10 months ago
#7

@spacedmonkey Clever. So you're leveraging the search capabilities of WP_Query to implement fuzzy redirect guessing, specifically via restricting the search_columns to just include the post_name.

Full diff from base: ​https://github.com/WordPress/wordpress-develop/compare/33c8d7efa3d96ddd9a87168dfe15e771fb81d29c...4c87073adeed8643d01d8a5934d7c8c5c00cf889

​@westonruter commented on ​PR #10531:


10 months ago
#8

In looking at ​https://github.com/WordPress/wordpress-develop/pull/10590 which has the aforementioned commit, unit tests are failing.

​@altf4falt commented on ​PR #10531:


10 months ago
#9

@spacedmonkey This approach makes sense. I'll try this out, however as @westonruter mentioned, the tests do not pass, so I'll try making the tests pass while keeping the core idea in mind. Thanks for the input!

​@spacedmonkey commented on ​PR #10531:


10 months ago
#10

@anandrajaram21 Here is the full changeset Full diff from base:
​https://github.com/WordPress/wordpress-develop/compare/33c8d7efa3d96ddd9a87168dfe15e771fb81d29c...ff81f8864ce2277b9c2c3596315af4e55942960b

I had to make some more tweaks to WP_Query, but it seems to be working. It needs unit tests and testing.

​@altf4falt commented on ​PR #10531:


9 months ago
#11

@westonruter @spacedmonkey I've made the required changes as suggested, and also tweaked the test cases to make sure they pass. Do let me know if I can go ahead and write unit tests for the modified WP_Query, or if there are any other changes to be made before I do so

This ticket was mentioned in ​Slack in #core-performance by westonruter. ​View the logs.


9 months ago

#13 @spacedmonkey
8 months ago

  • Keywords commit has-unit-tests needs-dev-note added

​@mukesh27 commented on ​PR #10531:


8 months ago
#14

@westonruter or @spacedmonkey Could either of you commit this PR so we can get proper testing during the betas/RCs? Since it touches WP_Query, it would be good to get it committed as soon as possible to allow broader testing and reduce any potential risk.

​@westonruter commented on ​PR #10531:


8 months ago
#15

Yes, @spacedmonkey is going to commit.

#16 @peterwilsoncc
8 months ago

  • Keywords changes-requested added

@westonruter @spacedmonkey Sorry, I haven't had much opportunity to review the proposed PR before now.

While I agree the query in redirect_guess_404_permalink() ought to be cached, I'm not sure that adding a dedicated feature to WP_Query is an appropriate approach.

If WordPress is to allow developers to specify the search position, then that's the parameter that should be introduced, it search_pos => start,end,none or whatever.

We'll also need to make sure it is/isn't available to be set on the front end, as appropriate, and include the variable in WP accordingly. With tests for that too.

As an initial step to include performance of this query it could be stored as a salted cache in the post-queries group.

It would also be good to consider the performance impact of caching the current code vs using a WP_Query instance. WP_Query is a lot while the current code is very little.

#17 @spacedmonkey
8 months ago

Thanks for your feedback @peterwilsoncc

I did consider just adding caching here. This issue was found because I am working on a site that has around 2.4 million posts and 1+ million attachments. The like search here, simply falls over. So it is best for a query of that size to be punted off to a search provider like elastic search. There are already a number of drop-in solution for adding ES or other scalable search solution / database to WP_Query. So WP_Query feels like a good fit here.

I like the idea of adding search_pos and having these options.

  • anywhere so %s%
  • start %s
  • end %s.

None does not make sense here, we already have exact parameter, that feel like that.

Some ideas for naming

'search_match_type' => 'prefix' | 'suffix' | 'contains'

'search_like' => 'left' | 'right' | 'both'

'search_position' => 'start' | 'end' | 'anywhere'

We could add it to WP, not sure in the value of that, but I am not against it.

This ticket was mentioned in ​Slack in #core-performance by westonruter. ​View the logs.


8 months ago

#19 @peterwilsoncc
8 months ago

@spacedmonkey You're right, none isn't a good word but I figured you'd get the idea. Of the three options you present, I like the third best with the first as a very close second.

My preference remains for the generic rather than the current code's version as it would be a little frustrating to end up with exact, starts_with and ends_with.

Re: ES drop-ins, I think a little outreach would need to be made to ensure that changes work as expected. As the addition of the slug to the searchable fields and the position (whether the changes I've proposed or those in the existing PR) would either bypass the s trigger for elastic search or trigger the wrong result set.

#20 @audrasjb
8 months ago

  • Milestone 7.0 → 7.1

Removing commit as it looks like there is still work to do on this proposal.
Moving to 7.1 as 7.0 beta 1 is tomorrow, but feel free to move it back to the milestone if the patch is ready to ship in the next few hours :)

#21 @audrasjb
8 months ago

  • Keywords commit removed

This ticket was mentioned in ​PR #10975 on ​WordPress/wordpress-develop by ​jonnynews.


8 months ago
#22

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

## Use of AI Tools

#23 @spacedmonkey
8 months ago

  • Milestone 7.1 → 7.0

I have updated ​PR based on @peterwilsoncc feedback.

Can I get another review from @westonruter @altf4falt @mukesh27

#24 @audrasjb
8 months ago

  • Milestone 7.0 → 7.1

WP 7.0 pre-beta1 Triage:
Unfortunately this enhancement didn't make it before beta 1 code freeze. Thus, let's move it to milestone 7.1.

#25 @wildworks
3 months ago

  • Milestone 7.1 → Future Release

As this ticket has been inactive for a while, I will move the milestone to "Feature Release". Please feel free to move it back if it can be completed by 7.1 beta 1.

#26 @spacedmonkey
5 weeks ago

  • Keywords early added; changes-requested removed
  • Milestone Future Release → 7.2

@peterwilsoncc @westonruter @mukesh27

I actioned all your feedback. This looks ready to commit. I really want to commit this as early as possible, so can I get a final review ASAP.

​@westonruter commented on ​PR #10975:


5 weeks ago
#27

I'm not familiar with the intricacies here. To get up to speed, I've had Claude do a medium-effort code review of the changes.


🤖 _Comment by Claude Opus 5_

A code review of this branch was run at medium effort. Six findings are reported below, ordered by severity. Two of them are believed to produce broken SQL or incorrect redirects on ordinary sites.

---

### 1. HIGH — search_position => 'end' empties search_orderby_title and yields malformed ORDER BY SQL

src/wp-includes/class-wp-query.php (in parse_search()):

$like = $start . $wpdb->esc_like( $term ) . $end;
if ( $end && ! $exclude ) {
        $query_vars['search_orderby_title'][] = $wpdb->prepare( "{$wpdb->posts}.post_title LIKE %s", $like );
}

The guard here was previously if ( $n && ! $exclude ), where $n was '' only when exact was set — the intent was "skip title relevance ordering for exact matches". Rewriting it as $end accidentally couples it to search_position: for 'end', $end is '', so no clause is ever collected and $query_vars['search_orderby_title'] stays empty.

That array being empty is not harmless, because parse_search_order() is still reachable. The guard in get_posts() is:

if ( ! empty( $query_vars['search_orderby_title'] ) && ( empty( $query_vars['orderby'] ) && ! $this->is_feed ) || ( isset( $query_vars['orderby'] ) && 'relevance' === $query_vars['orderby'] ) ) {

&& binds tighter than ||, so an explicit orderby => 'relevance' bypasses the non-empty check entirely. Two concrete failures:

  • new WP_Query( array( 's' => 'three', 'search_position' => 'end', 'orderby' => 'relevance' ) ) — single term, so parse_search_order() takes the else branch and returns reset( array() ) . ' DESC', producing ORDER BY DESC: a SQL syntax error and zero results.
  • new WP_Query( array( 's' => 'two three', 'search_position' => 'end', 'orderby' => 'relevance' ) ) — multi-term, so 'WHEN ' . implode( ' AND ', array() ) . ' THEN 2 ' produces CASE WHEN THEN 2 ...: also a SQL syntax error.

Separately, even without orderby => 'relevance', 'end' silently drops title relevance ordering altogether, which is a behavior regression unrelated to the position semantics.

The suggested fix is to key the guard off exact (its original meaning) and let $like carry the position, so 'end' orders on '%term' and 'start' on 'term%':

if ( empty( $query_vars['exact'] ) && ! $exclude ) {

No existing test covers a multi-term search or orderby => 'relevance' in combination with 'end', which is why this was not caught.

Note that a latent variant of the same crash pre-exists on trunk for exact => true combined with orderby => 'relevance', caused by the operator precedence noted above. That is out of scope here, but it becomes newly reachable through a documented, supported parameter with this change.

---

### 2. HIGH — Switching the loose 404 guess to s re-enables the - exclusion prefix, inverting the match

src/wp-includes/canonical.php, in redirect_guess_404_permalink(), the loose path now passes the slug as s rather than querying post_name directly.

WP_Query::parse_search() treats a term beginning with the wp_query_search_exclusion_prefix (- by default) as an exclusion: $like_op becomes NOT LIKE and the prefix is stripped. The value of get_query_var( 'name' ) comes straight from the rewrite match and is not passed through sanitize_title(), so a 404 request for /-foo/ yields name = '-foo' and the query becomes:

post_name NOT LIKE 'foo%'

That matches nearly every post on the site, and with posts_per_page => 1 and orderby => 'none' the visitor is 301-redirected to an arbitrary unrelated post. The previous post_name LIKE '-foo%' correctly matched nothing and returned false.

---

### 3. MEDIUM — s tokenizes the slug, breaking guesses for slugs containing ,, +, ", or whitespace

parse_search() splits the search string on /".*?("|$)|((?<=[\t ",+])|^)[^\t ",+]+/. A slug such as foo,bar therefore becomes two AND-ed terms:

(post_name LIKE 'foo%') AND (post_name LIKE 'bar%')

which no row can ever satisfy. The previous single post_name LIKE 'foo,bar%' would have matched. The effect is that loose 404 guessing silently stops working for any slug containing those characters. Passing 'sentence' => true — or keeping a direct prefix query — would avoid the tokenizer.

---

### 4. MEDIUM — The new WP_Query exposes 404 guessing to pre_get_posts and post_search_columns

suppress_filters does not gate pre_get_posts (fired before the suppress_filters checks in get_posts()) and does not gate post_search_columns at all. Because the loose path sets s, this query reports is_search() === true, so a very common plugin pattern —

add_action( 'pre_get_posts', function ( $q ) {
        if ( $q->is_search() ) {
                $q->set( 'post_type', 'product' );
        }
} );

— now silently rewrites the guess query on every 404 request. Likewise, a site-wide post_search_columns filter can strip post_name and cause the guess to match on post_title or post_content instead. The previous direct $wpdb query was unhookable. This is worth either documenting as intended or bypassing, for example via a private flag checked by core.

---

### 5. LOW — Strict and loose guesses now disagree about password-protected posts

parse_search() appends AND post_password = '' for logged-out users. That clause is added only on the loose path, which uses s, and not on the strict path, which uses name. Neither path excluded password-protected posts before this change, so strict_redirect_guess_404_permalink => true and the default now behave differently for such posts.

---

### 6. LOW — The mutual-exclusion _doing_it_wrong() fires on inputs that are not a conflict

The exact / search_position check runs in parse_query(), before the normalization added in get_posts(), and only tests 'anywhere' !== $query_vars['search_position']. As a result:

  • array( 'exact' => true, 'search_position' => 'garbage' ) emits the "mutually exclusive" notice even though 'garbage' is about to be normalized to 'anywhere' and has no effect on the query.
  • The notice also fires when s is empty, where neither parameter influences the query at all.

Validating the value first, and warning only for an actual 'start' or 'end' with a search present, would avoid these spurious notices.

---

A fix for finding 1, together with regression tests covering each search_position value with orderby => 'relevance' for both single-term and multi-term searches, has been prepared locally and can be pushed to this branch on request.

See Core-64250.

​@westonruter commented on ​PR #10975:


5 weeks ago
#28

Note that the PHPStan job was failing because this PR fixes two errors which had been baselined. Baselines regenerated in 1c45ffa via composer phpstan:baselines.

This ticket was mentioned in ​Slack in #hosting by spacedmonkey. ​View the logs.


5 weeks ago

This ticket was mentioned in ​Slack in #core-performance by spacedmonkey. ​View the logs.


5 weeks ago

#31 follow-up: @peterwilsoncc
5 weeks ago

@spacedmonkey I think I stand by my original comment (#16) that adding a feature to WP_Query for this isn't desirable and that, at most, caching should be added to the existing query.

redirect_guess_404_permalink() is for typo capturing and it's unlikely the same typo will be made multiple times so there's very little gain for most sites.

Very few sites use elasticpress and the pre_redirect_guess_404_permalink filter already exists for such sites to handle typos. For the typical site, I think instantiating WP_Query will slow the recovery down for most sites.

#32 in reply to: ↑ 31 @spacedmonkey
4 weeks ago

Replying to peterwilsoncc:

@spacedmonkey I think I stand by my original comment (#16) that adding a feature to WP_Query for this isn't desirable and that, at most, caching should be added to the existing query.

redirect_guess_404_permalink() is for typo capturing and it's unlikely the same typo will be made multiple times so there's very little gain for most sites.

Very few sites use elasticpress and the pre_redirect_guess_404_permalink filter already exists for such sites to handle typos. For the typical site, I think instantiating WP_Query will slow the recovery down for most sites.

I am not sure I understand the pushed back on using WP_Query here. The core team has slowly been migrating more and more parts of core to use WP_Query over raw cached queries. Example [59715]. The changes to WP_Query and small and opt-in by design. The functionality is not expose to WP by design, as this is not something I feel like front end users should need to do / want to do. But adding that in, is another discussion. We have had similar discussion in when we added caching to get_adjacent_post_link in #41131. In that case, there were filters like get_{$adjacent}_post_where that means we can't easily use WP_Query without breaking changes. However, there is nothing stopping us here, other than the fact we have to add a very small change to WP_Query. I feel like the change is useful for developers, I have personally had to work around the fact that I can't search for a something at the start of a field.

Is the issue here, that we using WP_Query or that we have to make tweaks to WP_Query. Would a work around like wp_allow_query_attachment_by_filename filter work better for you? See [54524].

This ticket was mentioned in ​PR #13498 on ​WordPress/wordpress-develop by ​@spacedmonkey.


4 weeks ago
#33

…se queries and improve performance. Add unit tests to verify caching behavior and cache invalidation scenarios.

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

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude
Model(s): Opopus 5
Used for: Initial code skeleton and test suggestions; final implementation and tests were reviewed and edited by me.

#34 @spacedmonkey
4 weeks ago

Still think we should use WP_Query here - but I used AI to put together a PR that does not use WP_Query to see what that would look like

​https://github.com/WordPress/wordpress-develop/pull/13498

#35 @peterwilsoncc
4 weeks ago

I think ​PR#13498 is a better approach. A test is currently failing so it will need a little more work.

The concern I have with using WP_Query is largely as stated in my original comment:

  • Instantiating WP_Query for this requires a lot more code to run to generate much the same query
  • Adding dedicated features to WP_Query for permalink guessing is overkill

While I understand your concern that this code is inefficient for a site with 2.4 million posts and 1+ million attachments, the preflight filter is available for this situation. Moving the query generation to WP_Query doesn't solve the issue, it just offloads it to another component.

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


4 weeks ago

#37 @im3dabasia1
4 weeks ago

As per today's 7.2 bug scrub:

I see there's been an ongoing discussion on the right approach here. @spacedmonkey @peterwilsoncc , it looks like the newer [PR]​https://github.com/WordPress/wordpress-develop/pull/13498 might be the direction to converge on, but it has a failing tests. Can we confirm that's the path forward and get that sorted?

#38 @spacedmonkey
3 weeks ago

I totally get the concern about WP_Query overhead compared to a raw SQL query, but in practice, WP_Query is already the engine running WordPress. A standard Twenty Twenty-Four page runs around 24 WP_Query instances, and core uses it for everything from render_block_core_template_part down to quick lookups like get_page_by_path. The minor class overhead really pales in comparison to the actual bottleneck here: unindexed SQL LIKE scans hitting the database directly every time a 404 happens.

The pre_redirect_guess_404_permalink filter has been available for years, but relying on custom fixes hasn't solved the issue across the broader ecosystem. Even enterprise hosts like VIP and Pantheon don't provide platform-level patches for this, and these unindexed queries regularly sneak past and drag down origin servers. If asking developers and hosts to find an obscure hook and patch a core flaw was going to work, it would have happened by now.

Switching this logic to WP_Query solves the problem using established core patterns. We get native object caching automatically out of the box, meaning core doesn't have to roll its own custom, one-off caching scheme. It also makes life much easier for external search engines. While providers like ElasticPress or Algolia will need to explicitly opt-in to handle this new parameter, that's a quick tweak for them since their integrations are already built around posts_pre_query and standard WP_Query hooks. Tapping into the caching and search infrastructure the ecosystem already uses just makes the most sense.

#39 @peterwilsoncc
13 days ago

@spacedmonkey I'm still unconvinced that WP_Query is the solution here and that ​PR#13498 is the best approach to take. Are you able to follow up the code review on the ticket to allow for commit -- mainly the addition of messages for tests that use multiple assertions.

​@spacedmonkey commented on ​PR #13498:


13 days ago
#40

Thanks for the review! Addressed in 56aa249a8d:

  • Added the missing message parameter to all assertSame()/assertFalse() calls flagged in tests/phpunit/tests/canonical.php.
  • Applied the suggested wording for the post-type collision assertion (Setting post type to page did not return the page permalink.).
  • Rebased on current trunk.

Verified locally: phpcs and phpstan pass clean on the changed files, and phpunit --group canonical passes (1062 tests, 1150 assertions), including test_redirect_guess_404_permalink_cache_keys_do_not_collide run repeatedly to confirm it's no longer flaky.

🤖 Generated with ​Claude Code

#41 @spacedmonkey
13 days ago

@peterwilsoncc Asserts are now updated.

This ticket was mentioned in ​Slack in #core-performance by spacedmonkey. ​View the logs.


13 days ago

#43 @peterwilsoncc
10 days ago

  • Resolution → fixed
  • Status reviewing → closed

In 64043:

Canonical: Cache redirect_guess_404_permalink().

Introduce caching of both hits and misses to the database query within redirect_guess_404_permalink().

Props altf4falt, audrasjb, im3dabasia1, spacedmonkey, westonruter, wildworks.
Fixes #64250.

#44 @peterwilsoncc
10 hours ago

In 64300:

Canonical: Generate stable cache for redirect_guess_404_permalink().

Run $wpdb->remove_placeholder_escape() on the prepared database query when generating the cache key.

The WPDB placeholder varies each time WordPress is executed so without replacement the cache key is unstable resulting in an unreachable cache entry.

Follow up to r64043.
Developed in ​https://github.com/WordPress/wordpress-develop/pull/14146

Props maheshbohara, therssoftware.
Fixes #66282.
See #64250.

Note: See TracTickets for help on using tickets.