Repository navigation
Canonical: Generate a stable cache key in redirect_guess_404_permalink() - #14146
maheshbohara wants to merge 5 commits into
Conversation
…nk()`. The cache key was derived from the SQL query while it still contained the `wpdb` placeholder escape for the `LIKE` clause. That escape string is regenerated on every request, so the key was different each time and a persistent object cache could never return the cached result. This removes the placeholder escape before hashing the query, as `WP_Query`, `WP_Term_Query` and `WP_User_Query` already do. Follow-up to [64043]. See #66282.
|
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 Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
peterwilsoncc
left a comment
peterwilsoncc
left a comment
There was a problem hiding this comment.
Thank you!
This change looks good to me. I took the liberty of pushing 035f12b to be a little more precise with the test.
I split the final assertion you wrote in to two:
- Check the cache key is generated using the SQL query
- Check the cache key does not include the placeholder
I'll aim to commit this on Monday Australian time, let me know if you're not happy with the changes.
|
Thanks for the review and the test tweak, @peterwilsoncc. I'm happy with the changes. The one failing check (Upgrade from 7.1 / PHP 8.4 / MySQL 8.4) looks like a runner issue: WP-CLI failed to install, so |
Night brain realized my test was broken: for multiple reasons
Yeah, that can happen with tests that use a network connection, as the other upgrades tests were passing I wasn't too concerned. |
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 #14146 Props maheshbohara, therssoftware. Fixes #66282. See #64250. git-svn-id: https://develop.svn.wordpress.org/trunk@64300 602fd350-edb4-49c9-b593-d223f7449a82
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 WordPress/wordpress-develop#14146 Props maheshbohara, therssoftware. Fixes #66282. See #64250. Built from https://develop.svn.wordpress.org/trunk@64300 git-svn-id: http://core.svn.wordpress.org/trunk@63450 1a063a9b-81f0-0310-95a4-ce76da25c4cd
2 participants
[64043] added object caching to the database lookup in
redirect_guess_404_permalink(). The cache key ismd5( $query ), but for the default (loose) guess the query contains aLIKEclause built by$wpdb->prepare(), so the literal%has been swapped for thewpdb::placeholder_escape()token.That token is regenerated on every request, so the cache key is different on every request. With a persistent object cache the stored value is never read back: each 404 guess is a cache miss followed by a new cache write.
This PR removes the placeholder escape before hashing, which is what
WP_Query::generate_cache_key(),WP_Term_QueryandWP_User_Queryalready do.Testing
Before the change, running the same lookup in separate PHP processes stores a different key each time:
After the change the key is the same on every run.
A unit test is included. It captures the SQL through the
queryfilter (where the placeholder escape has already been removed) and asserts that the result is cached under the key derived from that SQL. It fails on trunk and passes with this change. The existing tests from [64043] could not catch this because they call the function twice in one PHP process, where the token is constant.Trac ticket: https://core.trac.wordpress.org/ticket/66282
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus
Used for: Investigating and reproducing the bug, and drafting the patch, unit test and this description. I reviewed the changes and ran the tests locally.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.