Fix two more bugs found in a third code-review pass
- 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.
This commit is contained in:
@@ -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 . ')');
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user