Add GetShopProductImages Admin API endpoint

[PrestaEdit]

Questions	Answers
Branch?	dev
Description?	Adds GET /products/{productId}/shop-images using CQRSGetCollection with GetShopProductImages. Returns per-shop image associations: each element = {shopId, productImages: [{imageId, cover}, ...]}. Complements the existing GET /products/{productId}/images (ProductImageList, single-shop) with the multi-shop view. Unknown product id returns 200 [] (no exception), matching the underlying all-shops query.
Type?	new feature
Category?	CO
BC breaks?	no
Deprecations?	no
Fixed ticket?	Related to PrestaShop/PrestaShop#39630
How to test?	Run composer phpunit-integration; ShopProductImagesEndpointTest covers scope protection and the list happy path, asserting the exact per-shop {imageId, cover} grouping against the fixture data (image_shop table).

[github-actions]

🤖 Claude AI Pre-Review — Automated analysis. Does not replace human review.

📋 Summary of changes

Adds a new GET /products/{productId}/shop-images endpoint backed by the CQRSGetCollection operation, using the Core GetShopProductImages query. The endpoint returns a non-paginated collection grouping product images by shop ([{shopId, productImages: [{imageId, cover}, ...]}, ...]), acting as the multi-shop counterpart to the existing GET /products/{productId}/images (ProductImageList). An integration test ShopProductImagesEndpointTest is included.

⏱️ Estimated review time

10–15 minutes — the diff is small but several conventions from the analogous ProductImageList.php are missing from both the resource class and the test.

🎯 Scope

• Exposed operations: GET (non-paginated collection)
• CQRS entity: GetShopProductImages query
• Integration test: yes (partial — see issues below)

🧱 API Platform / CQRS architecture compliance

1. Missing exceptionToStatus — hard blocker

ShopProductImages.php declares no exceptionToStatus map. The directly analogous resource ProductImageList.php maps:

exceptionToStatus: [
    ProductNotFoundException::class => Response::HTTP_NOT_FOUND,
],

Without this, a GET /products/999999/shop-images with a non-existent product ID will produce an unhandled exception instead of a 404. The PR description explicitly promises "unknown-id → 404" but the implementation cannot deliver it.

Fix: Add exceptionToStatus at the #[ApiResource] level, mirroring ProductImageList.php.

---

2. Missing CQRSQueryMapping — likely blocker

ProductImageList.php (the single-shop equivalent) passes shop context to the query:

CQRSQueryMapping: [
    '[_context][shopConstraint]' => '[shopConstraint]',
],

ShopProductImages.php has no CQRSQueryMapping at all. Whether GetShopProductImages requires a shop constraint (or any other injection) needs to be verified against the Core query's constructor. If the query accepts a ShopConstraint, omitting this mapping means the collection is always fetched without shop context.

Fix: Check GetShopProductImages constructor signature in Core and add the corresponding CQRSQueryMapping entry if needed.

---

3. Missing ApiResourceMapping — needs verification

ProductImageList.php maps '[localizedLegends]' => '[legends]' because the Core query result field name differs from the DTO property. The new DTO exposes shopId and productImages — the Core query result must return fields with exactly those names for the mapping to be implicit. If the query result uses shop_id, images, or any other variant, the properties will always be null at runtime.

Fix: Read GetShopProductImages result class in Core and add ApiResourceMapping entries for any mismatched field names.

---

4. productId absent from DTO

ProductImageList.php exposes public int $productId as a DTO property. ShopProductImages.php does not. The URL parameter {productId} is passed to the query by the framework through auto-mapping, which relies on the query constructor parameter being named productId. This is likely fine but should be confirmed against the Core query signature. Not including productId in the DTO means it won't appear in the response body, which may or may not be intentional.

💡 Improvement suggestions

Test: getItem() used for a collection endpoint

CQRSGetCollection returns an array (non-paginated collection), not a single item. The test calls $this->getItem(...) which is the single-item GET helper. Semantically this is incorrect — requestApi() with a manual assertion, or a dedicated collection assertion, would be clearer. This may also silently pass because getItem() is a thin wrapper over GET, but it documents the intent incorrectly.

---

Test: assertions are too shallow

The test only checks that shopId and productImages keys exist and are the right types. Per the module conventions, integration tests should assert all fields — including verifying actual shopId values match the shop fixture and that productImages items contain imageId (int) and cover (bool) with concrete values, not just type assertions.

---

Test: no 404 scenario

The PR description promises "unknown-id → 404" but there is no testListShopProductImagesNotFound() method in the test. Once exceptionToStatus is added (see blocker #1), a test covering this path should be added.

---

Test: missing DatabaseDump::restoreTables()

While this is a read-only endpoint, if future tests in the same class (e.g. a seed step for a writable endpoint) modify the DB, the absence of restore logic would leave state. The canonical ContactEndpointTest always pairs setUpBeforeClass/tearDownAfterClass with restoreTables. Consider adding it defensively even for read-only tests.

✅ Pre-review checklist

URI & routing

• URI is plural, lowercase, kebab-case (/products/{productId}/shop-images)
• Identifier uses domain name + Id suffix (productId in URI)
• Sub-resources follow parent path (/products/{productId}/…)
• Bulk operation URI uses bulk- prefix and plural Ids parameter (N/A)

Operations & scopes

• Correct operation attribute per HTTP method (CQRSGetCollection for GET collection)
• Scope format: product_read — correct

API Resource properties

• All properties strictly typed (int $shopId, array $productImages)
• Naming conventions respected (no is prefix, no localized prefix)
• #[ApiProperty(identifier: true)] on ID property — $shopId is not marked as identifier; productId is not exposed as a DTO property at all (compare ProductImageList.php which exposes public int $productId)
• #[LocalizedValue] / #[DefaultLanguage] (N/A — no localized fields)

CQRS mapping

• QUERY_MAPPING direction — no CQRSQueryMapping defined; analogous ProductImageList.php has '[_context][shopConstraint]' => '[shopConstraint]'
• CQRSCommandMapping direction (N/A — read-only)
• CQRSQuery present on CQRSCreate/CQRSPartialUpdate (N/A)
• No SerializedName — mappings only

Forbidden practices (CI-enforced)

• No custom normalizers or processors
• No Value Objects in properties

Exception handling & validation

• ConstraintException → 422, NotFoundException → 404 — exceptionToStatus is entirely absent; ProductNotFoundException → 404 is missing
• Correct validationContext groups (N/A — GET only)

Multi-shop

• shopIds present/absent appropriately (N/A for this endpoint's purpose — it returns per-shop data, not a shop association array)
• Shop context ([_context][shopConstraint]) — not passed to query; needs verification against Core query signature

Listing field alignment (collection endpoint)

• DTO properties match fields from CQRS query result — unverified; ApiResourceMapping absent; if Core uses different field names the properties will be null
• ApiResourceMapping covers every name mismatch — none defined; needs verification
• filtersMapping (N/A — not a paginated list)
• No orphan DTO property — cannot confirm without reading Core query result class

Integration test

• Extends ApiTestCase
• @depends chain — N/A for single test, but no 404 test exists either
• Asserts all fields — only checks key presence and types, not actual values
• getProtectedEndpoints() lists the endpoint URI
• DatabaseDump::restoreTables() — absent (acceptable for read-only but inconsistent with canonical pattern)
• declare(strict_types=1) present
• testInvalid* / testNotFound* — no negative-path test despite PR description claiming "unknown-id → 404"

[mattgoud]

Synthesis on top of the AI pre-review 👇

Overstated / false positives:

• "Missing exceptionToStatus = hard blocker / 500": not a 500. GetShopProductImages returns 200 [] for an unknown product (getImagesFromAllShop / getAssociatedShopIds never throw ProductNotFoundException). So there is no unhandled exception, and CI is green.
• Shop-constraint and ApiResourceMapping mismatch flags are false positives too: this is an all-shops query by design, and the nested {shopId, productImages:[{imageId, cover}]} shape normalizes correctly (green happy path proves it).

One real decision: this endpoint behaves differently from its sibling ProductImageList, which maps ProductNotFoundException → 404. Either align (add exceptionToStatus + a 404 test) or keep 200 [] and drop the "unknown-id → 404" line from the description so the contract is accurate.

Worth doing (test): assert the nested imageId/cover values against a product with known images across shops, not just key presence + types, otherwise the per-shop grouping (the whole point vs the single-shop sibling) is exercised by nobody.

[mattgoud]

Thanks for strengthening the test 👍 but the new fixture query broke CI on every integration leg:

SELECT `id_product` FROM `ps_image_shop` ORDER BY `id_product` ASC LIMIT 1 LIMIT 1
                                                                       ^^^^^^^^^^^^^
PrestaShopDatabaseException: SQL syntax ... near 'LIMIT 1'

The LIMIT 1 is duplicated: the query string already ends with LIMIT 1 and the DB helper (getRow / getValue) appends its own LIMIT 1. Drop the explicit LIMIT 1 from the SQL (or use the helper without it) and the legs go green.

[PrestaEdit]

Good catch, thanks @mattgoud! Fixed in d352d14 — dropped the explicit LIMIT 1 since Db::getValue() already appends its own.

[mattgoud]

LGTM ✅ verified the wiring against core: GetShopProductImages returns the per-shop {imageId, cover} grouping, unknown product → empty 200 [] (no exception, so no exceptionToStatus needed), and it's intentionally all-shops so no shopConstraint mapping. the test asserts the full contract against the image_shop fixture.

non-blocking nit: if the test fixture has more than one shop, a quick assertion that the grouping actually spans multiple shops would nail down the endpoint's whole point — but the grouping logic itself lives in core, so fine to merge as-is.

[jolelievre]

Closing in favor of #410, which centralizes all the pending Product-domain endpoints into a single PR: mutualizing the changes makes the review simpler, and it allowed rebuilding the integration tests so that every fixture is created through the API alone (with whole-structure assertions). @PrestaEdit your work is kept and you are co-authored on the commits of #410 — thanks!

Note: in #410 the endpoint returns a single resource shared with the write operation: {productId, shopImages: [{shopId, images: [{imageId, cover}]}]}.