BRD-1225: stop dropping products and variants with an empty SKU - #78
Conversation
Shops do not require a SKU (PrestaShop reference, Shopify sku are optional), so products without one were skipped in sync and missing from search. Product, ProductVariant and ProductBuilder now accept an empty sku. The PrestaShop adapters read a null or missing sku as '' instead of throwing. id stays required.
|
@claude review |
|
Claude finished @PauliusInvertus's task in 1m 15s —— View job Claude finished reviewing this PR
Note: I could not execute SummaryThis is a clean, well-scoped change. It removes SKU validation exactly where it should (product/variant construction), leaves FindingsNo bugs found. A few minor observations, none blocking:
Overall: the change is minimal, targeted, and matches its stated scope (SDK-side SKU optionality, Magento untouched, no fabricated SKU fallback). I'd approve pending the actual CI run confirming the quality-gate triple, since I couldn't execute it myself here. |
* releases/2026.05 * BRD-1225: stop dropping products and variants with an empty SKU (#78) Shops do not require a SKU (PrestaShop reference, Shopify sku are optional), so products without one were skipped in sync and missing from search. Product, ProductVariant and ProductBuilder now accept an empty sku. The PrestaShop adapters read a null or missing sku as '' instead of throwing. id stays required. * BRD-1053: carry Shopify variant image into variant imageUrl (#79) * BRD-1053: carry Shopify variant image into variant imageUrl Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * BRD-1053: read variant image from media, not the deprecated image field Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * BRD-1216: custom product fields support in PrestaShopAdapterV2 (#77) * feat(prestashop): map merchant custom product fields into additionalFields * style(prestashop): strip development comments * fix(prestashop): sanitize custom field values with strip_tags transformCustomFields() inlined the localized-values loop instead of reusing addLocalizedField(), and the scalar branch never sanitized at all, so custom fields indexed raw HTML unlike every other localized field (name, description, brand). * fix(customfields): skip non-scalar custom field values instead of stringifying them A custom field whose value arrived as an array was cast with (string), which emits an Array to string conversion warning and stores the literal Array — silent data corruption in the index. An object value would have thrown outright, failing the whole product transform rather than one field. Guards both paths a custom field can take. The localized branch routes through addLocalizedField(), which is shared with name/description/brand/features, so the guard there also removes the same corruption path for core fields; skipping is strictly better than storing Array and no caller can want the old behaviour. Both tests fail without the guards, emitting the conversion warning. * fix(prestashop): keep accepting Stringable localized field values The previous commit tightened addLocalizedField()'s guard from `$value === null` to `!is_scalar($value)` to stop arrays being stringified into the index. That also silently dropped Stringable objects, which used to work via `(string) $value`, on name/description/descriptionShort/brand as well as on localized custom fields. Route every value through stringifyFieldValue(): scalars and Stringable are accepted, arrays and non-stringable objects stay rejected. * fix(customfields): stop strip_tags corrupting custom field values Two defects in one path: strip_tags() drops everything from an unmatched `<` to the end of the string. Custom columns hold codes, size ranges and numeric notes, not HTML bodies, so "30<x<40" was silently indexed as "30", "S<M<L" as "S" and "5<3" as "5". Non-text values were stripped at all, and the emptiness check ran on the raw value instead of the cleaned one, so a text value that cleaned down to '' was still written. brad-app maps non-text custom fields as integer/double/date and an empty string on one of those makes the backend reject the whole product. Now: HTML removal applies to text-typed values only, it no longer truncates at a literal `<` (literal angle brackets are parked behind a sentinel while strip_tags removes real markup), values are trimmed - char(32) columns arrive space-padded - and anything empty after cleaning is skipped. Core localized fields (name/description/descriptionShort/brand/features) keep their existing always-strip behaviour: the type awareness is passed in from transformCustomFields and defaults to off. * fix(customfields): skip MySQL zero dates on date-typed custom fields A nullable DATE/DATETIME column hands out '0000-00-00 00:00:00' rather than NULL. brad-app maps a date-typed custom field as an ES date, which rejects that value, and one rejected field fails the whole product document. The PrestaShop module already normalizes the zero date away for the core createdAt/updatedAt fields but not for custom columns, so filter it here. Only date-typed fields are affected: a text column may legitimately contain '0000-00-00' as literal content. * fix(customfields): validate custom field names before building field paths The name came straight off the network payload and was concatenated into a search field name. A `.` in it becomes an object path in the index, and an over-long name is rejected by the backend. Hold names to the module's own column-name rule, /^[a-zA-Z0-9_]{1,64}$/, and skip the entry otherwise; this also subsumes the previous empty-name check. * refactor(customfields): expose the custom_ prefix and document the entry shape The `custom_` literal is repeated across three repos, so publish it as PrestaShopAdapterV2::CUSTOM_FIELD_PREFIX and use it internally. The field name shapes (custom_<name>, custom_<name>_<locale>) are unchanged. Also document the accepted entry shape on transformCustomFields, the way transformFeatures documents its own: this is a wire contract between the PrestaShop module, this SDK and brad-app. * docs(customfields): cut the added prose to the non-obvious why * docs: drop the explanatory comments from the custom-field fixes * BRD-1216: normalise boolean custom fields and tighten the name pattern - real PHP booleans become 'true'/'false' instead of '1'/dropped - name pattern anchored with the D modifier so a trailing newline is rejected - tests for booleans, trailing newline, duplicate names and map-shaped input - document the custom field entry shape on transformCustomFields() Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Paulius Stukšys <paulius.stuksys@invertus.eu> Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: GantasG <gantas.grigalionis@invertus.eu>
2 participants
Self-Checks
Summary
Products and variants with an empty SKU are no longer rejected. Shops do not require a SKU (PrestaShop
referenceand Shopifyskuare optional), so the sync silently skipped those products and they were missing from search (Fishon, ~11k products, product 1524).QA Checklist Labels
QA Checklist
Additional Context
Product,ProductVariant:validateSku()removed.idis still required.ProductBuilder:skudefaults to'';->sku()is optional.PrestaShopAdapter(v1) andPrestaShopAdapterV2: productskuis read as(string) ($product['sku'] ?? ''). Before, anullor missing sku also dropped the product ("Required field 'sku' is missing"). The variant'sku' is requiredthrow is removed.''/null/ missing and variant''/ missing in both PrestaShop adapters;Product::fromArraywithout sku.phpunit1610 OK,phpstanOK.Independent validation
skucasts to the string"Array"— same unguarded cast as every other string field in these adapters, not changed here.null(adapter already maps to''), WooCommerce returns'', Magento requires sku — all confirmed against docs/source.Needs human verification
referenceand combinations with references. Search by name shows the product with no SKU row; search by a combination code shows that combination's SKU and price.Local end-to-end check (2026-08-27)
Ran with the three BRD-1225 branches wired together locally (SDK copied into brad-app vendor, brad-search live-reloaded).
reference+ 3 combinations (MUG-WHITE,MUG-BLACK, one without reference) → 19/19 indexed, no SKU errors.MUG-BLACKsearch shows the mug with that combination sku/price; name search showsMUG-WHITE(variant URL contains the product name, so variants match too); the combination without reference is indexed as""and does not wipe anything.null→"") incl. "The Premium Helmet #1000" and "The Essential Rugged Gloves #987" — both found by name with no SKU shown;HEL-00955-CRIstill returns the parent with that variant sku/price.Confidence: 95%
JIRA task link: https://invertus.atlassian.net/browse/BRD-1225