Skip to content

BRD-1053: carry Shopify variant image into variant imageUrl - #79

Merged
PauliusInvertus merged 2 commits into
releases/2026.05from
BRD-1053
Sep 1, 2026
Merged

BRD-1053: carry Shopify variant image into variant imageUrl#79
PauliusInvertus merged 2 commits into
releases/2026.05from
BRD-1053

Conversation

@PauliusInvertus

@PauliusInvertus PauliusInvertus commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Why

Shopify variants were sent without imageUrl on purpose (686a7ac), so search results always showed the parent image even when a specific variant matched. PO decision (BRD-1225 comment 141084): match PrestaShop — show the matched variant's photo.

Change

  • ShopifyAdapter::transformVariants sets imageUrl {small, medium} from variant.image.url when the variant has its own image; variants without one carry no imageUrl, so brad-search keeps the product image (empty-skip from BRD-1225).
  • Test replaced: testVariantWithOwnImageCarriesImageUrl.

Needs bradsearch-shopify-app1 to fetch image { url } on variants (companion PR) and a re-sync.

Jira: BRD-1053

🤖 Generated with Claude Code

Independent validation

  • Requirements: OK (variant with image → imageUrl {small,medium}; without → key absent; product image untouched)
  • Adversarial: no defects; all four image shapes (missing/null/array/string) safe under failOnWarning
  • Assumptions: ProductVariant.image is deprecated in Admin API 2026-04 → switched to media(first: 1) { nodes { ... on MediaImage { image { url } } } } (https://shopify.dev/docs/api/admin-graphql/2026-04/objects/ProductVariant)

Note: this deliberately reverses the earlier "keep the merchant's curated featuredImage" decision (686a7ac) per PO on BRD-1225.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@PauliusInvertus

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Aug 28, 2026

Copy link
Copy Markdown

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


Review

  • Gather context (diff, changed files, CLAUDE.md conventions)
  • Review ShopifyAdapter.php changes
  • Review test changes
  • Post review feedback

Summary: Small, well-scoped change. Logic and tests look correct to me — nothing blocking.

What I checked

  • src/Adapters/ShopifyAdapter.php:740-743 — sets imageUrl from variant['image']['url'] only when it's a non-empty string, otherwise the key is omitted entirely (not set to null/''). That matches the "empty-skip" contract described in the PR body and is exercised by the test (assertArrayNotHasKey('imageUrl', $variants[1]) for the null-image variant).
  • Shape consistency: ['small' => $url, 'medium' => $url] matches the ImageUrl value object's required fields (src/V2/ValueObjects/Product/ImageUrl.php:26-33, both small/medium required) and mirrors how PrestaShopAdapterV2 populates variant imageUrl — good alignment with the "match PrestaShop" goal stated in the PR.
  • Null-safety: $variant['image']['url'] ?? null is safe whether image is absent, null, or an array without url — no undefined-index warnings, which matters since phpunit.xml has failOnWarning.
  • The stale doc comment explaining the old "never surface variant image" rationale was correctly removed rather than left dangling.
  • ShopifyAdapter emits plain arrays (not V2 ValueObjects), so this isn't part of the OpenAPI golden-fixture contract in the root CLAUDE.md — no fixture update needed here, and none was made. Correct.
  • Test replacement (testVariantWithOwnImageCarriesImageUrl) covers both the with-image and without-image (image: null) variant cases in one test — reasonable coverage for the change.

Minor, non-blocking notes

  • .phpunit.cache/test-results is included in the diff. It's already a tracked file from before this PR (not introduced here), so this is just pre-existing churn, not something this PR needs to fix.
  • The test's mocked variant image includes width/height fields that go unused by the new code (only url is read) — harmless, just noting it's consistent with the companion app PR only needing to fetch image { url }.

Note on verification: I wasn't able to execute vendor/bin/phpunit, phpstan, or phpcs in this environment — Bash tool calls for those commands required approval that isn't available here. I reviewed the diff and surrounding code manually instead; if you'd like me to actually run the quality-gate triple, the --allowedTools for this workflow would need to permit vendor/bin/* execution.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@PauliusInvertus
PauliusInvertus marked this pull request as ready for review August 28, 2026 14:21
@PauliusInvertus
PauliusInvertus changed the base branch from main to releases/2026.05 September 1, 2026 11:33
@PauliusInvertus
PauliusInvertus merged commit 6191cbf into releases/2026.05 Sep 1, 2026
5 checks passed
PauliusInvertus added a commit that referenced this pull request Sep 2, 2026
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants