qdr.app.woo.plugins/studiou-wc-ord-print-statuses/docs/revise-1.4.2.md

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:

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:

$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:

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:

$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:

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:

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.