# Plugin Revision — v1.4.2 (Fresh-Eye Analytical Review)

> **Status as of 1.4.3 (2026-05-12):** every in-scope finding below has been resolved in plugin version **1.4.3**. Each finding now carries an inline status marker — ✅ resolved, ⏸ deliberately deferred. See the README changelog for the corresponding code changes.

**Scope:** Independent read-only analytical pass over the v1.4.2 codebase. Re-read all eight PHP files from scratch and looked for issues regardless of whether they'd appeared in earlier reviews. This time I deliberately compared paired code paths (the two import handlers, the two cap-checks, the listener-vs-bulk stamps) and walked through user scenarios that the previous reviews didn't exercise.

**Reviewer date:** 2026-05-12
**Target environment:** WordPress 6.9.4, WooCommerce 10.7.0 (HPOS enabled), PHP 7.2+.
**Files reviewed:** all PHP under `includes/`, the bootstrap `studiou-wc-ord-print-statuses.php`, and the markdown docs.
**Verification limitations:** Static review only. No live PHP/WP/WC available.

Severity reflects user-visible or correctness impact, not implementation effort.

---

## 1. Critical

*(None.)*

---

## 2. High

### 2.1 `import_delivered_protocol` does not skip terminal-status orders — asymmetry with `import_inprint_protocol` — ✅ resolved in 1.4.3

Resolution: added a `has_status(['cancelled', 'refunded', 'failed'])` guard with the same per-row error and `UtilsLog::log` call as the InPrint path. `completed` is intentionally excluded — re-running a delivered protocol on already-completed orders is a benign no-op.

`class-db-manager.php:200-230` (InPrint) and `:238-258` (Delivered).

The InPrint import explicitly skips orders in `cancelled`, `refunded`, `failed`, or `completed` states:

```php
if ($order->has_status(self::TERMINAL_STATUSES)) {
    $errors[] = sprintf(__('Order %s is in a final state and was not updated.', …), $row['order_no']);
    continue;
}
$order->update_status('in-print');
```

The Delivered import does not:

```php
$order = wc_get_order($row['order_no']);
if (!$order) { /* ... */ continue; }
// ← no terminal-status guard here
$order->update_status('completed');
$order->update_meta_data('external_ref_ord_delivered', current_time('mysql'));
```

**Concrete failure modes:**

- A `cancelled` order that appears in the print shop's delivered CSV (operator error, stale data, ghost entry) is silently **reactivated to `completed`**.
- A `refunded` order is reactivated to `completed`. The refund accounting in WC is now inconsistent with order state.
- A `failed` order is reactivated to `completed`. Same problem.

Whether the asymmetry is intentional cannot be inferred from the spec (`docs/instructions.txt` line 32-36 does not address terminal states). It looks accidental — the cancellation paths in `bulk_set_print_status` and `import_inprint_protocol` are clearly defensive, and `import_delivered_protocol` was implemented to a parallel template but the guard wasn't ported.

**Suggested fix:** mirror the InPrint guard. Optionally skip `completed` from the list since `completed → completed` is a no-op transition anyway and operators may legitimately re-deliver the same protocol.

### 2.2 Import handlers require `manage_woocommerce`, bulk actions require `edit_shop_orders` — shop managers cannot use the import screen — ✅ resolved in 1.4.3

Resolution: both `add_submenu_page()` and the `current_user_can` check in `run_import()` now use `edit_shop_orders`. Shop Managers see and can use the Import Protocols screen, matching the bulk-action capability requirement.

`class-import-manager.php:106` (`current_user_can('manage_woocommerce')`) vs `class-bulk-actions-manager.php:28` (`current_user_can('edit_shop_orders')`).

WC's default capabilities:
- The **Shop Manager** role grants `edit_shop_orders` (and most order-management caps) **but not** `manage_woocommerce`.
- `manage_woocommerce` is the WC administrator cap, normally given only to the Administrator role.

So a Studiou employee with the Shop Manager role can:
- Run the three bulk actions (`current_user_can('edit_shop_orders')` ✓).
- Edit the Print Information panel on the order edit screen (`current_user_can('edit_shop_order', $order_id) || 'edit_shop_orders'` ✓).

But they cannot:
- Open **WooCommerce → Import Protocols** (`manage_woocommerce` ✗) — `add_submenu_page` hides the page itself, and `wp_die` blocks the POST handler.

For day-to-day print-shop ops this means every protocol import has to go through an administrator, which is almost certainly not intended. Either:

- Lower the import requirement to `edit_shop_orders` to match the bulk-action requirement, **or**
- Raise the bulk-action requirement to `manage_woocommerce` and document the policy intent.

The first option is the operationally pragmatic one; the second would be the more locked-down choice.

---

## 3. Medium

### 3.1 `table_exists($table)` doesn't escape MySQL LIKE wildcards in `$wpdb->prefix` — ✅ resolved in 1.4.3

Resolution: `$wpdb->esc_like($table)` is applied before `$wpdb->prepare()`. The post-query equality check stays as a belt-and-braces safeguard.

`class-db-manager.php:128-132`:

```php
private static function table_exists($table) {
    global $wpdb;
    $found = $wpdb->get_var($wpdb->prepare('SHOW TABLES LIKE %s', $table));
    return $found === $table;
}
```

`$wpdb->prepare(..., '%s')` quotes the string for SQL, but `SHOW TABLES LIKE` interprets the value as a LIKE pattern where `_` is a single-character wildcard and `%` is a multi-character wildcard. `$wpdb->prefix` is conventionally alphanumeric + underscore (e.g., `wp_`, `wp_test_`), so the underscores in the prefix become wildcards.

For prefix `wp_test_`, the LIKE pattern `wp_test_wc_order_product_lookup` matches:

- `wp_test_wc_order_product_lookup` ✓ (intended)
- `wpXtestXwcXorderXproductXlookup` (each `_` matches any char)
- `wp1test2wc3order4product5lookup`

The strict `=== $table` comparison after the query mitigates false positives — if a wildcard match returns a different table name, the equality check rejects it. **So today this is safe**, but the safety relies on the post-query comparison rather than on the query itself returning at most the intended row. Two ways the post-comparison could miss:

1. If two tables happen to match the pattern and one is `wp_test_wc_order_product_lookup` exactly, MySQL returns multiple rows; `$wpdb->get_var` returns the first one. Order is engine-dependent.
2. If a future contributor moves `table_exists` to a context where multiple matches matter (e.g., a `tables_like` helper), the wildcard semantics will silently misbehave.

**Suggested fix:**

```php
$found = $wpdb->get_var($wpdb->prepare('SHOW TABLES LIKE %s', $wpdb->esc_like($table)));
```

`$wpdb->esc_like()` escapes `%` and `_` for LIKE pattern use.

### 3.2 `set_order_processing_status` is effectively dead code on modern WC — ✅ resolved in 1.4.3

Resolution: removed the handler and its `woocommerce_payment_complete` hook registration. The corresponding translation entry in `docs/translations.txt` was removed too. If a future custom payment gateway is found to fire the action without first setting the status, the handler can be reintroduced with an explanatory comment.

`class-order-status-manager.php:112-129`. The handler is hooked to `woocommerce_payment_complete`, which is fired by `WC_Order::payment_complete()`. WC's own implementation of `payment_complete()`:

1. Determines the destination status based on order content (downloadable-only → `completed`, otherwise → `processing`).
2. Sets the order to that status **before** firing the action.

When our handler then runs:

- If WC set `processing`, our `update_status('processing')` is a no-op (same-status, WC short-circuits inside `set_status`).
- If WC set `completed` (downloadable-only orders), our `has_status(['completed', 'to-print', 'in-print'])` guard hits → we return.

In both cases the handler does nothing. It would only matter if a custom payment gateway fires `woocommerce_payment_complete` without first setting the order status — which is non-standard and arguably the gateway's bug.

Two options:

- **Remove it.** The hook serves no purpose on a default WC installation.
- **Keep it and document.** Add a comment explaining what scenario it defends against (legacy gateway behaviour from pre-WC-4.0 days?). Otherwise a future reader will be confused.

The order note string (`"Payment received — order automatically set to processing by …"`) appears in `docs/translations.txt` and is shipped translated — if the code is removed, the translation entry should follow it.

### 3.3 CSV injection — output values may trigger Excel formula execution — ✅ resolved in 1.4.3

Resolution: `Studiou_DB_Manager::csv_escape()` is applied to every cell value in the PHP post-processing loop. Cells beginning with `=`, `+`, `-`, `@`, `\t`, or `\r` get a leading single quote — hidden by Excel/LibreOffice display, prevents formula evaluation.

`class-bulk-actions-manager.php:113-115` writes CSV cells directly from the export-query result. CSV cells beginning with `=`, `+`, `-`, `@`, or `\t` are interpreted by Excel and LibreOffice Calc as formulas. If a customer's `billing_email`, product name, or category name happens to begin with one of these characters — accidentally or maliciously — the print shop opens the CSV in Excel and a formula executes.

Realistic vectors:

- A customer registers `=cmd|'/c calc'!A1` as their email. The export includes it as-is. Excel parses it as a formula.
- A product category gets renamed to `=HYPERLINK("https://evil/", "click")` by a compromised admin or contractor. The export embeds it; opening the CSV silently navigates the cell-link target.

The mitigation in WP-aware code is to prefix any cell value starting with a dangerous character with a leading single-quote (`'`) or with a leading `\t`-equivalent zero-width-width control. The OWASP-recommended pattern is the leading single-quote:

```php
private function csv_escape($value) {
    $value = (string) $value;
    if ($value !== '' && in_array($value[0], array('=', '+', '-', '@', "\t", "\r"), true)) {
        return "'" . $value;
    }
    return $value;
}
```

Apply this to every cell before passing it to `fputcsv`. Excel still shows the value (without the leading quote) but does not evaluate it.

The risk is **low likelihood, real impact** — print shop spreadsheet users are a concrete attack surface for this class of vulnerability.

### 3.4 `datetime_local_to_mysql` accepts structurally-valid but semantically-out-of-range values — ✅ resolved in 1.4.3

Resolution: `checkdate()` validates the date components; explicit numeric range checks reject hour > 23, minute > 59, second > 59. Bogus values return empty string instead of being stored.

`class-order-fields-manager.php:108-118`:

```php
if (preg_match('/^(\d{4}-\d{2}-\d{2})T(\d{2}:\d{2})(?::(\d{2}))?$/', $value, $m)) {
    $seconds = isset($m[3]) ? $m[3] : '00';
    return $m[1] . ' ' . $m[2] . ':' . $seconds;
}
```

The regex matches `2026-13-45T25:99` as well as `2026-05-12T14:30`. The function then stores `2026-13-45 25:99:00` to postmeta. MySQL's behaviour for an out-of-range datetime depends on `sql_mode`:

- Strict mode: rejects the value, raises an error.
- Non-strict mode: silently coerces to `0000-00-00 00:00:00`.

Browsers' native `<input type="datetime-local">` validate the value before submit, so this is unlikely to come from a normal browser flow. But:

- A power user editing the form via DevTools, or
- A script POSTing directly to the order edit URL,

…can produce the bogus value. The downstream display (`mysql_to_datetime_local`) would reject the bogus stored value (the back-conversion regex would still match structurally but the data is meaningless) and show an empty field.

**Suggested fix:** validate via `checkdate()` and seconds/minutes/hours range before composing the MySQL string. Defense-in-depth.

---

## 4. Low

### 4.1 CSV uses `,` separator — Excel CS-locale users see single-column data — ⏸ documented, no code change

Resolution path: a `sep=,\n` directive at the top of the file (the Microsoft proprietary extension Excel respects) would break the export-then-reimport workflow because CSV parsers would see `sep=,` as the first header row. The README "Known limitations" section now documents the workaround (use Excel's Text Import wizard or change Windows regional list-separator).

The plugin exports with comma-separated values. The Czech edition of Excel defaults to `;` as the list separator (because comma is the decimal separator), so a Czech operator opening the file in Excel without changing the locale or pre-converting sees every row as one cell.

LibreOffice Calc prompts for the separator on import; Excel-CS does not. Workarounds the operator can apply: rename the file extension to `.txt` and use Excel's text-import wizard, or change Windows regional settings. Neither is ideal.

Options:

- Add a `\xEF\xBB\xBFsep=,\n` directive at the very top of the CSV (Microsoft proprietary extension; understood by Excel).
- Make the separator a settable plugin option.
- Document the locale workaround.

### 4.2 `SUM(order_products.product_qty)` returns DECIMAL — CSV shows `2.0000` for integer quantities — ✅ resolved in 1.4.3

Resolution: `Studiou_DB_Manager::format_quantity()` normalises whole numbers to integer string (`"2"`) and trims trailing zeros from decimals (`"2.5"` for `"2.5000"`).

`wc_order_product_lookup.product_qty` is `DECIMAL(8,4)` in the WC schema. `SUM()` of `DECIMAL` returns `DECIMAL`. `$wpdb->get_results()` returns strings. The CSV row therefore contains `2.0000` for an order of two items, not `2`.

The print shop's downstream tooling may or may not care. Cosmetic but minor friction.

**Suggested fix:** in the PHP post-processing step that already resolves `prod_img_url`, also normalise `$row->qty` — if the value is a whole number, drop trailing zeros and the decimal point.

### 4.3 `import_delivered_protocol::update_status('completed')` doesn't pass a note — ✅ resolved in 1.4.3

Resolution: both import handlers now pass an explanatory note as the second argument to `update_status` — "Marked In Printing by InPrint Protocol import." and "Marked completed by Delivered Protocol import." The order activity feed reflects how the transition was triggered.

`class-db-manager.php:252` — no second argument. WC writes a default-styled order note ("Order status changed from … to completed."). For traceability, passing a note like `"Marked completed by Delivered Protocol import."` would distinguish protocol-driven completions from manual ones. The InPrint and bulk paths similarly don't pass notes; an opportunity to add operational context across the board.

### 4.4 Import handlers don't log per-row to `UtilsLog::log` — ✅ resolved in 1.4.3

Resolution: both import handlers now write `UtilsLog::log` entries for not-found orders, terminal-status skips, and per-row success — symmetric with `bulk_set_print_status`.

`bulk_set_print_status` logs per-order success (`Order #N marked as "to-print"`) and per-order skip. The two import handlers log only at the bulk-action-level (via `UtilsLog::message`); they don't write to `error_log` per row. When debugging a failed import, the operator/developer cannot see which orders were touched and which weren't.

Asymmetric with the bulk path; consistency would help.

### 4.5 `Order_Status_Manager::set_order_processing_status` has no audit trail of why it was a no-op — ✅ resolved by removal in 1.4.3 (see §3.2)

When `set_order_processing_status` skips because of the protected-status guard (`completed`, `to-print`, `in-print`), it returns silently. Adding `UtilsLog::log` would help debug "why didn't payment_complete advance my order to processing?" support questions.

Related to §3.2 — if the handler is kept as-is, instrument it; if removed, this is moot.

### 4.6 `array_filter` without callback removes order ID 0 — ✅ resolved in 1.4.3

Resolution: `array_filter` now uses an explicit `function ($id) { return $id > 0; }` callback. Intent is unambiguous in the source.

`class-db-manager.php:42` — `array_filter(array_map('intval', (array) $order_ids))`. `array_filter` with no callback drops all falsy values, including integer `0`. WC orders cannot have ID 0 in practice, so this is harmless, but it's slightly misleading — a future contributor adapting this for `WP_Term` IDs or anything where 0 is a legitimate value would inherit the bug.

Cosmetic.

### 4.7 Trashed products still appear in the export — ✅ documented in README "Known limitations"

Resolution path: this is intentional — historical orders should reflect what was sold. The README "Known limitations" section now makes that explicit. No code change.

The `JOIN posts products ON products.ID = order_products.product_id` doesn't filter on `post_status`. A product whose post has been moved to trash (or set to draft) still produces a CSV row for any historical order line item. The print shop sees the product name as it was at the time of the trash, not what the customer originally saw.

Probably intended — historical orders should reflect what was sold, not the current product state — but worth documenting.

### 4.8 `import_inprint_protocol` and `import_delivered_protocol` emit one `Invalid row format in CSV.` error per malformed row — ✅ resolved in 1.4.3

Resolution: `Import_Manager::emit_import_summary()` runs `array_unique()` on the error list before formatting the notice. The numeric total still reflects the full error count.

`class-db-manager.php:206, 244`. If a CSV has 100 malformed rows, the error list has 100 identical strings. The Import Manager's `emit_import_summary` joins them with `, ` and shows the count — but the joined string can become very long. A simple `array_unique` on the errors list or a per-error-kind counter would tighten the UX.

### 4.9 `wp_kses_post()` allows `<a href>` — admin notice content could include arbitrary links — ✅ resolved in 1.4.3

Resolution: `UtilsLog::render_notices()` now uses `esc_html()` for the notice body. All current callers pass plain text; future callers wanting markup will need to pre-format with their own escape strategy.

`utils-log.php:66`. None of the current call sites pass HTML containing user-controlled links, but the `message()` API doesn't restrict what callers can pass. If a future caller passes a partially-escaped string with a user-controlled URL, `wp_kses_post` would allow `<a href="...">` through. The current set of callers builds notices from sprintf-of-translated-strings plus `esc_html`'d data, so no live risk — but the trust boundary is the API not the call sites.

A narrower allowlist via `wp_kses($html, array())` for plain-text-only or `wp_kses($html, array('code' => array(), 'strong' => array(), 'em' => array()))` would be defense-in-depth.

### 4.10 `register_post_status` keeps being called even though HPOS doesn't read it — ⏸ left as-is

Carried over from `revise-1.4.1.md` §4.8 — harmless under HPOS, kept for admin-chrome compatibility. No change.

---

## 5. Verification gaps (carried over from earlier reviews)

1. **HPOS search filter name** `woocommerce_order_table_search_query_meta_keys` against WC 10.7.0 source — still pending live verification.
2. **`woocommerce_process_shop_order_meta`** hook behaviour under WC 10.7.0 block-based order edit screen — pending live verification.
3. **Object-cache scenarios for `UtilsLog::message`** — pending live verification under Redis/Memcached configurations.

---

## 6. Status of prior reviews

| Review | Status of in-scope findings | Status of deferred findings |
|--------|------------------------------|------------------------------|
| `revise-1.3.2.md` | All resolved in 1.4.0 | 3 still deferred. |
| `revise-1.4.0.md` | All resolved in 1.4.1 | 1 verification-pending (HPOS filter). |
| `revise-1.4.1.md` | All resolved in 1.4.2 | 2 documented-without-code-change. |

No regressions detected. The new findings in this review (§2.1, §2.2) are pre-existing defects that survived three prior reviews because the paired code paths were never compared side-by-side.

---

## 7. Risk-prioritised summary

Ordered by user-visible severity. Status as of **1.4.3**:

1. **§2.1 Delivered import reactivates cancelled/refunded orders to `completed`.** ✅ resolved.
2. **§2.2 Shop managers cannot run protocol imports.** ✅ resolved.
3. **§3.3 CSV injection in exports.** ✅ resolved.
4. **§3.1 `SHOW TABLES LIKE` wildcard escape.** ✅ resolved.
5. **§3.2 `set_order_processing_status` is dead code on modern WC.** ✅ resolved (removed).
6. **§3.4 `datetime_local_to_mysql` accepts out-of-range structurally-valid values.** ✅ resolved.
7. **§4.1 CSV `,` separator vs Czech Excel locale.** ⏸ documented in README "Known limitations".
8. **§4.2 `SUM(qty)` produces `2.0000` not `2`.** ✅ resolved.

---

## 8. Out of scope

- Live behaviour testing against a running WP 6.9.4 / WC 10.7.0 install.
- Performance profiling of the export query under realistic order volumes.
- Refactoring the manager pattern into typed DI / interfaces (PHP 7.4+).
- Multisite, uninstall cleanup, block-based order edit screen (carried-over deferrals).

---

*End of review.*