revise-1.4.0.md 26 KB

Plugin Revision — v1.4.0 (Analytical Review)

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

Scope: Read-only analytical pass over the v1.4.0 codebase. No code changes were made as part of this review; the resolution was implemented separately and is recorded in the status markers below. This document is the counterpart of revise-1.3.2.md — it catalogues residual gaps, possible syntax pitfalls, regressions newly introduced by the 1.4.0 fix-up, and items from the prior review that remain open.

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/, plus studiou-wc-ord-print-statuses.php, README.md, CLAUDE.md, docs/translations.txt. Verification limitations: No PHP interpreter and no WordPress / WooCommerce install were available in the working environment, so syntax errors visible only at compile time are not ruled out and WC-internal filter names / hook semantics could not be cross-checked against a running site. The findings below reflect WC 10.7.0 / WP 6.9.4 as the target but are derived from source reading, not live behaviour.

Findings are grouped by severity. The severity reflects user-visible or correctness impact, not implementation effort to fix.


1. Critical — newly introduced regressions

1.1 Export SQL GROUP BY may fail under default MySQL strict mode — ✅ resolved in 1.4.1

class-db-manager.php:35-76 — the new export query introduces GROUP BY orders.id, order_products.product_id, order_products.variation_id so that products with multiple categories no longer duplicate rows. The SELECT list, however, contains several non-aggregated columns that are not in the GROUP BY clause:

  • products.post_title
  • product_variations.post_title
  • product_variations_name.name
  • images.guid
  • order_products.product_qty
  • orders.billing_email

Under MySQL 5.7+ and MariaDB 10.3+ the default sql_mode includes ONLY_FULL_GROUP_BY. MySQL does recognise functional dependency on a primary key column, so products.post_title is technically dependent on order_products.product_id (which references products.ID). In practice the recognition is brittle: it depends on engine version, whether the FK is declared, the optimiser's understanding of the join, and the exact sql_mode.

Risk: the bulk export may fail with ERROR 1055 (42000): Expression #N of SELECT list is not in GROUP BY clause and contains nonaggregated column ... on installations that follow the MySQL default. The previous v1.3.2 query had no GROUP BY and was immune.

Mitigation paths (informational only):

  • Wrap every non-aggregated column in ANY_VALUE(...) (MySQL 5.7+) or MIN(...).
  • Add every non-aggregated column to GROUP BY explicitly.
  • Reduce the join with a sub-query that pre-aggregates product_cat, then join in the rest without GROUP BY.

1.2 CSV exports now write a UTF-8 BOM; CSV imports do not strip it — ✅ resolved in 1.4.1

class-bulk-actions-manager.php:136 writes a leading UTF-8 BOM (\xEF\xBB\xBF) so Excel on Windows interprets the file as UTF-8. The matching read side Studiou_DB_Manager::parse_csv() (class-db-manager.php:224-248) does not strip a BOM from the first byte of the first header.

Symptom: if a user exports prepare_to_printing_export.csv, edits it, and re-imports it as a protocol, the first header is read as \xEF\xBB\xBForder_no instead of order_no. The header normaliser (normalise_header, line 250) lowercases and collapses whitespace but does not touch the BOM. Subsequent isset($row['order_no']) checks fail for every row, the import emits "Invalid row format in CSV." for the entire file, and no orders are updated.

This combination is realistic — exports often serve as templates for the next stage. Stripping a BOM is a single-line fix in parse_csv.


2. High — correctness, HPOS, time-zone

2.1 HPOS search filter name is unverified against WC 10.7.0 — ⏸ deferred (verification task)

class-order-search-manager.php:20 hooks woocommerce_order_table_search_query_meta_keys. The legacy CPT filter woocommerce_shop_order_search_fields (line 17) is well-established. The HPOS filter's exact name has varied across WC releases and could not be confirmed against WC 10.7.0's source without access to a running install.

Risk: if the filter name is wrong (or has been renamed in the active WC 10.7.0 build), search by external order number works only under legacy CPT, not under HPOS — which is exactly the failure mode the v1.3.2 review flagged as critical.

Verification ask (WC 10.7.0): confirm the filter still exists, e.g. by grep -RIn 'woocommerce_order_table_search_query_meta_keys' wp-content/plugins/woocommerce/, or by adding a temporary error_log inside the callback and observing whether it fires on the orders page with HPOS enabled. If the filter has been renamed, alternatives to consider in WC 10.x are woocommerce_order_query_args / woocommerce_orders_table_query_clauses for injecting meta_query at a higher level.

2.2 Datetime fields use server timezone, not WP timezone — ✅ resolved in 1.4.1

Resolution: switched Order_Fields_Manager to pure-string round-tripping (no strtotime()/date()). The form value and the stored value are now both treated as WP local time, matching current_time('mysql').

class-order-fields-manager.php:90 and :99 use date('Y-m-d\TH:i', $timestamp) and date('Y-m-d H:i:s', $timestamp) for the datetime-local round-trip. PHP's date() formats with the server's PHP timezone, not WordPress's site timezone.

Elsewhere the plugin stamps timestamps with current_time('mysql') — which honours wp_timezone() / get_option('timezone_string'). As a result:

  • Bulk action and import paths stamp to_print_date etc. in WP local time.
  • Manual edits via the Print Information panel display and save in server local time.
  • If the server is UTC and WP timezone is Europe/Prague (UTC+1 or +2 with DST), the panel shows values shifted by one or two hours.

Replacing date(...) with wp_date(...) or constructing a DateTimeImmutable with wp_timezone() would unify behaviour.

The orders-list column (class-custom-columns-manager.php:58) already does this correctly via date_i18n(...) — so the discrepancy is between the column display and the editable panel.

2.3 Status-change listener and bulk-set both stamp the same meta — ✅ resolved in 1.4.1 (documented as intentional)

Resolution: the listener and the bulk/import paths intentionally coexist — the listener owns manual single-order transitions, the bulk/import paths always re-stamp. A docblock above handle_status_transitions now explains the contract.

Order_Status_Manager::handle_status_transitions() (lines 71-86) is triggered by every woocommerce_order_status_changed event. Studiou_DB_Manager::bulk_set_print_status() (lines 108-146) and Studiou_DB_Manager::import_inprint_protocol() both call $order->update_status(...) which fires that event, and also explicitly call $order->update_meta_data($meta_key, current_time('mysql')).

End result per status change via the bulk/import path:

  1. update_status → fires woocommerce_order_status_changed → listener stamps meta (only if currently empty).
  2. Caller then overwrites meta with another current_time('mysql') (potentially a millisecond later).
  3. Single save() persists the second value.

The values are nearly identical, so this is not a correctness bug, but it is wasted work and a maintenance trap: future contributors changing one path may miss the other. The two paths should agree on a single source of truth — either remove the explicit stamping in the bulk/import path (let the listener handle it) or have the listener defer to the bulk/import path when the operation is bulk.

A subtler asymmetry: the listener stamps only if empty (if (!$order->get_meta($meta_key))), the bulk path always stamps. On a second-pass bulk action, the bulk path overwrites; the listener's "only if empty" guard is bypassed for the bulk path but enforced for manual transitions. That divergence is intentional in one direction (bulk should re-stamp on each run) but should be commented in the code.


3. Medium — robustness, maintainability

3.1 GROUP_CONCAT is silently bounded by group_concat_max_len — ✅ resolved in 1.4.1

Resolution: SET SESSION group_concat_max_len = 65535 is issued before the export query.

class-db-manager.php:38 uses GROUP_CONCAT(DISTINCT t.name ORDER BY t.name SEPARATOR ', '). The MySQL default for group_concat_max_len is 1024 bytes; once exceeded the result is silently truncated with no error returned. Products belonging to many or long-named categories may lose the tail of prod_cat without warning.

Either raise the limit with SET SESSION group_concat_max_len = 65535 before the query, or accept the truncation and document the limit.

3.2 parse_csv does not skip the UTF-8 BOM — ✅ resolved in 1.4.1 (see §1.2)

Related to §1.2. Even outside of the export-and-reimport workflow, any CSV authored in Notepad with "UTF-8 with BOM" encoding will hit the same issue. A single-line guard:

$first = fread($handle, 3);
if ($first !== "\xEF\xBB\xBF") {
    rewind($handle);
}

inside parse_csv would close this gap.

3.3 output_csv fallback path mixes CSV into HTML on header-sent edge — ✅ resolved in 1.4.1

Resolution: when headers_sent() returns true, the source file/line is logged and the request wp_die()s with a clear message instead of streaming CSV bytes into the HTML body.

class-bulk-actions-manager.php:130-138:

private function output_csv($csv_data, $filename) {
    if (!headers_sent()) {
        nocache_headers();
        header('Content-Type: text/csv; charset=utf-8');
        header('Content-Disposition: attachment; filename="' . $filename . '"');
    }
    echo "\xEF\xBB\xBF" . $csv_data;
    exit;
}

If headers have already been emitted (an upstream plugin printed something, a print_r left over from debugging, BOM in an included file…) the CSV is echoed without the right Content-Type. The browser will render it as part of the HTML page. Better: when headers_sent() returns true, log the event, queue an error notice via the new transient mechanism (problematic since we're about to exit), or wp_die() with a clear message instead of streaming garbage.

3.4 if ($file === null) { $this->redirect_back(); } is statically unreachable-friendly — ✅ resolved in 1.4.1

Resolution: run_import() now uses return $this->redirect_back() on every non-happy branch.

class-import-manager.php:88-91:

$file = $this->validated_upload($file_field);
if ($file === null) {
    $this->redirect_back();
}
$csv_data = Studiou_DB_Manager::parse_csv($file['tmp_name']);

redirect_back() ends with exit; so the code that follows is correctly unreachable, but static-analysis tools (Psalm, PHPStan) will flag $file['tmp_name'] as a possible null-index access because the function signature on redirect_back() does not advertise it as a noreturn. Re-shaping as if ($file === null) { return $this->redirect_back(); } (or annotating @return never) makes intent explicit and silences the warning.

3.5 normalise_csv_date returns the raw string on parse failure — ✅ resolved in 1.4.1

Resolution: parse failures are logged and the function returns '' so downstream UI is consistent (blank rather than "looks blank but has bogus data").

class-db-manager.php:271-278:

private static function normalise_csv_date($raw) {
    $raw = sanitize_text_field((string) $raw);
    if ($raw === '') { return ''; }
    $timestamp = strtotime($raw);
    return $timestamp ? date('Y-m-d H:i:s', $timestamp) : $raw;
}

On parse failure the raw string is stored. Downstream, Order_Fields_Manager::mysql_to_datetime_local() runs strtotime again, fails, and displays empty — so the operator silently loses the value. Either log the failure, surface a per-row import warning, or store an explicit sentinel ('') so the field is consistently blank rather than "looks blank, but has bogus data underneath".

3.6 Unused constants and dead label maps — ✅ partially resolved in 1.4.1

Resolution: Order_Fields_Manager now drives both rendering and saving from the datetime_fields() / text_fields() maps. The STUDIOU_WC_OPS_VERSION / STUDIOU_WC_OPS_FILE constants remain defined but unused inside the plugin — they are intentionally exported for external consumers (test fixtures, sibling plugins, custom code) and left in place.

  • STUDIOU_WC_OPS_VERSION and STUDIOU_WC_OPS_FILE are defined in the bootstrap (studiou-wc-ord-print-statuses.php:22-23) but never referenced inside the plugin. They are useful for code outside the plugin (test code, migrations), but if nothing reads them they are effectively documentation.
  • Order_Fields_Manager::$datetime_fields and $text_fields map keys to English labels, but the labels are never read — the rendered labels are hardcoded inline in display_custom_order_fields(). Either drop the labels (just keep the key arrays) or drive display_custom_order_fields from the maps to eliminate the duplication.

3.7 set_order_processing_status produces two order notes per payment-complete event — ✅ resolved in 1.4.1

Resolution: the explicit add_order_note() call was removed; the explanatory note is now passed as the second argument to update_status().

class-order-status-manager.php:103-109:

$order->update_status('processing', __('Payment received, order is now processing.', ...));
$order->add_order_note(__('Order automatically set to processing by Studiou WC Order Print Statuses plugin.', ...));

The second argument to update_status is already a note. Then a second note is added explicitly. The order activity log will show two consecutive lines for the same event. Choose one: pass the longer text as the update_status note, or drop the second-argument note and keep add_order_note.

3.8 Bulk action result UX when zero orders are eligible — ✅ resolved in 1.4.1

Resolution: a shared notify_moved_count() helper suppresses the "X orders moved" success notice when the count is zero. The "skipped because terminal status" warning from the DB manager stands alone.

class-bulk-actions-manager.php:52-78 — if every selected order is in a terminal state, set_orders_to_prepare_printing returns 0 and emits a "0 orders moved" notice. The DB manager additionally emits an "X orders skipped" warning. The "0 orders moved" notice is technically truthful but uninformative noise on top of the more useful "skipped" warning. Suppress the success notice when count is zero.

3.9 PHP 8.4 deprecation of implicit fputcsv / fgetcsv escape character — ✅ resolved in 1.4.1

Resolution: every fputcsv and fgetcsv call now passes '"' as the enclosure and '' as the escape character explicitly.

class-bulk-actions-manager.php:117, 119 and class-db-manager.php:232, 238 call fputcsv / fgetcsv with only the delimiter argument. From PHP 8.4 onward the implicit escape='\\' parameter is deprecated and emits warnings; passing an explicit empty string is the preferred forward-compatible form. The plugin currently requires PHP 7.2+ and does not pin a maximum, so installations on PHP 8.4 (released late 2024) will fill debug.log with deprecation warnings.

3.10 Order_Fields_Manager::save_custom_order_fields lacks a capability check — ✅ resolved in 1.4.1

Resolution: a current_user_can('edit_shop_order', $order_id) (with 'edit_shop_orders' fallback) gate was added.

The handler hooks woocommerce_process_shop_order_meta, which WC fires after its own form-handling pipeline that does include capability validation. So this is not exploitable in normal flow. Still, the pattern most third-party plugins follow includes a current_user_can('edit_shop_order', $order_id) (or the legacy edit_post) inside the callback for defense in depth, and to handle programmatic invocation from custom code paths.


4. Low — cosmetic, style, longer-term

4.1 dedupe_by_order_no is case-sensitive — ✅ resolved in 1.4.1

Resolution: keys are lowercased before insertion into the $seen set.

class-db-manager.php:257-269 — keys are trim((string) $row['order_no']). Order IDs in WooCommerce are numeric so this is irrelevant in practice, but if any deployment uses an order-number plugin that produces alphabetic prefixes (e.g. ORD-12345), ord-12345 and ORD-12345 would not dedupe. Normalising to a single case in the dedup key is cheap insurance.

4.2 run_import action-name dispatch is brittle — ✅ resolved in 1.4.1

Resolution: dispatch is now data-driven via Import_Manager::protocol_dispatch() (file field, nonce, DB handler callable per protocol).

class-import-manager.php:102-106 selects the DB handler by string equality on $action. Refactoring to a callable map keyed by action would prevent silent fall-through if a future protocol is added.

4.3 wp_kses_post may be heavier than necessary for plain notices — ⏸ left as-is

All current notice messages are constant strings or sprintf-with-esc_html content. wp_kses_post is already safe in this context; the optimisation would be cosmetic. Revisit if richer notice content is added.

utils-log.php:66 renders notice messages through wp_kses_post(). Notices are mostly plain text with the occasional <code> tag; wp_kses_post allows a broad set of post-content tags. If notice messages will only ever contain simple inline markup, a narrower allowlist via wp_kses would be safer (and faster). Low priority since current usage is already escaped at call sites.

4.4 Notice transient TTL is 60 seconds — ✅ resolved in 1.4.1

Resolution: UtilsLog::NOTICE_TTL raised to 300 seconds.

utils-log.php:8 — if the redirect that should display the notice is delayed (slow network, reverse-proxy hiccup, user closes the browser and re-opens), the notice is dropped. 5 minutes is a more typical TTL for "post-action feedback" patterns. Drop is silent — there is no way for the user to recover the message.

4.5 Per-tab race in render_notices — ⏸ accepted

UX edge case; cost/benefit of a more elaborate "notices delivered" registry doesn't justify the added complexity.

utils-log.php:49-69 deletes the transient on first render. If a user has two admin tabs open and both load simultaneously after a bulk action, only one tab shows the notices. Not a correctness issue, but a slight UX surprise.

4.6 STUDIOU_WC_OPS_FILE and STUDIOU_WC_OPS_VERSION are unused — ⏸ left as-is (see §3.6)

Already listed in §3.6. Repeated here only to note the cosmetic angle: define-time effort without a reader is noise.

4.7 No Network: header in the plugin file — ⏸ deferred (multisite not in scope)

Carried over from §4.6 of the previous review. Only relevant for multisite installs.

4.8 No uninstall.php / cleanup hook — ⏸ deferred (conscious carry-over)

Carried over from §3.11 of the previous review. Order meta keys remain in the database after plugin removal. Acceptable but undocumented.

4.9 register_post_status is invoked even under HPOS — ⏸ left as-is (required for WC admin chrome)

Order_Status_Manager::register_custom_order_statuses() registers the statuses via register_post_status. Under HPOS this is harmless and still needed for WC admin chrome that calls get_post_statuses(), but the contract is slightly mixed-paradigm. No action required.

4.10 update_status calls do not pass manual = true — ⏸ left as-is (bulk transitions are not manual)

bulk_set_print_status and the import handlers call $order->update_status($slug) without the optional $manual parameter. WC distinguishes manual (user-driven) from automated transitions in its hook payloads. Currently these transitions appear as "automated" in WC reports — which is arguably correct (the bulk action is automated) but is a UX nuance worth conscious choice.

4.11 WooCommerce 10.7.0 block-based order edit screen — ⏸ deferred (needs live verification)

WC 10.x continues to ship the block-based / React order edit screen alongside the classic one (feature-flagged per install). Order_Fields_Manager hooks woocommerce_admin_order_data_after_order_details and woocommerce_process_shop_order_meta, both of which target the classic order edit screen. Under the block-based screen the Print Information panel does not render and edits are not saved through that pipeline. Currently this is acceptable (most production installs of WC 10.7.0 still default to the classic screen), but if Studiou opts into the block-based edit experience, a parallel data-source / slot-fill integration will be needed.


5. Verification gaps (against WC 10.7.0 / WP 6.9.4)

The following claims in v1.4.0 rest on assumptions that could not be confirmed without an interactive test on the target stack:

  1. HPOS search filter — see §2.1. Needs confirmation that woocommerce_order_table_search_query_meta_keys is still emitted by WC 10.7.0.
  2. MySQL functional-dependency recognition for the new GROUP BY — see §1.1. Production MySQL/MariaDB version and sql_mode should be checked; WP 6.9.4 itself imposes no constraint here, the risk is the database server config.
  3. woocommerce_order_table_search_query_meta_keys payload shape — the callback assumes WC passes an indexed array of meta-key strings. If WC 10.7.0 has changed it to an associative array (key → label) or a different shape, in_array($meta, $keys, true) would silently no-op.
  4. Transient delivery timing under WC 10.7.0UtilsLog::message() queues notices via a transient keyed by user ID. If a request is served while WC is invalidating user-scoped object caches (e.g. session bootstrap), the notice could be lost between queue and render. Difficult to reproduce; worth a smoke test.
  5. Header ordering on the export responsenocache_headers() precedes our Content-Type: text/csv. WC may emit its own headers earlier in the request via WP filters; the resulting Content-Type and Cache-Control order should be inspected with a browser inspector or curl -I on a WC 10.7.0 install.
  6. Block-based order edit screen — §4.11. WC 10.7.0 ships both classic and block-based order edit screens. The Print Information panel is wired to the classic screen only; behaviour on the block-based screen has not been verified.

6. Status of 1.3.2 findings

1.3.2 finding Status in 1.4.0
§1.1 Search by External Order Number is broken Resolved in code, pending verification (see §2.1 of this review).
§1.2 handle_status_transitions writes to wrong table under HPOS Resolved ($order->update_meta_data() + save()).
§2.1 No CSV row deduplication Resolved (Studiou_DB_Manager::dedupe_by_order_no).
§2.2 CSV header casing drift Resolved (normalise_header).
§2.3 Bulk export non-atomic Resolved (CSV built before status flip).
§2.4 Multi-category row multiplication Resolved via GROUP BYnew risk introduced, see §1.1.
§2.5 CSV UTF-8 BOM Resolved on export — new asymmetry with import, see §1.2.
§2.6 Payment-complete demotion Resolved.
§2.7 Import file validation Resolved (validated_upload).
§2.8 Bulk handlers cap check Resolved.
§3.1 UtilsLog::message() dead code Resolved (transient + admin_notices).
§3.2 WP_DEBUG guard too strict Resolved.
§3.3 Asymmetric logging Resolved (shared bulk_set_print_status).
§3.4 No state-machine guard Resolved (terminal-status skip).
§3.5 Plugin-init order risk Resolved (priority pinning).
§3.6 SQL prepare via intval Resolved ($wpdb->prepare() + %d).
§3.7 Init-time logging volume Resolved (init logs removed).
§3.8 parse_csv line-length cap Resolved (fgetcsv(... 0 ...)).
§3.9 external_ref_ord_date validation Partially resolved — see §3.5 of this review.
§3.10 external_ref_ord_date UI exposure Resolved.
§3.11 No uninstall cleanup Not addressed (conscious carry-over).
§3.12 No version constant Resolved (STUDIOU_WC_OPS_VERSION), albeit unused.
§3.13 HPOS column callback parameter naming Resolved ($order_or_id).
§4.1 Translations incomplete Resolved (docs/translations.txt extended). The .mo file still needs a rebuild.
§4.2 README/CLAUDE.md overclaim HPOS Resolved in wording — but contingent on §2.1 verification.
§4.3 CLAUDE.md SQL prepare claim Resolved.
§4.4 // visualized comments Resolved.
§4.5 Bracket formatting Resolved as a side-effect of rewriting the class.
§4.6 No Network: header Not addressed (carried over).
§4.7 README architecture note Cosmetic — left as-is.

Net: 26 of 30 findings closed in code; 3 carried over; 1 (search) closed but flagged as verification-pending.


7. Risk-prioritised summary

Ordered by expected user-visible impact (status as of 1.4.1):

  1. §1.1 Export SQL GROUP BY may fail on default MySQL strict mode. ✅ resolved.
  2. §1.2 BOM-in-imported-CSV breakage. ✅ resolved.
  3. §2.1 HPOS search filter unverified. ⏸ deferred — still pending live verification on WC 10.7.0.
  4. §2.2 Server-vs-WP timezone drift in Print Information panel. ✅ resolved.
  5. §3.3 output_csv fallback echoes raw CSV into HTML. ✅ resolved.
  6. §3.5 normalise_csv_date silently keeps bogus dates. ✅ resolved.
  7. §3.1 GROUP_CONCAT truncation. ✅ resolved.
  8. §3.9 PHP 8.4 deprecation noise. ✅ resolved.
  9. §3.7 Two order notes per payment-complete event. ✅ resolved.

8. Out of scope for this review

  • Live behaviour testing against a running WordPress 6.9.4 / WooCommerce 10.7.0 install — every finding above is derived from source reading.
  • Performance profiling of the rewritten export query (GROUP BY plus four LEFT JOINs) under large order volumes.
  • Behaviour of the WP admin notices renderer when WC's own admin notices are also queued in the same request.
  • Behavioural verification on the WooCommerce 10.7.0 block-based order edit screen (covered analytically in §4.11).
  • .po / .mo correctness — translations were not exercised; this review covers only their source-side coverage in docs/translations.txt.

End of review.