Automattic / Automattic/wp-super-cache

Refactor: introduce a Config module to own the config-file write path

Open
#1,062 2 comments 0 reactions 0 assignees View on GitHub
enhancement ready-for-agent
Dominant language
PHP
Stars
436
Forks
130
Avg merge
15h 11m
Merged PRs (30d)
10

Description

> *This issue was generated by AI.*

Concrete, tiny-commit implementation plan for **#1047 item #1** (a `Config` module that owns the on-disk config file). Sequenced **after the `wp-cache.php` file move (#1061)** and building on the PHPUnit test net (#1051). Scope was deliberately narrowed in a planning interview to the **write path only**: globals stay exactly as they are; reads are untouched.

## Problem Statement

WP Super Cache's configuration is ~63 loose PHP variables (≈100 counting `$wp_cache_pages[...]` sub-keys) defined in `wp-cache-config.php`. The write path is the dangerous part:

- Writes go through two mechanisms: `wp_cache_setting()` (53 call sites) and **direct calls to `wp_cache_replace_line()` (64 call sites)** — a ~100-line function that rewrites a live PHP file with a per-line regex.
- More than half of all writes bypass the `wp_cache_setting()` wrapper and call the raw regex rewriter directly, each passing its own hand-built regex and replacement line.
- There is no single owner of the file's format. The effective "schema" is the union of 117 regex/line pairs scattered across the codebase.
- The file-rewriting logic has **no test coverage**, so any change to it is unverifiable.

This makes adding a setting a three-part chore (a new global, a new write call with a bespoke regex, a new read site) and makes the rewriter impossible to refactor safely.

## Solution

Introduce a single **`Config` module** that owns reading and writing the config file and is the only code that knows the file's on-disk format. Every write funnels through it.

Crucially, this is an **additive abstraction, not a replacement of the globals**:

- The globals stay populated exactly as today (`Config::set()` updates `$GLOBALS[$field]` just like `wp_cache_setting()` already does). All 193 read sites (`global $foo;`, `$GLOBALS['foo']`, and the settings-map's dynamic lookup) keep working unchanged.
- `wp_cache_setting()` and `wp_cache_replace_line()` **remain as global functions** — they are part of the public surface third-party cache plugins may call — but become thin delegations to the module.
- The dangerous regex-rewriting logic moves into one place, gets a small enough surface to test thoroughly, and the 64 direct callers migrate to a typed `Config::set($key, $value)` API.

The module follows the established `src/` convention (namespace `Automattic\WPSC`, `class-*.php` filename, explicit `require_once` at runtime — there is **no runtime autoloader**; the Composer classmap is dev/test-only). It is loaded explicitly from `wp-cache.php` for the admin write path and lazily (guarded by `class_exists`) from the two phase2 write shims so the debug-log write path keeps working when only the engine is loaded.

## Commits

### Phase A — Lock down current write behaviour first

1. **Characterization tests for the existing write path.** Against a temporary config file, assert the exact resulting file contents after `wp_cache_setting()` for each value type (numeric, boolean, array/object via `var_export`, string) and after representative direct `wp_cache_replace_line()` calls covering its three branches: replace an existing line, the "setting unchanged" early return, and the "not found → insert after the defines/assignments" append path. Also assert `$GLOBALS[$field]` is updated. These tests encode today's behaviour byte-for-byte and become the regression guard for everything that follows.

### Phase B — Introduce the owner (no caller changes yet)

2. **Add the `Config` class.** Create the class under `src/` following the existing convention, moving the file read/format/write logic into methods: a writer that performs the current atomic `tempnam` → write → `rename` → `chmod` → `opcache_invalidate` sequence, and a value formatter that reproduces the numeric/bool/array/string line formatting. Wire runtime loading: explicit `require_once` in `wp-cache.php`, and lazy `require_once` (guarded by `class_exists`) in the phase2 shims. No existing caller changes; the class is loadable but unused. Add direct unit tests mirroring the Phase A characterization tests against the class methods.

3. **Delegate `wp_cache_replace_line()` to the module.** Replace the function body with a call into the `Config` writer; keep the signature identical. Run the Phase A characterization suite — output must be byte-identical. This routes all 64 direct callers through the owner without touching any of them.

4. **Delegate `wp_cache_setting()` to the module.** Replace its body with a `Config::set()` call (which formats the value, updates `$GLOBALS`, and writes). Run the suite — identical behaviour. After this commit there is exactly one owner of the file format and all 117 write sites flow through it via the two shims.

### Phase C — Migrate direct callers to the typed API

Each commit converts one cluster of direct `wp_cache_replace_line( '^ *\$foo', "\$foo = ...;", $file )` calls into `Config::set( 'foo', $value )`, verified against the test suite and the e2e settings specs. Grouped by the (now-split, post-#1061) file/cluster so each commit is small and revertable:

5. **Lifecycle writes** — activation/deactivation/enable/disable toggles (e.g. enabling/disabling caching).
6. **Settings-form writes** — the rejected/accepted-list, tracking-parameter, and debug-settings updaters.
7. **Admin-UI / manager writes** — the `wp_cache_manager_updates` settings-save path.
8. **Preload writes** — preload-related settings persisted from the preload UI/cron.
9. **Remaining scattered writes** — htaccess/mod-rewrite flags, cron-check, version stamps, and any stragglers.

Calls that are not expressible as a simple key/value `set()` (e.g. removing a line, conditional `define` rewrites, or multi-line edits) stay on the raw `wp_cache_replace_line()` function — which still works, now delegating — and are listed explicitly so the exceptions are visible.

10. **Optionally retire `wp_cache_setting()` internal usage.** Convert the 53 internal `wp_cache_setting()` calls to `Config::set()` for consistency. The function itself stays as a public back-compat alias. Cosmetic; can be dropped if not worth the churn.

### Phase D — Document

11. **Document the module and the contract.** Update `CONTEXT.md` and add an ADR stating: all config writes go through `Config`; the globals/`$GLOBALS` remain a permanent, supported read API; reads are intentionally **not** migrated; and the config file stays a flat pre-WordPress PHP file. Note the locking decision and its rationale.

## Decision Document

- **Write path only.** This pass centralises and tames *writes*. The ~193 read sites (`global $foo;`, `$GLOBALS['foo']`, settings-map dynamic reads) are untouched.
- **Globals are permanent back-compat.** `Config` always populates `$GLOBALS[$field]` on write, exactly as `wp_cache_setting()` does today. Existing installs' `wp-cache-config.php` files and third-party cache plugins (e.g. bundled `plugins/multisite.php`, `wptouch.php`, `domain-mapping.php`, and the external `wp-super-cache-plugins` ecosystem) that read these globals keep working indefinitely. The globals are treated as a supported read API, not deprecated.
- **The config file stays a flat PHP file loaded pre-WordPress.** The drop-in (`wp-cache-phase1.php`) `@include`s it before WP boots, with no database available. It cannot move to `wp_options`. `Config` reads/writes that same file format.
- **`wp_cache_setting()` and `wp_cache_replace_line()` remain global functions** as delegating shims, because they are callable from third-party code. They are not removed.
- **No new locking or concurrency handling.** Per maintainer input: writes happen only in wp-admin, effectively always a single human changing one setting, and the path has been stable for years. The existing atomic `tempnam`+`rename` is preserved; file locking is explicitly out of scope.
- **The module builds on the existing settings map.** `rest/class.wp-super-cache-settings-map.php` already maps setting → `{global, option, get/set method}`; the `Config` key set aligns with it rather than inventing a parallel schema.
- **No runtime autoloader.** Confirmed in-code ("WPSC doesn't use an autoloader or composer"). `Config` is loaded by explicit `require_once` at runtime (mirroring `plugins/jetpack.php`'s loading of the device-detection class) and is classmap-autoloaded only under test.
- **Depends on #1061.** Phase C clusters map onto the post-split `inc/` files, so the file move lands first.

## Testing Decisions

- **A good test here asserts the on-disk result and the populated global, not internals.** The binding contract is "the config file ends up in this exact state and `$GLOBALS[$field]` holds this value." Tests write to a temp file and assert file contents + global state for representative value types and rewriter branches.
- **The write path is the strongest unit-test target in the plugin** — it is nearly pure (a file in, a file out) once isolated. Characterization tests are written *before* the module exists (Phase A) and re-run unchanged against the module (Phase B) to prove byte-identical behaviour.
- **Modules tested:** the `Config` writer and value formatter, plus the `wp_cache_setting()` / `wp_cache_replace_line()` delegations. The per-caller migrations in Phase C are covered by the same suite plus the existing e2e settings specs (`tests/e2e/specs/settings/`).
- **Tiering (per #1051):** these config write tests touch WP runtime helpers (e.g. `wp_rand`, transients via `set_transient`), so they run in the **local integration tier** via the Makefile target against the Dockerized database — not CI. CI stays lint + a no-database smoke suite. (If the writer's WP dependencies are trivially stubbable, individual cases may drop to the smoke tier, but assume integration by default.)
- **Prior art:** the integration test harness established in #1051 (run locally via the Makefile target) and the e2e settings tests as the behavioural backstop.

## Out of Scope

- **Read migration.** Converting `global $foo;` / `$GLOBALS['foo']` reads to `Config::get()` is a separate, much larger and riskier effort touching the engine and third-party compat. Not included.
- **Removing or deprecating the globals.** They remain a permanent read API.
- **Moving config off the flat PHP file** (e.g. into `wp_options` or a serialized store). Impossible given the pre-WordPress drop-in load.
- **File locking / concurrency safety.** Explicitly declined (single-human admin writes).
- **The REST read pattern.** `rest/class.wp-super-cache-rest-get-settings.php` and `-get-status.php` re-`include()` the config file to refresh globals on read; that is the read path and is left as-is (the module could own a reload later).
- **Any engine/serving logic** in `wp-cache-phase2.php` beyond the two write shims.

## Further Notes

- This is item #1 of the #1047 umbrella and is the recommended foundation for the later `CacheKey` (#2) and `Preload` state machine (#5) work, since those will want a clean config surface.
- Highest-risk commits are the Phase C migrations where a direct `wp_cache_replace_line()` call did something subtler than a key/value set; the explicit "exceptions stay on the raw API" rule contains that.
- Sequencing rationale: the maintainer chose to do the file move (#1061) before this, even though #1047 suggested config-first — fine, because the conservative write-only scope here doesn't require chasing reads, and the post-split files make the Phase C clusters cleaner.
- **Constraint for the future `Config::get()` read API (out of scope here, but recorded so it isn't lost):** a `get()` will be added later, but the globals must remain because third-party code depends on them — so `get()` and the globals must be a *single* source of truth, not two. `Config::get($key)` must be a typed read-through accessor over `$GLOBALS` (the canonical store, populated from the flat file pre-WordPress), **not** a value served from a private copy that `Config` parsed separately. Otherwise a third-party plugin that writes a global directly would be invisible to `get()` (and vice versa) and the two would drift. This is symmetric with the write side, which already keeps `$GLOBALS` authoritative. Casting/typing belongs inside `get()`, since the globals hold whatever raw PHP values the config file's `include` produced.

Contributor guide

No contributing guide indexed for this repository

Research direction

Confirm the prerequisite wp-cache.php move in #1061, then read the existing wp_cache_setting() and wp_cache_replace_line() paths and the loading points in wp-cache.php and the phase2 shims. Start with characterization tests against a temporary config file, using the local integration Makefile target and existing tests/e2e/specs/settings/ as references. Done means all writes have one tested Config owner while globals and public shim behavior remain byte-identical.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
backend, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.