litespeedtech / litespeedtech/lscache_wp

[API] `litespeed_save_conf` gives callers no write outcome - `Conf::update()` is void, drops WP's `update_option()` return, and never reads back

Open
#1,047 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
PHP
Stars
257
Forks
123
PR merge metrics
No merged PRs in 30d

Description

`litespeed_save_conf` (added in 7.2) is the supported way to write LSCWP configuration from code. A caller cannot determine whether a save was persisted, silently rejected, or coerced. The path returns nothing at every level, never reads back from the database, and emits no outcome event.

This is a feature request about write-outcome signalling, and it is **independent of the object cache** - it reproduces identically on an install with no object cache at all.

---

## What happens

**The hook shape forecloses an answer.** `src/api.cls.php:74`:

```php
74 add_action( 'litespeed_save_conf', [ $this, 'save_conf' ] );
```

Registered as an action, not a filter - `do_action()` discards callback returns by language contract.

**The handler returns nothing.** `src/api.cls.php:317-319`:

```php
317 public function save_conf( $the_matrix = false ) {
318 $this->cls( 'Conf' )->update_confs( $the_matrix );
319 }
```

**Neither does anything below it.** `Conf::update_confs()` (`src/conf.cls.php:473`, docblocked `@return void` at `:471`) and `Conf::update()` (`:539`, `@return void` at `:537`).

**`Conf::update()` has four silent early returns, none observable by the caller:**

```php
545 if ( self::_VER === $id ) { return; }

549 if ( self::O_SERVER_IP === $id ) {
550 if ( $val && ! Utility::valid_ipv4( $val ) ) {
551 $msg = sprintf( __( 'Saving option failed. IPv4 only for %s.', ... ) );
552 Admin_Display::error( $msg ); // a wp-admin notice
553 return;
554 }
555 }

557 if ( ! array_key_exists( $id, self::$_default_options ) ) {
558 if ( defined( 'LSCWP_LOG' ) ) {
559 Debug2::debug( '[Conf] Invalid option ID ' . $id );
560 }
561 return;
562 }

564 if ( $val && $this->_conf_pswd( $id ) && ... ) { return; } // all-asterisk password value
```

The `:549` rejection surfaces only as an admin notice, which goes nowhere for a REST, WP-CLI or cron caller. The `:557` rejection - a typo'd option id - leaves **no trace at all** unless `LSCWP_LOG` happens to be defined, and is indistinguishable from a successful write.

**The one durability signal WordPress hands back is dropped on the floor.** `src/root.cls.php:571-573`:

```php
571 public static function update_option( $id, $v ) {
572 update_option(self::name($id), self::_maybe_encode($v));
573 }
```

WP's `update_option()` returns `bool`. It is not captured, not returned, not logged. `Root::add_option()` (`:553-555`) does the same. This single line is where the persistence question dies.

**There is no read-back.** The only post-write comparison is `src/conf.cls.php:586`:

```php
583 self::update_option( $id, $val );
...
586 if ( $this->conf( $id ) !== $val ) {
...
604 $this->set_conf( $id, $val );
```

`Root::conf()` (`src/root.cls.php:430`) reads the static in-memory `self::$_options` array - not the database - and `:604` updates that array **unconditionally**, regardless of whether the row landed. So your own canonical read path, `apply_filters( 'litespeed_conf', $key )` (wired at `src/api.cls.php:70`), returns the new value for the remainder of the request either way. Same-request verification through the public API is structurally incapable of detecting a failed write.

**The one hook that fires is not an outcome signal.** `src/conf.cls.php:505`:

```php
473 public function update_confs( $the_matrix = [] ) {
474 if ( $the_matrix ) {
475 foreach ( $the_matrix as $id => $val ) {
476 $this->update( $id, $val );
477 }
478 }
...
505 do_action( 'litespeed_update_confs', $the_matrix );
```

Three problems: it fires **unconditionally** - `API::save_conf()` called with its default `false` skips the loop at `:474` and still reaches `:505`, so "nothing was attempted" and "everything persisted" emit the identical event; it carries the **requested** matrix rather than what changed; and `$_updated_ids` (declared `private` at `:30`, appended at `:587`) is never passed to it.

**wp-admin is equally blind.** `src/admin-settings.cls.php:334-337` calls the same `update_confs()` and then prints `Options saved.` unconditionally. This is not an API-only gap - the UI reports the same unverified success.

## The contrast is already in your codebase

The purge subsystem has exactly the shape the conf path lacks. `src/purge.cls.php` carries **26** past-tense `litespeed_purged_*` actions, fired only on the successful path:

```php
251 do_action( 'litespeed_purged_all' );
968 do_action( 'litespeed_purged_link', $url ); // only after both validation gates
```

`purge_url()` returns silently **without** firing when it rejects the input, so the hook is a genuine success signal - we consume it in production today.

Meanwhile:

```
$ grep -rn "do_action( 'litespeed_[a-z_]*conf" src/
src/conf.cls.php:505: do_action( 'litespeed_update_confs', $the_matrix );
```

One hook, present-tense, unconditional. There is no `litespeed_conf_updated`, no `litespeed_confs_saved`, nothing per-key, nothing carrying `_updated_ids`.

## What should happen

1. `Root::update_option()` / `Root::add_option()` return WP's `bool` (`src/root.cls.php:553-555`, `:571-573`).
2. `Conf::update()` returns an outcome instead of `void`, so the four early returns at `:545`, `:549-555`, `:557-562`, `:564-566` become reportable.
3. `Conf::update_confs()` returns a per-id map - written / unchanged / rejected-with-reason.
4. Add a past-tense `do_action( 'litespeed_confs_updated', $updated_ids, $failed_ids )`, modelled on `litespeed_purged_all` (`src/purge.cls.php:251`), fired only when writes actually occurred.
5. Because `do_action()` cannot carry a result back at all, an `apply_filters( 'litespeed_save_conf', ... )` variant alongside the action would make this usable from REST, CLI and cron contexts where `Admin_Display::error()` goes nowhere.

Even items 1 and 4 alone would be enough for external tooling.

## Environment

- LiteSpeed Cache 7.9.1 (stock, hash-identical to the shipped ZIP), WordPress 6.9.7, PHP 8.3.33
- Independent of Object Cache state by construction: `Root::update_option()` calls WP's `update_option()` directly and nothing on this path consults the cache. The plugin files are byte-identical on our installs with and without a drop-in.
- Conf rows are autoloaded: `SELECT autoload, COUNT(*) ... WHERE option_name LIKE 'litespeed.conf.%'` returns `auto / 191`, consistent with `Root::update_option()` passing no `$autoload` argument

## Minimal reproduction

```php
$key = 'optm-js_exc'; // bare option ID, not the litespeed.conf.* row name
$new = [ 'jquery.js', 'repro-' . time() . '.js' ];

$ret = do_action( 'litespeed_save_conf', [ $key => $new ] ); // always null

// (A) your canonical read path - shows the new value whether or not it persisted
var_dump( apply_filters( 'litespeed_conf', $key ) );

// (B) the only truthful read
global $wpdb;
var_dump( $wpdb->get_var( $wpdb->prepare(
"SELECT option_value FROM {$wpdb->options} WHERE option_name = %s",
'litespeed.conf.' . $key
) ) );
```

Nothing in `$ret`, in the `litespeed_update_confs` event, or in any other exposed signal distinguishes a persisted write from one that was not. (A) and (B) are the only two views available, and (A) cannot disagree with the request.

**Silent rejection, same session:**

```php
do_action( 'litespeed_save_conf', [ 'not-a-real-key' => 1 ] );
```

Takes the `array_key_exists` early return at `src/conf.cls.php:557-562` and emits nothing unless `LSCWP_LOG` is defined - yet `litespeed_update_confs` fires at `:505` exactly as on a successful save, and wp-admin would print `Options saved.`

## What would let us delete our workaround

Our MCP tooling performs a direct `$wpdb` read of `litespeed.conf.` after every save and returns a `persisted` boolean to the caller, purely because the API offers no way to ask. Any of items 1-4 retires that read-back entirely; item 3 would additionally let us surface *why* a key was rejected instead of reporting a generic failure.

## Related

- #1045 and #1046 (ours) - the same argument on the purge path: an API that reports a success it did not verify, and gives the caller no read side. This is that thesis applied to configuration writes, and we would rather it be seen as one coherent line than three unrelated complaints.
- #480 - a reporter unable to tell whether `Purge::purge_all()` did anything, resorting to reading the debug log. Same gap, four years old.
- This report is deliberately scoped to the conf write path only. It needs no object cache to reproduce, and makes no claim about one.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with src/api.cls.php, src/conf.cls.php, and src/root.cls.php, tracing litespeed_save_conf through update_confs(), update(), and WordPress update_option(). Compare the successful-event pattern in src/purge.cls.php and review the listed early returns and existing litespeed_update_confs hook. Done should expose write outcomes and distinguish successful, unchanged, and rejected configuration saves for API callers.

Written by the indexing model from the issue text.

Assessment

Tech stack
php, wordpress
Domain
api, backend
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.