|
|
@@ -1,6 +1,8 @@
|
|
|
# Plugin Revision — v1.4.0 (Analytical Review)
|
|
|
|
|
|
-**Scope:** Read-only analytical pass over the v1.4.0 codebase. No code changes were made. This document is the counterpart of [`revise-1.3.2.md`](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.
|
|
|
+> **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`](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+.
|
|
|
@@ -13,7 +15,7 @@ Findings are grouped by severity. The severity reflects user-visible or correctn
|
|
|
|
|
|
## 1. Critical — newly introduced regressions
|
|
|
|
|
|
-### 1.1 Export SQL `GROUP BY` may fail under default MySQL strict mode
|
|
|
+### 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:
|
|
|
|
|
|
@@ -33,7 +35,7 @@ Under MySQL 5.7+ and MariaDB 10.3+ the default `sql_mode` includes `ONLY_FULL_GR
|
|
|
- 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
|
|
|
+### 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.
|
|
|
|
|
|
@@ -45,7 +47,7 @@ This combination is realistic — exports often serve as templates for the next
|
|
|
|
|
|
## 2. High — correctness, HPOS, time-zone
|
|
|
|
|
|
-### 2.1 HPOS search filter name is unverified against WC 10.7.0
|
|
|
+### 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.
|
|
|
|
|
|
@@ -53,7 +55,9 @@ This combination is realistic — exports often serve as templates for the next
|
|
|
|
|
|
**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
|
|
|
+### 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.
|
|
|
|
|
|
@@ -67,7 +71,9 @@ Replacing `date(...)` with `wp_date(...)` or constructing a `DateTimeImmutable`
|
|
|
|
|
|
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
|
|
|
+### 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'))`.
|
|
|
|
|
|
@@ -85,13 +91,15 @@ A subtler asymmetry: the listener stamps **only if empty** (`if (!$order->get_me
|
|
|
|
|
|
## 3. Medium — robustness, maintainability
|
|
|
|
|
|
-### 3.1 `GROUP_CONCAT` is silently bounded by `group_concat_max_len`
|
|
|
+### 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
|
|
|
+### 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:
|
|
|
|
|
|
@@ -104,7 +112,9 @@ if ($first !== "\xEF\xBB\xBF") {
|
|
|
|
|
|
inside `parse_csv` would close this gap.
|
|
|
|
|
|
-### 3.3 `output_csv` fallback path mixes CSV into HTML on header-sent edge
|
|
|
+### 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`:
|
|
|
|
|
|
@@ -122,7 +132,9 @@ private function output_csv($csv_data, $filename) {
|
|
|
|
|
|
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
|
|
|
+### 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`:
|
|
|
|
|
|
@@ -136,7 +148,9 @@ $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
|
|
|
+### 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`:
|
|
|
|
|
|
@@ -151,12 +165,16 @@ private static function normalise_csv_date($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
|
|
|
+### 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
|
|
|
+### 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`:
|
|
|
|
|
|
@@ -167,15 +185,21 @@ $order->add_order_note(__('Order automatically set to processing by Studiou WC O
|
|
|
|
|
|
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
|
|
|
+### 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
|
|
|
+### 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
|
|
|
+### 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.
|
|
|
|
|
|
@@ -183,47 +207,57 @@ The handler hooks `woocommerce_process_shop_order_meta`, which WC fires after it
|
|
|
|
|
|
## 4. Low — cosmetic, style, longer-term
|
|
|
|
|
|
-### 4.1 `dedupe_by_order_no` is case-sensitive
|
|
|
+### 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
|
|
|
+### 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
|
|
|
+### 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
|
|
|
+### 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`
|
|
|
+### 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
|
|
|
+### 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
|
|
|
+### 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
|
|
|
+### 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
|
|
|
+### 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`
|
|
|
+### 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
|
|
|
+### 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.
|
|
|
|
|
|
@@ -283,17 +317,17 @@ Net: 26 of 30 findings closed in code; 3 carried over; 1 (search) closed but fla
|
|
|
|
|
|
## 7. Risk-prioritised summary
|
|
|
|
|
|
-Ordered by expected user-visible impact:
|
|
|
-
|
|
|
-1. **§1.1 Export SQL `GROUP BY` may fail on default MySQL strict mode.** This is the most likely cause of a fresh production incident on 1.4.0.
|
|
|
-2. **§1.2 BOM-in-imported-CSV breakage.** Subtle, easy to reproduce by exporting then re-importing.
|
|
|
-3. **§2.1 HPOS search filter unverified.** If wrong, the headline 1.4.0 fix is incomplete.
|
|
|
-4. **§2.2 Server-vs-WP timezone drift in Print Information panel.** Will produce off-by-one-hour confusion in CZ timezone with DST.
|
|
|
-5. **§3.3 `output_csv` fallback echoes raw CSV into HTML.** Edge case but visible when it happens.
|
|
|
-6. **§3.5 `normalise_csv_date` silently keeps bogus dates.** Operator-visible inconsistency.
|
|
|
-7. **§3.1 `GROUP_CONCAT` truncation.** Unlikely but silent.
|
|
|
-8. **§3.9 PHP 8.4 deprecation noise.** Cosmetic but log-spammy.
|
|
|
-9. **§3.7 Two order notes per payment-complete event.** Cosmetic noise in order history.
|
|
|
+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.
|
|
|
|
|
|
---
|
|
|
|