BRD-1216: custom product fields support in PrestaShopAdapterV2

[GantasG]

Summary

Adds custom product field handling to PrestaShopAdapterV2 so custom_* values coming from the PrestaShop module's product payloads are transformed and forwarded to the engine (both plain and per-locale suffixed names).

Testing

• 34 tests green, PHPStan level 4 clean, PHPCS clean.
• Verified end-to-end locally against the full pipeline (module → brad-app → engine → storefront search hits on custom values).

Merge order / release

• Merge this first. Recommend tagging v4.15.0 (minor bump) after merge — brad-app needs its composer.json bumped from v4.14.0 to consume this.

Jira: BRD-1216

[GantasG]

@claude please review this PR

[claude]

Claude finished @GantasG's task in 2m 56s —— View job

---

Review complete ✅

Todos

• Gather context (diff, CLAUDE.md conventions, related ValueObjects)
• Review PrestaShopAdapterV2.php changes
• Review test changes
• Check locale-suffix contract compliance
• Run quality-gate checks — could not run (see note below)
• Post final review

---

Summary

The change adds transformCustomFields() (src/Adapters/PrestaShopAdapterV2.php:593-624), called once from transformProduct() (:148), which maps each customFields[] entry into additionalFields['custom_' . name], with per-locale suffixing when localizedValues is present. It's small, well-scoped, and matches the file's existing patterns closely — I'd compare it directly against transformFeatures() and addLocalizedField().

Findings

• Locale-suffix behavior is correct for this codebase, despite reading like a CLAUDE.md violation at first glance. The root CLAUDE.md documents a contract where the default locale is unsuffixed (name, not name_en-US). This PR's testTransformLocalizedCustomFieldSuffixesEveryLocale explicitly asserts the opposite — every locale gets a suffix, including the first one. However, this matches PrestaShopAdapterV2's own established behavior for name/brand/description via addLocalizedField() (see the existing test comment at tests/Adapters/PrestaShopAdapterV2Test.php:194: // en-US locale fields (always suffixed)). So this PR is internally consistent with the adapter it's extending — not a regression — but it's worth flagging that this specific adapter's convention diverges from the documented contract (which appears to target the V2 LocalizedField ValueObject / other consumers, not this bespoke additionalFields map). Nothing to change here, just noting it so it isn't mistaken for a bug later.

• Custom field names are forwarded with no format validation. $name comes straight from the merchant-defined customFields[].name and becomes part of an Elasticsearch field key (custom_{$name}) with no sanitization (spaces, dots, unicode, casing all pass through untouched). This mirrors how the file already handles dynamic keys elsewhere, but those are all system-controlled (locale codes, numeric remoteIds) rather than free-form merchant input. If the PrestaShop module already normalizes field names before sending them (the PR description mentions e2e verification against the real module), this is fine as-is — just flagging it as the one part of this change that touches genuinely uncontrolled input. Worth a quick confirmation that the module slugifies/validates names before this reaches the SDK.

• Minor: no (string) cast on $field['value'] (:626) — other optional fields in this file (ean13, mpn, timestamps) are explicitly cast with (string) before being stored. Here the raw value (whatever type the module sends) is stored as-is. Given additionalFields is array<string, mixed> and gets spread directly into jsonSerialize(), this isn't a correctness bug, just an inconsistency with the surrounding code's defensiveness. Not blocking.

• Test coverage is thorough: locale suffixing, non-localized values, name-collision with core fields (price, id), missing customFields key, and malformed entries (non-array items, missing/empty name, empty localized value) are all covered. The malformed-entry test correctly matches the continue-after-inner-loop control flow in transformCustomFields() (:614), which prevents empty localized values from falling through to the value branch.

• No fixture updates needed — additionalFields is a free-form pass-through map in Product::jsonSerialize() (src/V2/ValueObjects/BulkOperations/Product.php:222-224), not a fixed OpenAPI shape, so this correctly stays outside the golden-fixture parity discipline.

Not verified

I don't have permission to run composer install in this environment, so I could not execute the quality-gate triple (phpunit, phpstan, phpcs) myself. The PR description states all three are green — the code itself reads as PHPStan-level-4-clean and PSR-12-consistent with the rest of the file, but this wasn't independently confirmed by me. If you'd like me to verify, granting Bash network/composer permissions would let me run the full suite.

Verdict

No blocking issues. The two notes above (custom field name sanitization, missing (string) cast) are worth a quick look but aren't correctness bugs given the adapter's existing conventions — approve-able as is, pending your own CI confirmation.

[GantasG]

@claude please review this PR again.

The earlier review ran against an older head. Since then: fixes from the branch code review were applied.

Current head: 7867be8

[claude]

Claude finished @GantasG's task in 1m 28s —— View job

---

Re-review complete ✅

• Gather context (diff at head 7867be8, prior review notes)
• Review changes since last review (43271d0, 7867be8)
• Confirm previously flagged notes were addressed
• Run quality-gate checks — could not run (see note below)
• Post final review

---

Summary

Two commits landed since the last review, both addressing the earlier feedback:

1. 43271d0 (style) — removed a stale-looking dev comment block above transformCustomFields(). Minor note: the removed comment actually explained two non-obvious things (why the custom_ prefix is mandatory to avoid colliding with core fields like id/price, and why every locale — including the first — gets suffixed here unlike Shopify/Magento). That context is still correct and useful; it's now only preserved in commit history and test names rather than in the source. Not a problem, just noting it's gone if anyone reads the method cold in the future.

2. 7867be8 (fix) — this is the substantive one, and it addresses both actionable notes from the previous review:

   • The localized-values branch now calls addLocalizedField() (src/Adapters/PrestaShopAdapterV2.php:612) instead of inlining a duplicate loop, so it inherits that method's strip_tags() sanitization for free.
   • The scalar branch now does strip_tags((string) $field['value']) (:626) instead of storing the raw value untouched — this also resolves the previous "missing (string) cast" note as a side effect.

   Net effect: custom field values (both localized and plain) are now sanitized the same way name/description/brand already are, closing the one gap where merchant-supplied HTML could have reached the index unescaped. Good catch and clean fix — reusing addLocalizedField() instead of re-implementing it also removes the duplicate-loop code smell from the original version.

Verification

• New tests (testTransformLocalizedCustomFieldStripsHtml, testTransformNonLocalizedCustomFieldStripsHtml, tests/Adapters/PrestaShopAdapterV2Test.php:1181-1207) cover both branches with <b>ALPHA</b>-7741 → ALPHA-7741, consistent with how strip_tags behavior is already tested elsewhere in this file.
• Confirmed the fix doesn't regress the malformed-entry/collision/missing-key tests from the first commit — logic in the surrounding foreach (continue after the localized branch, empty-value guard before the scalar branch) is unchanged.
• The remaining note from the earlier review (merchant-supplied custom field names, e.g. custom_<name>, are still unsanitized/unvalidated) was flagged as informational only last time, not a requested fix, and wasn't addressed here — that's consistent with the PR description's scope (values, not names) and fine to leave as-is unless the module doesn't already constrain field-name format.

Not verified

Same limitation as the previous run: composer install requires network access I don't have in this environment, so I couldn't independently execute vendor/bin/phpunit, phpstan analyse, or phpcs. The diff is small, matches existing file conventions, and the PR description states all three are green — but this wasn't independently confirmed by me.

Verdict

The fix commit directly and correctly resolves the substantive point from the first review. No new issues found. Approve-able pending your own CI confirmation.

[GantasG]

@claude please review this PR.

The head is unchanged since your last review, so this is a fresh full-PR look rather than a delta.

Worth attacking: whether custom_* values are forwarded and sanitised correctly for both the localized and non-localized branches, and whether a malformed or partial custom-field entry from the plugin can break the transform for the whole product page rather than just that field.

[claude]

Claude finished @GantasG's task in 2m 20s —— View job

---

Review complete ✅

Todos

• Gather context (diff, CLAUDE.md conventions, prior review notes)
• Re-read transformCustomFields() and addLocalizedField() in full
• Review test coverage for malformed/partial entries
• Confirm whether a bad entry can break the whole product transform
• Run quality-gate checks — could not run (see note below)
• Post final review

---

Summary

This is a fresh full-PR look at the unchanged head (7867be8). Focusing on the two questions raised: forwarding/sanitization correctness for custom_* values, and blast radius of a malformed custom-field entry.

Findings

1. Sanitization is correct and consistent across both branches.

• Localized branch (src/Adapters/PrestaShopAdapterV2.php:611-615) delegates to addLocalizedField(), which applies strip_tags((string) $value) per locale (:676) and skips empty/null values per-locale.
• Non-localized branch (:621) applies strip_tags((string) $field['value']) directly.
  Both paths now sanitize identically to name/description/brand, closing the gap flagged in the previous review round.

2. A malformed or partial customFields[] entry cannot break the rest of the product. The loop in transformCustomFields() (:597-623) treats every failure mode as continue, never a throw: non-array item (:600), missing/non-string/empty name (:604-607), and empty localized/scalar values are all skipped individually while the rest of the array (and the rest of the product) is processed normally. testMalformedCustomFieldEntriesAreSkippedNotFatal (tests/Adapters/PrestaShopAdapterV2Test.php:1221-1238) exercises exactly this mix (non-array item, missing name, empty name, empty localized value, one good entry) and confirms errors stays empty and the good field survives. Even at the outer level, transform() wraps each product's transform in try/catch (:48-58), so even a hypothetical throw here would only drop that one product, not the batch.

3. One real gap: non-scalar value/localized value isn't guarded before the (string) cast. Both :621 ((string) $field['value']) and addLocalizedField()'s per-locale cast (:676) assume the value is already scalar. Every other malformed case in this method is defensively skipped, but if the PrestaShop module ever sends an array for a field's value — plausible for a multiselect/checkbox-style custom field type, since type is currently just informational and never branched on — (string) $arrayValue triggers PHP's "Array to string conversion" warning and silently stores the literal string "Array" instead of skipping the field or preserving its content. Not fatal (it won't take down the product or the batch), but it's a silent data-corruption path inconsistent with how carefully every other bad shape is handled here, and it's untested. Suggest guarding with is_scalar($field['value']) (mirroring the existing is_string($name) guard at :604) before the cast, and the same for the loop value in addLocalizedField() — that second one is pre-existing code shared by name/description/brand/features, so fixing it is broader than this PR's scope; flagging for awareness rather than as a blocking change here.
Fix this →

4. Previously-noted items remain unchanged (informational only, not blocking):

• Merchant-supplied custom field names (not values) are still forwarded into the Elasticsearch field key (custom_{$name}) with no format validation — out of this PR's stated scope (values, not names), fine if the module already constrains name format.
• The dev-comment removal in 43271d0 dropped some useful "why" context (custom_ prefix avoiding collisions with core fields, every-locale-suffixed convention) from the source into commit history only — cosmetic, not a concern.
• No fixture updates needed — additionalFields stays outside the OpenAPI golden-fixture discipline as a free-form pass-through map.

Not verified

Same limitation as both previous review rounds: no permission to run composer install / network in this environment, so vendor/bin/phpunit, phpstan analyse, and phpcs couldn't be executed independently here. The PR description states all three are green; the diff reads as consistent with the rest of the file's style and defensiveness except for finding #3 above.

Verdict

No blockers. Finding #3 (non-scalar value guard) is the one substantive new observation from this pass — a real but non-fatal gap, worth a small follow-up but not required to merge given it mirrors a pre-existing pattern in addLocalizedField(). Approve-able as-is pending your own CI confirmation.

[PauliusInvertus]

Review

Small diff, no SQL, no BC break on constructors/interfaces. Field naming (custom_<name> / custom_<name>_<locale> in additionalFields) matches what brad-app #548 and module #317 expect.

Blocking

strip_tags() cuts custom-field values at a bare < — src/Adapters/PrestaShopAdapterV2.php:621 (plain) and :676 (localized path).
strip_tags drops everything from an unmatched < to the end: S<M<L → S, 5<3 → 5, <n/a> → "". Custom columns are codes/ranges/notes, not HTML bodies, so the description sanitizer is the wrong tool here.
Also the empty check at :617 runs before the strip, so a value that becomes "" is still written — for a field brad-app maps as integer/double/date that makes OpenSearch reject the whole product.
Fix: only strip when ($field['type'] ?? 'text') === 'text', and re-check === '' after cleaning (both branches).

Important

• type is received and dropped — everything is sent as a string. Works today only because ES coerces "5"/"true". Breaks on MySQL zero dates (0000-00-00 00:00:00) in a date column → mapper_parsing_exception, product not indexed. Cheapest guard is here in transformCustomFields (module #317 should also guard it).
• addLocalizedField guard change null → !is_scalar at :671 also changes behaviour for name/description/descriptionShort/brand (a Stringable value is now silently dropped) and has no test for those callers.
• No name validation: 'custom_' . $name at :609 trusts the network payload. A . in the name becomes an object path in ES. One preg_match('/^[a-zA-Z0-9_]{1,64}$/') mirrors the module's own rule.
• Test gaps: no test with type = integer/double/date/boolean (module emits all four), duplicate name entries (last wins silently), customFields as non-list.

Nits

• transformCustomFields has no format docblock unlike transformFeatures at :551-568 — this is a 3-repo wire contract.
• 'custom_' is repeated across 3 repos; expose a public const here.
• No trim() — char(32) columns arrive space-padded.

Note on merge order

The PS module vendors its own Product model and doesn't require this package, so the v4.15.0 tag only gates brad-app #548, not module #317.

Verdict: approve after the strip_tags fix + check-after-strip ordering.

[PauliusInvertus]

Re-review (fix commits d0b5300..61ec66a)

Friday's blocker is properly fixed: stripHtmlTags() (src/Adapters/PrestaShopAdapterV2.php:746-752) protects bare < with a sentinel, stripping is gated on type === 'text', and the === '' check now runs after cleaning in the shared cleanCustomFieldValue() (:655-669), used by both branches. Zero-date guard (:664) covers both 0000-00-00 and 0000-00-00 00:00:00. Trim, the CUSTOM_FIELD_PREFIX const, name validation and the Stringable re-acceptance (with tests for the core-field callers) are all in. Full suite green (1624 tests, 4414 assertions).

Blocking — new bug introduced by the fix round

Boolean custom fields are broken both ways. Routing values through stringifyFieldValue() + the post-clean empty check means:

• false → '' → dropped at :660 — the field silently never indexes.
• true → "1" — OpenSearch boolean fields accept only true/false/"true"/"false", so "1" throws mapper_parsing_exception and the whole product is rejected. That's the exact failure class this review round was about.

Fix: normalise bool to 'true'/'false' in stringifyFieldValue() (or skip boolean-typed values that aren't already those strings). The module emits type: boolean, so this is reachable in prod.

Small

• Name pattern /^[a-zA-Z0-9_]{1,64}$/ at :619: PCRE's $ matches before a trailing newline, so "good\n" passes and emits custom_good\n. Use \A…\z or add the D modifier.
• Two cheap missing tests: duplicate names (last-wins is correct today, just unpinned) and a real PHP bool value — the case above.
• The wire-contract docblock added in a8cd8c8b was deleted again in the comment cleanup; the text|integer|double|boolean|date vocabulary is now documented nowhere in code. One line on transformCustomFields would do — it's a 3-repo contract, not prose.

Verdict: fix the boolean handling, then good to merge and tag v4.15.0.

[PauliusInvertus]

@claude review

[claude]

Claude finished @PauliusInvertus's task in 3m 39s —— View job

---

Re-review complete (head b78b057)

• Gather context (diff against the true branch point ce60ea8 — note: origin/releases/2026.05 in this shallow checkout is a disjoint/unrelated ref, so I diffed against the actual merge-base commit instead, confirmed by matching the PR's stated +610/-8 across 2 files)
• Review the latest commit (b78b057) against Friday's blocking finding (boolean custom fields)
• Check the small/nit items from the last round (regex $/\z, missing tests, docblock)
• Run quality-gate checks — could not run (see note below)
• Post final review

---

Summary

This pass focuses on b78b057, which responds to @PauliusInvertus's 2026-08-31 review. I re-read the full transformCustomFields / cleanCustomFieldValue / addLocalizedField / stringifyFieldValue chain (src/Adapters/PrestaShopAdapterV2.php:613-767), not just the diff, to make sure the fix composes correctly with the earlier rounds (strip_tags sentinel guard, type-gating, zero-date guard, Stringable acceptance).

Blocking finding from last round — resolved

Boolean custom fields. stringifyFieldValue() now special-cases is_bool() before the generic scalar cast (:758-760):

if (is_bool($value)) {
    return $value ? 'true' : 'false';
}

Traced both paths:

• Non-localized: false passes isset($field['value']) (isset only excludes null), stringifyFieldValue(false) → 'false', cleanCustomFieldValue('false', 'boolean') takes the non-text branch (no stripping) → 'false' survives. true → 'true'.
• Localized: the early empty-guard at :722 is $value === '', and false === '' is false, so it isn't filtered before stringification either.

Both are covered by testBooleanCustomFieldValuesAreNormalisedToTrueFalseStrings (tests/Adapters/PrestaShopAdapterV2Test.php:1497-1517), which checks all four combinations (plain true/false, localized true/false). This closes the mapper_parsing_exception/silent-drop failure class Friday's review was about.

Regex fix — correct

CUSTOM_FIELD_NAME_PATTERN gained the D modifier (:24). Without D, PCRE's $ matches before a trailing \n, so "good\n" would pass /^[a-zA-Z0-9_]{1,64}$/ and produce a field key with an embedded newline. With /D, $ only matches true end-of-string, so "good\n" is correctly rejected. Verified by testCustomFieldNameWithTrailingNewlineIsSkipped (:1519-1533).

Other items from the "Small" list

• Duplicate names: testDuplicateCustomFieldNameLastEntryWins (:1535-1545) pins the existing last-write-wins behavior (plain array-overwrite semantics — nothing to fix, just now covered).
• customFields as a non-list/associative map: testCustomFieldsGivenAsAssociativeMapIsHandledWithoutErrors (:1547-1559) confirms a string-valued entry ('stock_count' => '42') is skipped via the existing !is_array($field) guard rather than causing a type error, and a keyed-but-array entry still processes normally.
• Docblock: transformCustomFields() now documents the entry shape including the text|integer|double|boolean|date vocabulary (:607-609), addressing the "documented nowhere in code" nit.

One pre-existing, non-blocking observation (not introduced by this commit)

testTextCustomFieldValueThatBecomesEmptyAfterStrippingIsSkipped (:1382-1403) pins that a text-typed value of literally <n/a> gets silently dropped — stripHtmlTags()'s heuristic (LITERAL_ANGLE_PATTERN, :30) can't distinguish "looks like a tag" from "literal bracket notation," and <n/a> happens to parse as a well-formed-looking tag (letter, non-angle body, closing >), so strip_tags removes it and the post-strip empty check drops the field. This is already deliberately tested/named, and it's a silent single-field drop rather than a whole-product rejection (the failure class this review round cared about), so it doesn't block — just noting it's a known, accepted edge of the heuristic rather than something this commit changed.

Not verified

Same limitation as every prior round on this PR: no permission to run composer install (no vendor dir present, network-gated) or even a bare php -l in this sandboxed environment, so I could not independently execute vendor/bin/phpunit, phpstan analyse, or phpcs. @PauliusInvertus's 2026-08-31 comment states the full suite was green (1624 tests) as of the pre-boolean-fix commit; the new commit adds 4 tests on top of that but I couldn't re-run the suite myself. If you want me to verify directly, granting network/composer permissions in --allowedTools would let me run the full quality-gate triple.

Verdict

Friday's blocking issue (boolean custom fields) is correctly fixed and tested. The regex fix, doc addition, and the three new coverage gaps Paulius asked for are all in. No new blocking issues found. Approve-able pending your own CI confirmation of the full quality-gate triple.
· branch BRD-1216-custom-product-fields