Add the missing Product domain endpoints

[jolelievre]

Questions	Answers
Description?	Centralizes all the pending Product-domain Admin API endpoint PRs into a single PR, with consolidated resources and API-only integration tests
Type?	new feature
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes PrestaShop/PrestaShop#42054, related to PrestaShop/PrestaShop#39630
Sponsor company	PrestaShop SA

What this PR does

Merges the content of #256, #268, #269, #308, #337, #353, #354, #361, #374, #383 and #384 (all authored by @PrestaEdit, co-authored on the commits) into one PR, then reworks the endpoints and tests as a whole.

Endpoints

Method	URI	CQRS
GET	/products/{productId}/suppliers	GetProductSupplierOptions
PUT	/products/{productId}/suppliers	SetSuppliersCommand
PATCH	/products/{productId}/suppliers	UpdateProductSuppliersCommand
DELETE	/products/{productId}/suppliers	RemoveAllAssociatedProductSuppliersCommand
PUT	/products/{productId}/default-supplier	SetProductDefaultSupplierCommand
GET	/products/{productId}/shop-images	GetShopProductImages
PUT	/products/{productId}/shop-images	SetProductImagesForAllShopCommand
PATCH	/products/{productId}/stock	UpdateProductStockAvailableCommand
PATCH	/products/combinations/{combinationId}/stock	UpdateCombinationStockAvailableCommand
GET	/products/{productId}/stock-movements	GetProductStockMovements
GET	/products/{productId}/attribute-groups	GetProductAttributeGroups
POST	/products/{productId}/virtual-files	AddVirtualProductFileCommand
POST	/products/{productId}/virtual-files/{virtualProductFileId}	UpdateVirtualProductFileCommand
DELETE	/products/virtual-files/{virtualProductFileId}	DeleteVirtualProductFileCommand
GET	/products/free-gift-candidates	SearchProductsForFreeGift (minVersion: 9.2.0)

Design decisions

• Minimal resource classes: one class per structure, multiple operations per class (ProductSuppliers, ShopProductImages, VirtualProductFile, ...).
• Read/write symmetry: GET and write operations share the same URI and properties; every write returns the updated content through a CQRSQuery (the shop-images PUT returns the same {productId, shopImages: [{shopId, images: [{imageId, cover}]}]} resource as the GET).
• Singular URIs for sub-resources that exist at most once per product (default-supplier, stock), with matching Rector keyword exceptions.
• File uploads use multipart POST: the product images and the virtual product file take the file with the request, so their create and update operations are POST (PHP only fills the uploaded files of a request for that method).
• The shop-images operations always cover all the shops of the product: the CQRS query and command behind them take a product id and no shop constraint, which their OpenAPI descriptions state explicitly.
• GetProductIsEnabled, GetAssociatedSuppliers, SearchProductsForAssociation, SearchCombinationsForAssociation and SearchProductCombinations are intentionally NOT exposed: they duplicate existing endpoints (see GenerateApiTrackingTableCommand::EXCLUDED_CQRS_CLASSES). This supersedes Add GetProductIsEnabled Admin API endpoint #352 and Add Product associated-suppliers read endpoint #286.
• Product now exposes virtualProductFile (read side of the virtual file endpoints).
• Add bulk setProductImageSettings() to SetProductImagesForAllShopCommand PrestaShop#42049 is not needed anymore: the serializer cannot build the nested ProductImageSetting value objects even with the bulk setter, so the command is built by a dedicated module denormalizer (SetProductImagesForAllShopSerializer), which works on every supported core version.

Tests

Every endpoint has integration coverage where all fixtures are created through the Admin API itself (products, suppliers, attribute groups, attributes, combinations, images, stock) — no ObjectModel, no command-bus seeding, no raw SQL fixtures. Assertions compare the complete JSON structure of each entity with a single assertEquals so missing or extra fields fail the test.

⚠️ Core dependency

PATCH /products/{productId}/stock requires the StockMvt employee guard from PrestaShop/PrestaShop#41803: without it, any stock update through the Admin API fails with a TypeError (this is also why the original #256 CI was red). The two stock test classes stay red on CI until that core PR is merged into the target branches.

The stock-movements endpoint also exposes apiClientIds/apiClientNames: cores that include PrestaShop/PrestaShop#41803 record which API client created each movement (through the mutation table) and the endpoint returns it. On older cores the fields are empty arrays, and the test assertions are feature-detected accordingly.

How to test

Run the module integration suite against a 9.2.x core that includes PrestaShop/PrestaShop#41803, e.g. locally:

composer create-test-db
_PS_ROOT_DIR_=$(pwd) php -d date.timezone=UTC ./vendor/phpunit/phpunit/phpunit -c modules/ps_apiresources/tests/Integration/phpunit-local.xml

Full suite is green locally (587 tests, 4799 assertions) with the #41803 guard applied.

This PR supersedes #256, #268, #269, #308, #337, #352 (excluded), #353, #354, #361, #374, #383, #384 and #286 (excluded).

[mattgoud]

Reviewed the whole PR resource-by-resource against core develop 👍 Overall this is solid: the consolidation is clean, read/write symmetry is consistent, the custom SetProductImagesForAllShopSerializer correctly replaces the abandoned core #42049, the Rector singular-URI exceptions are sound, the version gating (minVersion: 9.2.0) proves both sides, and the earlier per-endpoint defects (#337 dead 404 mapping, #374 disabledReason/imageUrl) are all fixed. Tests build fixtures through the Admin API with full-JSON assertEquals — nice.

One recurring thing worth addressing before merge, plus a few isolated nits.

Input validation: 500 instead of 4xx on malformed bodies (4 resources)

On several write/search endpoints an invalid body surfaces as a 500 because the domain exception isn't in exceptionToStatus, and there's no testInvalid* covering it (the module convention asks for one):

• ShopProductImages (PUT) — a shopImages item without shopIds → array_map('intval', null) TypeError → 500; imageId <= 0 (or missing → 0) → ProductImageConstraintException (unmapped) → 500. The write path denormalizes straight into the command via SetProductImagesForAllShopSerializer, so the resource's #[Assert\NotBlank] on $shopImages never runs (dead constraint). Suggest guarding the body in the serializer (or mapping the constraint exception to 422/400).
• ProductSuppliers (PUT) — empty supplierIds → Core\Exception\InvalidArgumentException (unmapped) → 500.
• FreeGiftCandidate (GET) — phrase < 3 chars or limit <= 0 → ProductConstraintException (unmapped) → 500. The openapiContext minLength/minimum are doc-only, not enforced.
• ProductAttributeGroupList (GET) — productId <= 0 → ProductConstraintException (unmapped); also this operation is missing the requirements: ['productId' => '\d+'] its sibling product resources declare.

Adding the exception mappings (+ the \d+ requirement) and a testInvalid* per resource would close this consistently.

Isolated nits (non-blocking)

• Resources/Product/Product.php — the virtualProductFile doc comment says the plural /products/{productId}/virtual-files, but the actual URI is singular virtual-file.
• ProductSuppliersEndpointTest — testSetDefaultSupplier asserts only defaultSupplierId instead of the full JSON like the other ops; and the supplier fixture uses a raw \Db::getInstance()->getValue(...) to look up the FR country id, slightly at odds with the "no SQL seeding" approach.
• SetProductImagesForAllShopSerializer::getSupportedTypes() returns 'object' => null, '*' => null (non-cacheable for every type) — narrowing it to [SetProductImagesForAllShopCommand::class => true] is cleaner. supportsDenormalization itself is correctly narrow.
• ProductStockMovements declares offset/limit twice (QueryParameters + openapiContext.parameters) — harmless duplication.

None of these are correctness bugs for well-formed traffic — the 4xx-vs-500 hardening is the one I'd treat as a should-fix.

[github-actions]

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

📋 Summary of changes

This PR consolidates 10+ pending PRs into a single Product-domain endpoint batch, adding 8 new resource classes and 7 new integration test files (~2 500 lines). It exposes suppliers (GET/PUT/PATCH/DELETE /products/{productId}/suppliers + PUT /products/{productId}/default-supplier), shop-image associations (GET/PUT /products/{productId}/shop-images), stock updates (PUT /products/{productId}/stock and /products/combinations/{combinationId}/stock, both gated at minVersion: 9.2.0), stock movements (GET /products/{productId}/stock-movements), product attribute groups (GET /products/{productId}/attribute-groups), virtual-file management (POST/PATCH/DELETE), and a free-gift search (GET /products/free-gift-candidates, minVersion: 9.2.0). All map to Core CQRS commands/queries from the Product domain.

⏱️ Estimated review time

35–50 minutes — large surface (8 resource classes, 7 test classes, 2 custom normalizers, PHPStan config, Rector allowlist). Several architectural decisions that need explicit sign-off.

🎯 Scope

• Exposed operations: GET, PUT, PATCH, DELETE, POST
• CQRS entity: Multiple Product sub-domains — Supplier, Stock, Image, VirtualProductFile, AttributeGroup, StockMovements, FreeGift
• Integration test: yes — full test file per resource, fixtures created entirely via the API

🧱 API Platform / CQRS architecture compliance

1. Custom denormalizer SetProductImagesForAllShopSerializer — acknowledged rule deviation

src/ApiPlatform/Normalizer/SetProductImagesForAllShopSerializer.php is a new DenormalizerInterface added in the module. CONTEXT.md explicitly forbids custom normalizers/processors.

The PR author justifies it: SetProductImagesForAllShopCommand collects ProductImageSetting value objects through an addProductSetting() adder that the generic serializer cannot drive from a JSON body — the same situation as the pre-existing GenerateCombinationsSerializer. It is correctly added to the PHPStan ApiResourceNormalizerRule allowlist.

Action required: The reviewer must explicitly accept this exception. The justification is technically sound, but each new allowlisted normalizer needs a human sign-off.

---

2. AttributeGroupWithAttributesNormalizer scope expansion

The existing normalizer (already in the allowlist) is extended to trigger for ProductAttributeGroupList. Since both classes expose the same Core AttributeGroup query result, sharing the normalizer is consistent. No new normalizer class is introduced.

---

3. VirtualProductFile.php — missing validationContext on CQRSCreate

The CQRSCreate operation declares no validationContext, yet the DTO has #[Assert\NotBlank(groups: ['Create'])] on $filePath and $displayName. Without validationContext: ['groups' => ['Default', 'Create']], Symfony only runs the Default group — the Create-group constraints are silently skipped. A missing field produces a PHP type error rather than a clean 422.

Suggestion: Add validationContext: ['groups' => ['Default', 'Create']] to the CQRSCreate operation, matching the pattern in Contact.php.

---

4. VirtualProductFile.php — no CQRSQuery on CQRSPartialUpdate

CONTEXT.md: "Use a CQRSQuery on CQRSCreate and CQRSPartialUpdate when the endpoint should return the full updated state." The PATCH has no CQRSQuery, so the response contains only the identifier. The integration test compensates by doing a separate GET on the parent product — callers cannot observe the update atomically.

Suggestion: Add CQRSQuery: GetProductForEditing::class (and matching CQRSQueryMapping) to the PATCH operation.

---

5. ProductStockMovements.php — no #[ApiProperty(identifier: true)]

CQRSGetCollection items should carry #[ApiProperty(identifier: true)] for API Platform IRI generation. ProductStockMovements has no scalar identifier (movements use a stockMovementIds array). Compare with FreeGiftCandidate, which correctly marks $productId. If items are never addressed individually this is defensible, but the absence may cause API Platform to silently pick the wrong field.

---

6. ProductAttributeGroupList.php — $attributeGroupId not marked as identifier

public int $attributeGroupId; has no #[ApiProperty(identifier: true)]. Each item in the /products/{productId}/attribute-groups collection should have its natural identifier declared — looks like an oversight given FreeGiftCandidate correctly marks its identifier.

---

7. Command-parameter alignment (unverifiable without Core)

Two write operations carry no CQRSCommandMapping:

Operation	Command	DTO field assumed to match command param
PUT /products/{productId}/suppliers	SetSuppliersCommand	supplierIds
PUT /products/{productId}/default-supplier	SetProductDefaultSupplierCommand	defaultSupplierId

If the Core command constructors use different parameter names, values are silently dropped. The reviewer should verify command signatures in Core match the DTO property names.

---

8. Read/write asymmetry in ShopProductImages

GET response: [{shopId, images: [{imageId, cover}]}] (keyed by shop).
PUT payload: [{imageId, shopIds}] (keyed by image).

Intentional and documented, but callers cannot round-trip the GET response as a PUT payload. Worth calling out in documentation/changelog.

💡 Improvement suggestions

VirtualProductFileEndpointTest.testDeleteVirtualProductFile — fragile null check

$this->assertArrayNotHasKey('virtualProductFile', $product) silently passes if the serializer emits "virtualProductFile": null instead of omitting the key. Consider $this->assertNull($product['virtualProductFile'] ?? null) or document which serializer config guarantees key omission when null.

---

ProductSuppliersEndpointTest.testSetDefaultSupplier — partial response assertion

Only $updatedSuppliers['defaultSupplierId'] is checked. For consistency with the rest of the test class (full assertEquals over the whole response), consider asserting all fields.

---

FreeGiftCandidate.php — no exceptionToStatus

If SearchProductsForFreeGift can surface domain exceptions they should be mapped. If not, a brief comment clarifying that intent would help.

---

ProductStockMovementsEndpointTest — raw SQL for API client lookup

\Db::getInstance()->getValue('SELECT id_api_client FROM ...') couples the test to the DB schema. If ApiTestCase exposes a helper for this, that should be preferred.

✅ Pre-review checklist

URI & routing

• URI is plural, lowercase, kebab-case (/suppliers, /shop-images, /stock-movements, /attribute-groups, /free-gift-candidates; stock, virtual-file, default-supplier are singular by documented design decision and correctly added to the Rector exceptions list)
• Identifier uses domain name + Id suffix (productId, combinationId, virtualProductFileId)
• Sub-resources follow parent path
• Bulk operation URI uses bulk- prefix and plural Ids parameter (N/A)

Operations & scopes

• Correct operation attribute per HTTP method
• Scope format: product_read / product_write, singular form

API Resource properties

• All properties strictly typed, scalars/arrays only (no Value Objects)
• Naming conventions respected (disabled not active, no is prefix, no localized prefix)
• #[ApiProperty(identifier: true)] on ID property — missing on ProductStockMovements (no scalar identifier by design), missing on ProductAttributeGroupList.$attributeGroupId (apparent oversight)
• #[LocalizedValue] on localized fields, #[DefaultLanguage] with correct fieldName (where applicable)

CQRS mapping

• QUERY_MAPPING direction: QueryResult field → API field (verified for all new resources)
• CQRSCommandMapping direction: API field → Command parameter
• CQRSQuery present on CQRSCreate/CQRSPartialUpdate when full object must be returned — missing on VirtualProductFile CQRSPartialUpdate
• No SerializedName — mappings only

Forbidden practices (CI-enforced)

• No custom normalizers or processors — SetProductImagesForAllShopSerializer is a new custom denormalizer (forbidden by CONTEXT.md; justified by adder-based command pattern; added to PHPStan allowlist). Requires explicit human sign-off.
• No Value Objects in properties

Exception handling & validation

• ConstraintException → 422, NotFoundException → 404 (where applicable)
• Correct validationContext groups on Create / Update operations — missing on VirtualProductFile CQRSCreate

Multi-shop

• shopIds present when entity is shop-associated; ShopProductImages correctly handles multistore context
• Shop context ([_context][shopConstraint], [_context][shopId]) passed when needed

Listing field alignment (N/A — no PaginatedList or CQRSPaginate added in this PR)

• (not applicable)

Integration test

• testInvalid* with assertValidationErrors — missing in CombinationStockEndpointTest, ProductStockEndpointTest, ProductSuppliersEndpointTest, ShopProductImagesEndpointTest ($shopImages has #[Assert\NotBlank] but no empty-payload 422 test)
• getProtectedEndpoints() lists all URIs (version-gated classes correctly yield unconditionally)
• DatabaseDump::restoreTables() covers all affected tables
• declare(strict_types=1) present in all new test files

[jolelievre]

@ibahloul-ps the review points are addressed in the last commits:

• Virtual product file: the file is now uploaded with the request (file multipart part, mapped to the command filePath). Create is multipart only; update is a POST (like the product image) that accepts JSON to keep the current file or multipart to replace it. Both POST operations have explicit OpenAPI summaries.
• Attribute groups: nested names are documented with a locale-keyed example ({"en-US": "M", "fr-FR": "M"}) plus a full attributes example.
• Stock movements: the endpoint description explains the grouping (edition = one manual update, orders = consecutive order movements aggregated between two editions), hence the id lists, the summed deltaQuantity and the add vs from/to dates. Each field has a description.
• Product and combination stock: CQRSPartialUpdate (PATCH) instead of PUT.
• Shop images PUT: the payload now uses the same shape as the GET response ([{shopId, images: [{imageId, cover}]}], cover ignored), so a response can be sent back as is. Invalid payloads return a 422 instead of a 500, the request body is documented explicitly, and removing a cover image returns a 422.
• PHPStan 9.1.5: SearchProductsForFreeGift ignored like in the 9.0.3 config.

[Quetzacoalt91]

A first set of comments

[Quetzacoalt91]

Can you please update the PR description to match this URL ? You mistakenly wrote /products/virtual-file/{virtualProductFileId}

[jolelievre]

Good catch, even if the productId is not useful here, at least the REST convention is respected

[Quetzacoalt91]

@jolelievre, because the relation between the product and virtual files is 1:1, what do you think about these URLs:

Method	URI	CQRS
POST	/products/{productId}/virtual-file	AddVirtualProductFileCommand
PATCH	/products/virtual-files/{virtualProductFileId}	UpdateVirtualProductFileCommand
DELETE	/products/virtual-files/{virtualProductFileId}	DeleteVirtualProductFileCommand

This almost follows what you wrote, expect virtual-files is in plural form to follow the URLs of combinations and images.

[Quetzacoalt91]

Partial testing of the changes

Product suppliers: OK 🟢

• Scenario 1: Remove and bring back a supplier of the product 13

  • Check Initial state

• Remove Supplier ID 2 from the product 13

• Confirm state with GET

• Add supplier ID 2 to product 13

Price is 0 and the reference empty. It could be updated.

• Update supplier 2 for Product 13 with original values

• Confirm state with GET

• Delete the suppliers of the product 13

[jolelievre]

Regarding the virtual-files plural suggestion, I don't mind

Actually, I force the singular form on purpose (even by adding an exception in the rector rule) because today a product can have only one virtual product, and I wanted to emphasize this

But it also makes sense to favor respecting the plural form for convention (even if there's only one). I'll lean to the plural form for another reason though: if in the future we improve the virtual product and allow it to have multiple virtual files, at least the API URLs will natively be able to handle them

For the other singular exceptions, stock and default-supplier I think those two still make sense even in the future. The default supplier, without a doubt, there will always be only one default supplier among the list For the stock I wonder if a product could have multiple stocks, but since we removed the notion of advanced stock management and warehouses, I don't think so

I'm waiting for @ibahloul-ps feedback, and then I'll handle them all in one go

[jolelievre]

@ibahloul-ps @Quetzacoalt91 latest commits:

• Virtual file URIs are plural: POST /products/{productId}/virtual-files, POST /products/{productId}/virtual-files/{virtualProductFileId}, DELETE /products/virtual-files/{virtualProductFileId}. PR description updated.
• file is no longer documented in the responses, only as the multipart binary part.
• expirationDate, accessDays and downloadTimesLimit are returned as null instead of being omitted.
• Shop images: both operations document that they always cover every shop. The query and command take a product id and no shop constraint, so shopId, shopGroupId, shopIds and allShops never restrict them.
• Attribute groups: \d+ requirement plus the constraint exception mapped, so 0 returns 422 instead of 500.
• The custom serializers declare only the command they handle: the 'object' / '*' entries were disabling the serializer support cache for every type.

Unchanged, on purpose:

• productId in the create body comes from the command schema, and the core CQRSOpenApiFactoryTest asserts it for this kind of multipart body.
• 200 on the update: the file already exists.
• productId ignored on the update: it builds the response, and no query reads a virtual file by its own id.
• 200 on shop images for an unknown product: needs a guard in GetShopProductImagesHandler.

Question: the DELETE is the only one without productId, since the create needs it for the command and the update for the response. Add it back for symmetry even though it would be decorative there?

[Quetzacoalt91]

Question: the DELETE is the only one without productId, since the create needs it for the command and the update for the response. Add it back for symmetry even though it would be decorative there?

I'd prefer to see the product ID but that's entirely your call. :)

[Quetzacoalt91]

Blocked by PrestaShop/PrestaShop#42960