From ed70207453bf4cdec07e031577ea8a47ba97d6b5 Mon Sep 17 00:00:00 2001 From: Twooey Date: Thu, 27 Aug 2026 17:02:41 -0400 Subject: [PATCH] Fix two more bugs found in a third code-review pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SupplierOffer::delete_for_isbn() was the one Catalog method still missing a $wpdb failure check. There's no unique constraint on isbn13 alone (schema only has a composite key on isbn13+status+base_price), so SyntheticOfferGenerator's delete-then-insert pattern relied entirely on the delete actually succeeding — a silent failure there left the stale row in place alongside the new insert, two rows for one ISBN, with best_offer_for_isbns() picking whichever was cheaper by chance and generate-offers reporting full success throughout. Verified with the same forced-failure reproduction as the review that found it (a BEFORE DELETE trigger): before this fix, that scenario left 2 rows for the ISBN; after, it throws immediately (caught by generate_all()'s existing per-ISBN handler) and the row count stays at 1. - deploy/lib/env.sh's env_get() didn't strip a trailing \r or leading/ trailing whitespace from extracted values. Consequential specifically because of restore.sh's DB drop/recreate (added earlier this session): DB_NAME_VALUE feeds directly into DROP DATABASE IF EXISTS/CREATE DATABASE for that name — a CRLF-saved or hand-edited-with-trailing-space .env file would silently target a different database name than the real one, with no error, defeating the "clean restore" guarantee that fix exists for. Also mattered for quote-stripping: a trailing \r landing after a closing quote made the end-with-quote check silently fail to match, leaking literal quote characters into the value. Verified against CRLF-terminated, trailing-whitespace, and quoted+CRLF combinations, including a byte-level check that no \r survives; confirmed no regression against normal LF-terminated unquoted/quoted values via a full deploy.sh dev run. --- deploy/lib/env.sh | 13 +++++++++++++ .../includes/Catalog/SupplierOffer.php | 17 ++++++++++++++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/deploy/lib/env.sh b/deploy/lib/env.sh index 54f3a14..ce79f56 100755 --- a/deploy/lib/env.sh +++ b/deploy/lib/env.sh @@ -9,6 +9,19 @@ env_get() { local file="$1" key="$2" line val line="$(grep -m1 -E "^${key}=" "$file" 2>/dev/null || true)" val="${line#*=}" + # Strip a trailing \r (a CRLF-saved file — edited on Windows, or via some + # SFTP/GUI tools) and any leading/trailing whitespace BEFORE the + # quote-detection below. Order matters: a CRLF-terminated quoted value has + # its \r land after the closing quote, so the end-with-quote check below + # would silently fail to match and the literal quote characters would + # leak into the returned value instead of being stripped. Verified + # directly: left unstripped, a corrupted DB_NAME value fed into restore.sh's + # `DROP DATABASE IF EXISTS \`$DB_NAME_VALUE\`` targets a DIFFERENT + # database name than the real one — no error, just a "restore" that + # silently doesn't clean the actual target database first. + val="${val%$'\r'}" + val="${val#"${val%%[![:space:]]*}"}" + val="${val%"${val##*[![:space:]]}"}" if [[ "$val" == \"*\" && "$val" == *\" ]]; then val="${val#\"}"; val="${val%\"}" elif [[ "$val" == \'*\' && "$val" == *\' ]]; then diff --git a/wp-content/plugins/bookstore-core/includes/Catalog/SupplierOffer.php b/wp-content/plugins/bookstore-core/includes/Catalog/SupplierOffer.php index c092b9c..dcce9ba 100644 --- a/wp-content/plugins/bookstore-core/includes/Catalog/SupplierOffer.php +++ b/wp-content/plugins/bookstore-core/includes/Catalog/SupplierOffer.php @@ -74,6 +74,21 @@ class SupplierOffer public static function delete_for_isbn(string $isbn13): void { global $wpdb; - $wpdb->delete(self::table(), ['isbn13' => $isbn13]); + $result = $wpdb->delete(self::table(), ['isbn13' => $isbn13]); + if ($result === false) { + // The one Catalog method still missing this check, and not a + // hypothetical gap: there's no unique constraint on isbn13 + // alone (schema only has a composite KEY on isbn13+status+ + // base_price), so SyntheticOfferGenerator's delete-then-insert + // pattern relies entirely on the delete actually succeeding. + // Verified directly (a forced delete failure via a BEFORE + // DELETE trigger): a silent no-op here left the stale row in + // place alongside the new insert — two rows for one ISBN, with + // best_offer_for_isbns() picking whichever was cheaper by + // chance, and generate-offers reporting full success the whole + // time. Throwing here routes it into generate_all()'s existing + // per-ISBN catch instead of a silent duplicate. + throw new \RuntimeException('bsc_supplier_offer delete failed: ' . $wpdb->last_error . ' (isbn13: ' . $isbn13 . ')'); + } } }