|
|
@@ -0,0 +1,260 @@
|
|
|
+# Plugin Revision — v1.4.4 (Fresh-Eye Analytical Review)
|
|
|
+
|
|
|
+> **Status as of 1.4.5 (2026-05-12):** all four mediums and items §4.1, §4.2 are resolved in plugin version **1.4.5**. §4.3 and §4.4 are documented and deferred (architectural / edge case). §3.4 is documented in README "Known limitations" without a code change. Each finding carries an inline ✅ resolved / ⏸ deferred status marker.
|
|
|
+
|
|
|
+**Scope:** Independent read-only analytical pass over the v1.4.4 codebase. Read with deliberate scepticism toward the changes introduced in 1.4.4 itself, looking for regressions and gaps the previous reviews missed.
|
|
|
+
|
|
|
+**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, the markdown docs, and the `.po` translation source.
|
|
|
+**Verification limitations:** Static review only. No live PHP/WP/WC available.
|
|
|
+
|
|
|
+Severity reflects user-visible or correctness impact.
|
|
|
+
|
|
|
+> **Headline:** no critical findings in this round. The plugin is in a steady state. Four medium-severity items and four low-severity items remain — most are defensive improvements rather than active defects. Two of the mediums (§3.1, §3.3) are *gaps* in fixes I introduced in 1.4.4 itself.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 1. Critical
|
|
|
+
|
|
|
+*(None.)*
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 2. High
|
|
|
+
|
|
|
+*(None.)*
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 3. Medium — gaps and asymmetries
|
|
|
+
|
|
|
+### 3.1 `normalise_csv_date` fast-path lacks range validation — asymmetric with `datetime_local_to_mysql` — ✅ resolved in 1.4.5
|
|
|
+
|
|
|
+Resolution: the fast path now runs `checkdate($month, $day, $year)` plus explicit hour ≤ 23 / minute ≤ 59 / second ≤ 59 bounds. Inputs that match the regex structurally but fall outside the valid ranges (e.g., `"2026-13-45T25:99"`) drop through to the `DateTimeImmutable` fallback, which throws and routes to the catch → empty string + logged.
|
|
|
+
|
|
|
+`class-db-manager.php:459-462`:
|
|
|
+
|
|
|
+```php
|
|
|
+if (preg_match('/^(\d{4})-(\d{2})-(\d{2})[T ](\d{2}):(\d{2})(?::(\d{2}))?$/', $raw, $m)) {
|
|
|
+ $second = isset($m[6]) ? $m[6] : '00';
|
|
|
+ return $m[1] . '-' . $m[2] . '-' . $m[3] . ' ' . $m[4] . ':' . $m[5] . ':' . $second;
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+The regex matches structurally — `"2026-13-45T25:99"` passes — but doesn't validate that the date is real or that the time components are in range. The fast-path returns `"2026-13-45 25:99:00"`, which is then stored to postmeta as-is.
|
|
|
+
|
|
|
+Compare with `Order_Fields_Manager::datetime_local_to_mysql` (`class-order-fields-manager.php:118-131`), which I extended in 1.4.3 to add `checkdate()` plus explicit hour/minute/second bounds. **The same defence belongs on the import side**, because:
|
|
|
+
|
|
|
+- A CSV from the print shop's system could realistically include malformed dates (data-entry typo, locale mismatch, broken upstream tool).
|
|
|
+- The bogus stored value round-trips through display: `Order_Fields_Manager::mysql_to_datetime_local` matches the regex but produces nonsense (`2026-13-45T25:99`), which the browser renders as an empty input. The operator can't tell anything happened until they save the order — at which point `datetime_local_to_mysql` finally rejects it.
|
|
|
+
|
|
|
+**Suggested fix** — apply the same validation in the fast path:
|
|
|
+
|
|
|
+```php
|
|
|
+if (preg_match('/^(\d{4})-(\d{2})-(\d{2})[T ](\d{2}):(\d{2})(?::(\d{2}))?$/', $raw, $m)) {
|
|
|
+ list(, $y, $mo, $d, $h, $mi) = $m;
|
|
|
+ $s = isset($m[6]) ? $m[6] : '00';
|
|
|
+ if (!checkdate((int) $mo, (int) $d, (int) $y) || (int) $h > 23 || (int) $mi > 59 || (int) $s > 59) {
|
|
|
+ // Fall through to the DateTimeImmutable path, which will throw on
|
|
|
+ // unparseable values and reach our catch.
|
|
|
+ return self::normalise_csv_date_fallback($raw);
|
|
|
+ }
|
|
|
+ return $y . '-' . $mo . '-' . $d . ' ' . $h . ':' . $mi . ':' . $s;
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+…or simpler: drop the fast path entirely and always use `DateTimeImmutable($raw, wp_timezone())`. The fast path was an optimisation; the fallback handles the common case correctly too.
|
|
|
+
|
|
|
+### 3.2 `emit_import_summary` is silent when 0 successes and 0 errors — ✅ resolved in 1.4.5
|
|
|
+
|
|
|
+Resolution: added an `elseif (empty($errors))` branch that emits an info notice ("The CSV file contained no actionable rows (empty order_no column).") so the operator gets feedback even when dedup-by-order_no filters out every row.
|
|
|
+
|
|
|
+`class-import-manager.php:172-209`. The 1.4.4 change gated the "%d orders updated successfully" notice on `$updated > 0`. That's correct *when* there are errors to surface, but if both counters are zero the operator sees nothing.
|
|
|
+
|
|
|
+How can both be zero? `run_import` already bails earlier when `$csv_data` is empty (line 121). But `dedupe_by_order_no` (line 413) further filters out rows whose `order_no` is empty after `trim()`. A CSV with header + N rows where every row has an empty `order_no` column produces `count($csv_data) === N` but yields zero rows after dedup. No order_no → no `wc_get_order` lookup → no error entry → zero on both sides.
|
|
|
+
|
|
|
+The operator gets no feedback that the import "succeeded" (it ran without error) but did nothing.
|
|
|
+
|
|
|
+**Suggested fix** — add an else branch:
|
|
|
+
|
|
|
+```php
|
|
|
+} elseif (empty($errors)) {
|
|
|
+ UtilsLog::message(
|
|
|
+ __('The CSV file contained no actionable rows (empty order_no column).', 'studiou-wc-ord-print-statuses'),
|
|
|
+ 'info'
|
|
|
+ );
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+### 3.3 `csv_escape` checks only the literal first character — leading-whitespace bypass — ✅ resolved in 1.4.5
|
|
|
+
|
|
|
+Resolution: `csv_escape` now calls `ltrim` and checks the first non-whitespace character. Cells like `" =CMD"` are now prefixed with `'`.
|
|
|
+
|
|
|
+`class-db-manager.php:174-186`:
|
|
|
+
|
|
|
+```php
|
|
|
+$first = $value[0];
|
|
|
+if (in_array($first, array('=', '+', '-', '@', "\t", "\r"), true)) {
|
|
|
+ return "'" . $value;
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+Cells beginning with a space followed by a danger character (e.g., `" =CMD"`) slip past — `$value[0]` is the space. Some spreadsheet apps trim leading whitespace before evaluating, others don't; behaviour varies by version and config.
|
|
|
+
|
|
|
+**Vector likelihood:** low in this codebase — the danger-prone fields are `prod_name`, `prod_var`, `prod_cat`, `email`. Product / category names are admin-controlled. `email` is customer-controlled but WC's email validation rejects strings starting with space-equals.
|
|
|
+
|
|
|
+Still, the OWASP recommendation is to check the value after `ltrim`, not just `$value[0]`. One-line fix:
|
|
|
+
|
|
|
+```php
|
|
|
+$trimmed = ltrim($value);
|
|
|
+if ($trimmed !== '' && in_array($trimmed[0], array('=', '+', '-', '@', "\t", "\r"), true)) {
|
|
|
+ return "'" . $value;
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+### 3.4 Possible collation mismatch on `terms.slug = postmeta.meta_value` — ⏸ documented in README "Known limitations"
|
|
|
+
|
|
|
+Resolution path: the issue is theoretical and hasn't been observed in production. Adding an explicit `COLLATE` clause to the join condition risks breaking installs whose actual collation differs from whatever we'd hardcode. The pragmatic choice is to document the failure mode (which manifests as `ERROR 1267 (Illegal mix of collations)` from the bulk export) so future operators can add the fix if it surfaces. README "Known limitations" now mentions this.
|
|
|
+
|
|
|
+`class-db-manager.php:104-105`:
|
|
|
+
|
|
|
+```sql
|
|
|
+LEFT JOIN `{$wpdb->terms}` `product_variations_name`
|
|
|
+ ON `product_variations_name`.`slug` = `product_variations_attr`.`meta_value`
|
|
|
+```
|
|
|
+
|
|
|
+`terms.slug` is `VARCHAR(200)`; `postmeta.meta_value` is `LONGTEXT`. Both columns *should* use `utf8mb4_unicode_ci` on a default WP install, but installs that have been around long enough to have been migrated across MySQL versions (or had per-table collation changes applied) can end up with mismatched collations.
|
|
|
+
|
|
|
+When MySQL encounters a join condition between columns with incompatible collations, it can either:
|
|
|
+- Throw `ERROR 1267 (HY000): Illegal mix of collations` and abort the query, or
|
|
|
+- Implicitly coerce via temporary string conversion, which can silently change case sensitivity / accent handling for that join.
|
|
|
+
|
|
|
+The export query has no `COLLATE` clause, so it inherits the DB-default collation. Most production sites won't be affected — Studiou's install presumably wasn't, given that earlier 1.3.2 / 1.4.0 / 1.4.1 reviews didn't surface this. Worth noting as an unbounded edge case rather than a confirmed bug.
|
|
|
+
|
|
|
+**Mitigation** (only if it becomes a problem): add `COLLATE utf8mb4_unicode_ci` to the JOIN condition.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 4. Low
|
|
|
+
|
|
|
+### 4.1 `bulk_set_print_status` note selection is a binary ternary — implicit else — ✅ resolved in 1.4.5
|
|
|
+
|
|
|
+Resolution: lifted to an explicit `array('to-print' => …, 'in-print' => …)` map with `isset` fallback to empty string. Future contributors adding a third slug get a no-op note rather than the wrong note.
|
|
|
+
|
|
|
+`class-db-manager.php:229-231`:
|
|
|
+
|
|
|
+```php
|
|
|
+$note = ($status_slug === 'to-print')
|
|
|
+ ? __('Marked Prepare to Printing by bulk action.', 'studiou-wc-ord-print-statuses')
|
|
|
+ : __('Marked In Printing by bulk action.', 'studiou-wc-ord-print-statuses');
|
|
|
+```
|
|
|
+
|
|
|
+The function is only ever called with `'to-print'` or `'in-print'`, so the else branch is correct today. If a future contributor extends the helper to a third status, the else-implicit selection of "In Printing" would silently produce a misleading note. Defensive option: a small dispatch map.
|
|
|
+
|
|
|
+### 4.2 `parse_csv` doesn't detect duplicate headers — ✅ resolved in 1.4.5
|
|
|
+
|
|
|
+Resolution: after `array_map(normalise_header, $headers)`, `parse_csv` compares `count($headers)` against `count(array_unique($headers))` and bails (with an `UtilsLog::log` entry naming the headers) on mismatch. The downstream import then emits the existing "could not be parsed" notice.
|
|
|
+
|
|
|
+`class-db-manager.php:391-397`:
|
|
|
+
|
|
|
+```php
|
|
|
+$headers = array_map(array(__CLASS__, 'normalise_header'), $headers);
|
|
|
+while (($data = fgetcsv($handle, 0, ',', '"', '')) !== false) {
|
|
|
+ if (count($data) !== count($headers)) {
|
|
|
+ continue;
|
|
|
+ }
|
|
|
+ $csv_data[] = array_combine($headers, $data);
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+If the CSV has two columns sharing a normalised name (e.g., `Order No` and `order_no` both normalising to `order_no`), `array_combine` silently overwrites — the second column's data wins for every row, the first column's data is lost. The user gets no warning.
|
|
|
+
|
|
|
+Realistic? Unlikely with a well-formed protocol CSV. Defensive — add `count($headers) === count(array_unique($headers))` check and abort with a clear notice if not.
|
|
|
+
|
|
|
+### 4.3 Main plugin instance is short-lived; managers persist via the WP filter array — ⏸ left as-is (architectural)
|
|
|
+
|
|
|
+Resolution path: refactoring would require either making the main instance a singleton (with `getInstance()`) or moving manager initialisation to a static method. Both are larger refactors than the symptom warrants — there is no functional impact, only a documentation concern. Worth a CLAUDE.md note if the lifecycle pattern surprises a future contributor.
|
|
|
+
|
|
|
+`studiou-wc-ord-print-statuses.php:47-50`:
|
|
|
+
|
|
|
+```php
|
|
|
+public static function initPlugin() {
|
|
|
+ $class = __CLASS__;
|
|
|
+ new $class;
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+The new instance has no global / static reference. After `__construct()` calls `init()` and `init()` returns, the local `$class` variable goes out of scope and PHP's reference counter for the main instance hits zero. The instance is GC'd.
|
|
|
+
|
|
|
+The managers stored in `$this->order_status_manager` etc. would normally be GC'd along with the main instance — *except* each manager's constructor registers WP hooks with `array($this, '...')`, which adds references to the manager instances inside the global `$wp_filter` array. Those references keep the managers alive for the rest of the request.
|
|
|
+
|
|
|
+So the private `$order_status_manager` / `$order_fields_manager` / etc. fields on `Studiou_WC_Ord_Print_Statuses` are effectively *decorative* — they only matter during the few microseconds of `initialize_managers()`. After `init()` returns they're inaccessible.
|
|
|
+
|
|
|
+**Implications:** none in normal flow. But code that tries to look up "the plugin's order status manager" via `Studiou_WC_Ord_Print_Statuses` (e.g., a third-party hook integrating with this plugin) will find no instance. The pattern is unusual and worth documenting.
|
|
|
+
|
|
|
+### 4.4 `woocommerce_missing_notice` registered conditionally but never unregistered — ⏸ left as-is (edge case)
|
|
|
+
|
|
|
+`studiou-wc-ord-print-statuses.php:54-60`. If WC is not yet active when our `init()` runs, the notice hook is registered. If WC becomes active later in the same request (extremely unlikely — WC would have to be activated mid-request by another plugin's hook), the notice would still display once on the next admin page load. Edge case.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 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. The legacy filter `woocommerce_shop_order_search_fields` may not fire under HPOS at all — making it not a fallback. Smoke test by adding a temporary `error_log` inside the callback and observing whether the HPOS path actually fires.
|
|
|
+2. **`woocommerce_process_shop_order_meta`** behaviour under the WC 10.7.0 block-based order edit screen.
|
|
|
+3. **Object-cache scenarios for `UtilsLog::message`** (Redis/Memcached).
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 6. Status of prior reviews
|
|
|
+
|
|
|
+| Review | Resolved | Carried-over deferrals |
|
|
|
+|--------|----------|-------------------------|
|
|
|
+| `revise-1.3.2.md` | 1.4.0 | 3 (`Network:` header, uninstall, README phrasing). |
|
|
|
+| `revise-1.4.0.md` | 1.4.1 | 1 (HPOS filter verification). |
|
|
|
+| `revise-1.4.1.md` | 1.4.2 | 2 (custom-order-number plugins, register_post_status). |
|
|
|
+| `revise-1.4.2.md` | 1.4.3 | 1 (Excel CS-locale CSV separator). |
|
|
|
+| `revise-1.4.3.md` | 1.4.4 | (listener save() re-entrancy, bulk/listener double-stamp — both intentional, documented; csv_escape on negatives — OWASP trade-off; "—" literal column, `\r`-only line endings, `set_orders_to_*` API surface — vanishingly rare). |
|
|
|
+
|
|
|
+**No new regressions introduced by 1.4.4** in the strict sense. §3.1 and §3.3 are *under-applied defences* — patterns that should have extended to the import path / been stricter in csv_escape — rather than active bugs.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 7. Risk-prioritised summary
|
|
|
+
|
|
|
+Ordered by user-visible severity. Status as of **1.4.5**:
|
|
|
+
|
|
|
+1. **§3.1 `normalise_csv_date` fast-path missing range validation.** ✅ resolved.
|
|
|
+2. **§3.2 Silent import when all rows have empty `order_no`.** ✅ resolved.
|
|
|
+3. **§3.3 `csv_escape` leading-whitespace bypass.** ✅ resolved.
|
|
|
+4. **§3.4 Collation mismatch on slug join.** ⏸ documented in README "Known limitations".
|
|
|
+5. **§4.1 Binary note selection in `bulk_set_print_status`.** ✅ resolved.
|
|
|
+6. **§4.2 Duplicate-header detection in `parse_csv`.** ✅ resolved.
|
|
|
+7. **§4.3 Main plugin instance lifecycle.** ⏸ left as-is (architectural, no functional impact).
|
|
|
+8. **§4.4 `woocommerce_missing_notice` re-fire.** ⏸ left as-is (edge case).
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 8. Out of scope
|
|
|
+
|
|
|
+- Live behaviour testing against a running WP 6.9.4 / WC 10.7.0 install.
|
|
|
+- Performance profiling under realistic order volumes.
|
|
|
+- Behavioural verification on the WC 10.x block-based order edit screen.
|
|
|
+- Multisite, uninstall, custom-order-number plugin integration (carried-over deferrals).
|
|
|
+- Verifying every `msgstr` in the refreshed `.po` against the running plugin.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 9. Process observation
|
|
|
+
|
|
|
+The "single new HIGH per fresh-eye pass" pattern from 1.4.0 → 1.4.3 has tailed off. This pass surfaces only mediums and lows. Either:
|
|
|
+
|
|
|
+- The plugin has genuinely reached a stable state (likely), or
|
|
|
+- I'm developing scope blindness on this codebase after seven reviews and four fix-up releases (possible — fresh-eye reviews work best with rotating reviewers).
|
|
|
+
|
|
|
+If a second reviewer is available, this is a good moment for an outside pass. Otherwise, the diminishing-returns curve suggests the next iteration of fixes should focus on the carried-over verification gaps (live testing against WC 10.7.0) rather than another static review cycle.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+*End of review.*
|
