|
|
@@ -0,0 +1,267 @@
|
|
|
+# Plugin Revision — v1.4.3 (Fresh-Eye Analytical Review)
|
|
|
+
|
|
|
+> **Status as of 1.4.4 (2026-05-12):** every in-scope finding below has been resolved in plugin version **1.4.4**. Each finding 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.3 codebase. Re-read every PHP file from scratch and walked through user scenarios with deliberate scepticism toward fixes I made in the 1.4.2 / 1.4.3 cycles. The intent of this pass was specifically to look for *regressions introduced by recent fixes* rather than longer-standing issues.
|
|
|
+
|
|
|
+**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`, the markdown docs, and the refreshed `.po` translation source.
|
|
|
+**Verification limitations:** Static review only. No live PHP/WP/WC available.
|
|
|
+
|
|
|
+Severity reflects user-visible or correctness impact.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 1. Critical
|
|
|
+
|
|
|
+*(None.)*
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 2. High — newly discovered, introduced by recent fix-ups
|
|
|
+
|
|
|
+### 2.1 `Studiou_DB_Manager::normalise_csv_date()` shifts timezone-less CSV dates by the WP timezone offset — ✅ resolved in 1.4.4
|
|
|
+
|
|
|
+Resolution: `strtotime` removed entirely. Fast path uses pure-regex reformat for ISO-like inputs (no timezone shift). Fallback uses `DateTimeImmutable($raw, wp_timezone())->setTimezone(wp_timezone())` — interprets timezone-less input as WP local, honours explicit timezones, throws on garbage. The structural fix recommended in the review's §9 (zero `strtotime` calls in `includes/`) is in effect.
|
|
|
+
|
|
|
+`class-db-manager.php:426-437`:
|
|
|
+
|
|
|
+```php
|
|
|
+private static function normalise_csv_date($raw) {
|
|
|
+ $raw = sanitize_text_field((string) $raw);
|
|
|
+ if ($raw === '') {
|
|
|
+ return '';
|
|
|
+ }
|
|
|
+ $timestamp = strtotime($raw);
|
|
|
+ if (!$timestamp) {
|
|
|
+ UtilsLog::log('normalise_csv_date: could not parse "' . $raw . '"');
|
|
|
+ return '';
|
|
|
+ }
|
|
|
+ return wp_date('Y-m-d H:i:s', $timestamp);
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+This was changed from `date(...)` to `wp_date(...)` in 1.4.2 with the stated intent of "matching `current_time('mysql')` elsewhere". **The change introduced a timezone shift bug.** Trace:
|
|
|
+
|
|
|
+WordPress calls `date_default_timezone_set('UTC')` very early in its boot. So `strtotime($raw)` for a timezone-less input string interprets it as UTC. Then `wp_date($fmt, $timestamp)` treats its `$timestamp` argument as a UTC value and formats it in the WP site timezone.
|
|
|
+
|
|
|
+Concrete walkthrough with WP site timezone `Europe/Prague` (UTC+2 in summer):
|
|
|
+
|
|
|
+1. CSV `externalorderdate` cell: `"2026-05-12 14:30"` — the print shop wrote this intending Prague-local 14:30.
|
|
|
+2. `strtotime("2026-05-12 14:30")` in WP's UTC-default PHP context → timestamp T (representing 14:30 UTC).
|
|
|
+3. `wp_date('Y-m-d H:i:s', T)` → formatted in Prague timezone → `"2026-05-12 16:30:00"`.
|
|
|
+4. Stored to meta. Operator opening the order edit screen sees **16:30**. CSV said 14:30. **2-hour drift.**
|
|
|
+
|
|
|
+The same direction-of-drift bug as 1.4.0 §2.2 / 1.4.1 §2.1, just in a different function. The previous fix-pass made `Order_Fields_Manager::mysql_to_datetime_local` and `datetime_local_to_mysql` use pure string reformatting (no `strtotime`/`date`/`wp_date`) precisely to avoid this. `Custom_Columns_Manager::format_meta_date` was rebuilt with `DateTimeImmutable(..., wp_timezone())` for the same reason. **`normalise_csv_date` was missed.**
|
|
|
+
|
|
|
+**Suggested fix** — fast-path the common "no explicit timezone" pattern with pure string reformat, fall back to `DateTimeImmutable` with `wp_timezone()` for everything else:
|
|
|
+
|
|
|
+```php
|
|
|
+private static function normalise_csv_date($raw) {
|
|
|
+ $raw = sanitize_text_field((string) $raw);
|
|
|
+ if ($raw === '') {
|
|
|
+ return '';
|
|
|
+ }
|
|
|
+ // Fast path: ISO-ish date with no explicit timezone — treat as WP local
|
|
|
+ // time, no shift.
|
|
|
+ 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;
|
|
|
+ }
|
|
|
+ // Fallback: parse in WP timezone (no shift on naive input), convert
|
|
|
+ // to WP timezone (handles explicit-TZ input by shifting to local).
|
|
|
+ try {
|
|
|
+ $dt = (new DateTimeImmutable($raw, wp_timezone()))->setTimezone(wp_timezone());
|
|
|
+ return $dt->format('Y-m-d H:i:s');
|
|
|
+ } catch (Exception $e) {
|
|
|
+ UtilsLog::log('normalise_csv_date: could not parse "' . $raw . '"');
|
|
|
+ return '';
|
|
|
+ }
|
|
|
+}
|
|
|
+```
|
|
|
+
|
|
|
+This restores the documented invariant: imported `external_ref_ord_date` reads back at the value the print shop sent.
|
|
|
+
|
|
|
+The drift was hidden in 1.4.2 because `Custom_Columns_Manager::format_meta_date` had its own *opposite* drift bug — the two cancelled out in the columns view. Now that the columns view is correct (1.4.2), the import-side drift is the visible asymmetry.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 3. Medium — pre-existing or longstanding
|
|
|
+
|
|
|
+### 3.1 Plugin assumes HPOS without enforcing it — ✅ resolved in 1.4.4
|
|
|
+
|
|
|
+Resolution: `get_orders_for_printing()` now runs `table_exists("{$wpdb->prefix}wc_orders")` parallel to the existing `wc_order_product_lookup` check. If HPOS is disabled, the export bails with an admin notice ("Export requires WooCommerce HPOS … Enable High-Performance Order Storage …") instead of failing with a cryptic SQL error.
|
|
|
+
|
|
|
+`class-db-manager.php:84` references `{$wpdb->prefix}wc_orders` (the HPOS custom table). The bootstrap declares HPOS compatibility:
|
|
|
+
|
|
|
+```php
|
|
|
+FeaturesUtil::declare_compatibility('custom_order_tables', __FILE__, true);
|
|
|
+```
|
|
|
+
|
|
|
+The third argument is `$compatible=true`, meaning "this plugin works correctly with HPOS". It does *not* mean "this plugin requires HPOS". If a site has HPOS turned off (legacy CPT storage only), the `wc_orders` table either doesn't exist or is unsynced. The export query then either:
|
|
|
+
|
|
|
+- Errors with "Table 'wp_wc_orders' doesn't exist" — caught by the `$results === false` branch, logged but user-visible as "no exportable items".
|
|
|
+- Returns rows from an incomplete or stale lookup — the export is incorrect but emits no warning.
|
|
|
+
|
|
|
+The plugin has a precedent for surfacing this kind of dependency: `table_exists($wpdb->prefix . 'wc_order_product_lookup')` already runs at the top of the same method (1.4.2 §3.7). A parallel check for `wc_orders` would close the gap, or the plugin could refuse to load entirely under legacy CPT and surface an admin notice à la `woocommerce_missing_notice`.
|
|
|
+
|
|
|
+The Search Manager has fallback (`woocommerce_shop_order_search_fields` for legacy CPT alongside the HPOS filter), but the export path doesn't. Asymmetric.
|
|
|
+
|
|
|
+### 3.2 `bulk_set_print_status` doesn't pass a note to `update_status`, while the import handlers do — ✅ resolved in 1.4.4
|
|
|
+
|
|
|
+Resolution: `bulk_set_print_status` now passes "Marked Prepare to Printing by bulk action." / "Marked In Printing by bulk action." to `update_status`. The audit trail in the order activity feed is now consistent across bulk-action and import paths.
|
|
|
+
|
|
|
+`class-db-manager.php:227`:
|
|
|
+
|
|
|
+```php
|
|
|
+$order->update_status($status_slug);
|
|
|
+```
|
|
|
+
|
|
|
+…versus `class-db-manager.php:280-283`:
|
|
|
+
|
|
|
+```php
|
|
|
+$order->update_status(
|
|
|
+ 'in-print',
|
|
|
+ __('Marked In Printing by InPrint Protocol import.', 'studiou-wc-ord-print-statuses')
|
|
|
+);
|
|
|
+```
|
|
|
+
|
|
|
+Bulk-action-triggered transitions get WC's default "Status changed from X to Y" note. Import-triggered transitions get the explanatory `"Marked … by … import."` note. **Asymmetric audit trail.** The 1.4.3 release added explanatory notes to the import paths; the bulk paths were missed.
|
|
|
+
|
|
|
+Pass an explanatory note from `bulk_set_print_status` too — e.g., `"Marked … by bulk action."`.
|
|
|
+
|
|
|
+### 3.3 `normalise_csv_date` fallback (after fix) — `strtotime()` is permissive — ✅ resolved in 1.4.4
|
|
|
+
|
|
|
+Resolution: subsumed by §2.1's fix. `DateTimeImmutable` throws on inputs `strtotime` would silently misinterpret (`'foo'`, `'1'` etc.), so the catch-Exception branch logs and returns empty rather than persisting a meaningless timestamp.
|
|
|
+
|
|
|
+Independent of §2.1, `strtotime` returns a valid timestamp for many ambiguous inputs:
|
|
|
+
|
|
|
+- `strtotime('1')` → current year, January 1st.
|
|
|
+- `strtotime('next thursday')` → next Thursday.
|
|
|
+- `strtotime('foo bar')` → false (the only one that errors).
|
|
|
+
|
|
|
+If a print shop's CSV has a malformed date that happens to parse, the import silently stores a meaningless date. The current `if (!$timestamp)` guard only catches the `false` return value, not the "parsed but absurd" cases.
|
|
|
+
|
|
|
+The fix-suggestion in §2.1 — going through `DateTimeImmutable($raw, wp_timezone())` — is stricter; it throws on unparseable input rather than guessing.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 4. Low — style and cosmetic
|
|
|
+
|
|
|
+### 4.1 Re-entrancy of `$order->save()` inside `handle_status_transitions` — ⏸ left as-is (forward-looking comment kept in code)
|
|
|
+
|
|
|
+The behaviour is correct under WC 10.7.0's caching semantics; the docblock in `class-order-status-manager.php:83-93` already explains the contract. Refactoring would require a more invasive design change to avoid the listener saving entirely.
|
|
|
+
|
|
|
+`class-order-status-manager.php:107-109`. The listener fires from within `woocommerce_order_status_changed`, which is itself inside `WC_Order::update_status()`. The listener calls `$order->save()` to persist the meta change. The parent path (bulk action, import handler, or manual edit) then calls its own `$order->save()` afterwards.
|
|
|
+
|
|
|
+Today this works because WC's data store returns the same object instance from `wc_get_order($id)` while the request is hot — both saves operate on the same object, the second save is a no-op or just re-saves the same state. If WC ever changes the cache semantics (e.g., to return fresh objects after a save), the parent and listener would operate on different instances and the meta change could be clobbered by the parent's save.
|
|
|
+
|
|
|
+Brittle but not broken. Worth a forward-looking comment.
|
|
|
+
|
|
|
+### 4.2 `bulk_set_print_status` and the listener both stamp the same meta key — ⏸ left as-is (intentional, documented)
|
|
|
+
|
|
|
+Acknowledged in `class-order-status-manager.php:71-77`'s docblock as intentional. The redundancy costs one extra `save()` per bulk-changed order, but the behaviour is correct. The 1.4.0 review §2.3 and the 1.4.1 review covered this.
|
|
|
+
|
|
|
+### 4.3 `Custom_Columns_Manager` calls `wc_get_order` per cell — ✅ resolved in 1.4.4
|
|
|
+
|
|
|
+Resolution: under HPOS the column-content callback receives the `WC_Order` object directly — `wc_get_order` is no longer called. Under legacy CPT a per-request `$order_cache` indexed by ID memoises the lookup so the three custom columns share one resolved instance per row.
|
|
|
+
|
|
|
+`class-custom-columns-manager.php:31`. On a 50-orders-per-page list with three custom columns, that's 150 calls per page load. WC's own object cache mitigates the cost, but a per-page-load static cache keyed by ID inside the manager would be tighter.
|
|
|
+
|
|
|
+### 4.4 `emit_import_summary` always shows "%d order(s) updated successfully" even when the count is 0 — ✅ resolved in 1.4.4
|
|
|
+
|
|
|
+Resolution: gated on `$updated > 0`, matching the pattern `Bulk_Actions_Manager::notify_moved_count` introduced in 1.4.3.
|
|
|
+
|
|
|
+`class-import-manager.php:175-186`. If 100% of rows errored, the operator sees "0 orders updated successfully" + the more useful error notice. The `notify_moved_count` helper in `Bulk_Actions_Manager` (1.4.3) already does the suppression-on-zero pattern; the import summary could adopt it.
|
|
|
+
|
|
|
+### 4.5 `add_action('before_woocommerce_init', function () {...})` uses an anonymous callback — ✅ resolved in 1.4.4
|
|
|
+
|
|
|
+Resolution: converted to a named function `studiou_wc_ord_print_statuses_declare_hpos_compat`. Other plugins / test harnesses can now `remove_action('before_woocommerce_init', 'studiou_wc_ord_print_statuses_declare_hpos_compat')` if they need to override.
|
|
|
+
|
|
|
+`studiou-wc-ord-print-statuses.php:26-30`. Anonymous functions cannot be unhooked from outside the file. If another plugin or test setup ever wants to disable the HPOS compatibility declaration, they can't. Cosmetic — converting to a named function is one line.
|
|
|
+
|
|
|
+### 4.6 `Studiou_DB_Manager::csv_escape` would prefix legitimate negative quantities with `'` — ⏸ left as-is
|
|
|
+
|
|
|
+Acknowledged trade-off — OWASP's recommendation to defuse formula injection is stronger than the cosmetic concern of a leading single quote on negative numbers (which Excel hides anyway). If negative quantities ever become a real workflow signal, csv_escape can be adjusted to whitelist numeric leading-minus.
|
|
|
+
|
|
|
+`class-db-manager.php:164-176`. A `qty` of `"-1"` (which shouldn't occur but might in refund-handling scenarios) starts with `-`, so `csv_escape` adds a leading single quote: `"'-1"`. Excel/LibreOffice display strips the quote so the user sees `-1` — no UX impact — but the underlying CSV bytes look unusual to programmatic consumers. The OWASP recommendation is consistent with this trade-off (defending against formula injection is more important than clean negative-number display), but worth noting.
|
|
|
+
|
|
|
+### 4.7 `call_user_func($cfg['handler'], $csv_data)` in `Import_Manager::run_import` — ✅ resolved in 1.4.4
|
|
|
+
|
|
|
+Resolution: replaced with `$handler = $cfg['handler']; $result = $handler($csv_data);` — direct callable invocation. Slightly faster, less verbose.
|
|
|
+
|
|
|
+`class-import-manager.php:129`. PHP 7+ supports invoking array callables directly: `$cfg['handler']($csv_data)`. The `call_user_func` form is slightly slower and slightly more verbose. Style only.
|
|
|
+
|
|
|
+### 4.8 `add_custom_shop_order_column_content`'s `external_order_number` cell relies on `$value !== ''` rather than `$value !== '' && $value !== '—'` — ⏸ left as-is (vanishingly rare edge case)
|
|
|
+
|
|
|
+If anyone ever stores the literal string `"—"` to `external_ref_ord_no` (operator typing it in for an unknown number), the column displays the literal `"—"`. Confusing but a vanishingly rare edge case.
|
|
|
+
|
|
|
+### 4.9 `parse_csv` doesn't handle `\r`-only line endings — ⏸ left as-is (legacy macOS, extremely rare)
|
|
|
+
|
|
|
+`class-db-manager.php:351-384`. PHP's `auto_detect_line_endings` was deprecated in 8.1 and removed in 9.0. macOS-Classic CSVs (`\r`-only) wouldn't tokenise correctly. Extremely unlikely in practice — modern macOS uses `\n`, Excel-Mac uses `\r\n`. Not worth fixing.
|
|
|
+
|
|
|
+### 4.10 `set_orders_to_prepare_printing` and `set_orders_to_in_printing` are thin wrappers — ⏸ left as-is (stable public API surface)
|
|
|
+
|
|
|
+`class-db-manager.php:197-209` — two two-line methods that delegate to `bulk_set_print_status`. The public API would be cleaner as a single `bulk_set_status($order_ids, $slug)` that the bulk action handler calls with the right slug. Refactor-only; behaviour unchanged.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 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.
|
|
|
+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 | Status of in-scope findings | Carried-over deferrals |
|
|
|
+|--------|------------------------------|-------------------------|
|
|
|
+| `revise-1.3.2.md` | All resolved in 1.4.0 | 3 (`Network:` header, uninstall, cosmetic README). |
|
|
|
+| `revise-1.4.0.md` | All resolved in 1.4.1 | 1 (HPOS filter verification). |
|
|
|
+| `revise-1.4.1.md` | All resolved in 1.4.2 | 2 (custom-order-number plugins, register_post_status). |
|
|
|
+| `revise-1.4.2.md` | All resolved in 1.4.3 | 1 (Excel CS-locale CSV separator). |
|
|
|
+
|
|
|
+**One newly introduced regression** identified in this pass: §2.1 above, the side-effect of the 1.4.2 `date()` → `wp_date()` change in `normalise_csv_date`. Worth noting as a process observation: the same class of bug has now appeared in three separate functions across three releases (1.4.0 `Order_Fields_Manager`, 1.4.1 `Custom_Columns_Manager`, 1.4.2 `normalise_csv_date`). A future fix should audit *every* call to `strtotime` + `date_i18n`/`wp_date`/`date` in the codebase for the same pattern.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 7. Risk-prioritised summary
|
|
|
+
|
|
|
+Ordered by user-visible severity. Status as of **1.4.4**:
|
|
|
+
|
|
|
+1. **§2.1 `normalise_csv_date` shifts timezone-less CSV dates by the WP offset.** ✅ resolved.
|
|
|
+2. **§3.1 No hard guard against HPOS-disabled installs.** ✅ resolved.
|
|
|
+3. **§3.2 `bulk_set_print_status` doesn't pass an explanatory note.** ✅ resolved.
|
|
|
+4. **§3.3 `normalise_csv_date` fallback accepts permissive `strtotime` interpretations.** ✅ resolved (subsumed by §2.1).
|
|
|
+5. **§4.1 Listener `save()` re-entrancy.** ⏸ left as-is (docblock kept).
|
|
|
+6. **§4.3 Per-cell `wc_get_order` in column rendering.** ✅ resolved.
|
|
|
+7. **§4.4 Zero-count import "success" notice.** ✅ 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 on large order volumes.
|
|
|
+- Behavioural verification on the WC 10.x block-based order edit screen.
|
|
|
+- Audit of the refreshed `.po` against the running plugin (the file was rewritten in this cycle; cross-verifying every `msgstr` against the source `msgid` is out of scope for this review).
|
|
|
+- Multisite, uninstall, custom-order-number plugin integration (carried-over deferrals).
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+## 9. Process observation
|
|
|
+
|
|
|
+The two prior fresh-eye reviews (`revise-1.4.1`, `revise-1.4.2`) each surfaced a single new high-severity bug that had survived earlier passes. This review continues the pattern with §2.1 — and the cause is a regression *I introduced* in the 1.4.2 fix for a different timezone bug. The same code shape (`strtotime` + format-in-WP-timezone) appears in three different functions across three releases, fixed each time only in the function specifically called out by the review.
|
|
|
+
|
|
|
+The reliable fix for this class of bug is to **drop `strtotime` for the "stored as WP-local MySQL datetime ↔ display in WP-local" pipeline entirely**. Either pure string reformatting (as `Order_Fields_Manager` now does) or `DateTimeImmutable(..., wp_timezone())` (as `Custom_Columns_Manager` now does) sidesteps the WP UTC-default trap. A grep for `strtotime` across `includes/` would catch any future regression.
|
|
|
+
|
|
|
+Today the only remaining `strtotime` call is in `normalise_csv_date`. After §2.1 is fixed, there should be none.
|
|
|
+
|
|
|
+> **Confirmation in 1.4.4:** a grep for `strtotime` across `includes/` returns zero hits in executable code. Only the docblock comments in `class-custom-columns-manager.php`, `class-db-manager.php`, and `class-order-fields-manager.php` mention it — explaining why each function avoids the pattern.
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+*End of review.*
|
