Skip to content

Build every supercache path in one place #1089

Description

@donnchawp

This issue was written by AI.

Nine places in the plugin turn a URL into a supercache directory, and every one of them decides for itself how the host segment is derived, what sanitising the URI gets, whether case is normalised, and now whether the path is short enough to use. They are meant to produce identical paths. Nothing makes them.

# Builder Host segment from
1 get_current_url_supercache_dir(), wp-cache-phase2.php:1031 $WPSC_HTTP_HOST
2 get_supercache_dir(), wp-cache-phase2.php:863 home option, via the wp_super_cache_supercachedir filter
3 wpsc_supercache_dir_for_url(), wp-cache-phase2.php:1024 via #2
4 wp_cache_get_ob(), wp-cache-phase2.php:2578 $home_url['host']
5 Admin delete, inc/cache-files.php:119 none, the base64 uri parameter carries it
6 REST delete, rest/class.wp-super-cache-rest-delete-cache.php:35 none
7 Domain mapping, plugins/domain-mapping.php:28,31,55 $sitedir
8 mod_rewrite rules, inc/htaccess.php:195-212 %{SERVER_NAME}
9 Root special cases, wp-cache-phase2.php:1455, inc/lifecycle.php:258 none

Three host derivations, two filter names, and sanitising that has been copied around and drifted.

Why it keeps costing us

The failure is always the same shape. A rule lands on the writing side and does not reach every deleting side, so a deletion builds a path the writer never used, misses, and the stale page carries on being served.

Each fix has been correct and each has been applied by hand to whichever builders the bug happened to expose. The next rule will land the same way.

What I want

One function that maps ( host, uri ) to an absolute supercache directory, holding the sanitising, the case normalisation and the length rule in a single place, with all nine callers routed through it.

Builder 8 is the odd one out and does not have to be solved. The rules emit %{SERVER_NAME} and a raw $1, both expanded by Apache at request time, so no PHP function can be called from them, and mod_rewrite cannot lowercase a captured path anyway since RewriteMap is not allowed in .htaccess. When the URI case differs, the -f test simply fails, the rule does not fire, and the request falls through to PHP, which builds the normalised path and serves the cached file. That costs the static fast path on a request whose URI is not already lowercase. It does not cost a cache hit.

Worth stating plainly, because it is the tempting wrong turn: do not close that gap by taking wpsc_normalize_uri_case() out of the PHP side. Matching Apache that way trades a performance edge case for the writer and deleter mismatch #1084 just finished fixing.

What builder 8 can usefully share is the host segment. Three of the PHP builders derive it three different ways, and one function producing a value that provably matches what %{SERVER_NAME} yields would settle that, with a test to hold it. Keep in mind the two are not the same thing: %{SERVER_NAME} is the configured vhost name and $WPSC_HTTP_HOST is the client's Host header, so they part company on a vhost answering several domains.

Worth folding in while the rule lives in one place: NAME_MAX. A single path segment over 255 bytes fails mkdir even when the whole path fits, and on Linux, where the limit is 4096, that is the more likely of the two failures. It degrades quietly today, wp_mkdir_p() returns false and the request is not cached, so it is not urgent, but it belongs in the same helper as the length check rather than in a tenth place.

The shape of the function

Pinning this down before anyone starts, because the three existing entry points each take something different and the refactor has to converge them: get_supercache_dir( $blog_id = 0 ), wpsc_supercache_dir_for_url( $url ) and get_current_url_supercache_dir( $post_id = 0 ).

/**
 * @param string $uri     Request URI or the path part of a URL. Raw, unnormalised.
 * @param string $host    Defaults to the current request's host.
 * @param int    $blog_id Defaults to the current blog.
 * @return string|false   Absolute trailing-slashed directory, or false if it cannot be used.
 */
function wpsc_supercache_dir( $uri, $host = '', $blog_id = 0 )

Everything the nine builders currently do separately happens inside: the host segment, wpsc_normalize_uri_case(), wpsc_deep_replace(), the query and fragment strip, the length check. Callers pass what they have and do no preparation of their own. It has to be idempotent, since some callers hold a value that has already been through part of this and it is not always possible to tell which.

The return type is the part worth arguing about now rather than later. false for a path that cannot be used is right, but it does not drop into the current call sites, because they concatenate a filename onto the result the moment they get it, and false . 'index.html' is the relative path index.html, which file_exists() may well find in the WordPress root. That is why #1085 returns a placeholder directory instead of an empty string.

So the placeholder is a workaround for callers that do not check, and converting them to check is the migration. Every call site has to test the return value before it concatenates, at which point the placeholder and WPSC_PATH_TOO_LONG_DIR can both go. Writers respond to false by switching caching off for the request, deleters by doing nothing, which is what they already do when handed a directory that is not there.

Worth being honest that this is the risky part of the work. It touches every cache read, write and delete in the plugin, and getting it wrong looks like #1081 and #1084 did: nothing throws, deletions quietly miss, and stale pages carry on being served. It wants tests that assert the writer and the deleters agree on a path, not just that each one returns something reasonable.

Not in scope

Making long URLs cacheable. A path over 4096 bytes on Linux is either a request carrying a pile of context that should not be cached anyway, or a misconfigured page handing the browser a broken srcset. Neither is a page worth caching, and #1085 already stops them filling the error log.

Follows #1081, #1084 and #1087.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions