[boo-code]

Questions	Answers
Branch?	dev
Description?	Adds the Admin API endpoints for attachments: POST /attachments, GET /attachments/{attachmentId}, POST /attachments/{attachmentId} (update), DELETE /attachments/{attachmentId}, GET /attachments and DELETE /attachments/bulk-delete. The creation and the update are multipart requests: the file part is required to create, optional to update (the current file is kept), and the localized names and descriptions are required on both since EditAttachmentHandler writes them whatever was sent. The update is a POST because PHP only parses the multipart body of a POST. The get, create and update operations declare minVersion 9.2.1: GetAttachmentForEditing only accepts the attachmentId the API builds it from with PrestaShop/PrestaShop#42124, older cores answer 500. Until it merges, the 9.2.x and develop legs of the CI are red on those operations. Covered by AttachmentEndpointTest: 16 tests / 137 assertions with #42124 applied on a 9.2.1 core.
Type?	new feature
Category?	WS
BC breaks?	no
Deprecations?	no
How to test?	Run AttachmentEndpointTest on a core with PrestaShop/PrestaShop#42124. Or: POST a multipart request to /attachments with file, names[<locale>], descriptions[<locale>]; GET and DELETE /attachments/{attachmentId}; POST a multipart request to /attachments/{attachmentId} with names[<locale>], descriptions[<locale>] and optionally file.
Fixed ticket?	Contributes to PrestaShop/PrestaShop#39630
Related PRs	Depends on PrestaShop/PrestaShop#42124 (9.2.x) and its develop twin PrestaShop/PrestaShop#41889: they expose the attachment id and accept scalar ids in the for-editing CQRS, required by the get, create and update operations
Sponsor company	Boring Investments

[ps-jarvis]

Hello @boo-code!

This is your first pull request on ps_apiresources repository of the PrestaShop project.

Thank you, and welcome to this Open Source community!

[mattgoud]

hey @boo-code 👋 nice work on this.

one thought while going through the Attachment domain: this PR is currently blocked on core PrestaShop/PrestaShop#41889 because GetAttachmentForEditing doesn't expose the attachment id (needed for the GET/POST responses).

GetAttachmentInformation already returns the id (+ localized names/descriptions, fileName, mimeType, fileSize). if you back the GET (and the create response) with it instead of GetAttachmentForEditing, this PR wouldn't need #41889 at all — and it'd also cover the GetAttachmentInformation query that's still listed as missing in the tracking (PrestaShop/PrestaShop#39630).

for reference i added the search + file read endpoints in #311 (SearchAttachment + GetAttachment), which don't overlap with the CRUD here. happy to help if useful 👍

[mattgoud]

small correction to my previous comment 🙇 — switching to GetAttachmentInformation would only remove the need to expose the id in EditableAttachment; the scalar-id constructors from PrestaShop/PrestaShop#41889 are still needed for the PATCH (every other API-exposed Edit command already takes a scalar id, EditAttachmentCommand is the only one still taking a value object). so #41889 stays useful either way — sorry for the noise!

[mattgoud]

update: #311 now also exposes GetAttachmentInformation (at GET /attachments/{attachmentId}/information), so my earlier "it'd also cover the missing Information query" point is moot — that's handled there now. no need to change #295's design: keeping GetAttachmentForEditing + PrestaShop/PrestaShop#41889 here is the right call (and #41889's scalar-id constructors are needed for the PATCH regardless). sorry for the back-and-forth! 🙏

[boo-code]

Thanks @mattgoud, and no worries about the back-and-forth. Agreed on the conclusion: keeping GetAttachmentForEditing here is intentional, and the scalar-id constructors from PrestaShop/PrestaShop#41889 are what the PATCH needs to match every other API-exposed Edit command, so #295 stays blocked on that core PR by design. #41889 is approved now, so this should unblock once it merges.

And good that #311 covers SearchAttachment + GetAttachment + GetAttachmentInformation separately, no overlap with the CRUD here. Appreciate the review.

[boo-code]

The six integration jobs were failing on all three core refs with:

Cannot create an instance of "...\Attachment\Query\GetAttachmentForEditing" from serialized data
because its constructor requires the following parameters to be present : "$attachmentIdValue".

GetAttachmentForEditing::__construct takes $attachmentIdValue while the CQRS normalizer builds it from attachmentId, so POST /attachments answers 500. The core side is PrestaShop/PrestaShop#41889 (develop, approved) and PrestaShop/PrestaShop#42124 (9.1.x). 9.0.3 can never carry it, since 9.0.x was deleted on 2026-04-06, so a version check is not sufficient here.

09d3375 adds a reflection capability probe instead: the two POST paths (testAddAttachment and the createAttachment helper used by the bulk-delete test) call markTestSkipped when the constructor's first parameter is not attachmentId. Verified against both states — an unpatched 9.1.x reports attachmentIdValue and skips, a build carrying the core fix reports attachmentId and runs.

So these jobs stop failing now, and the assertions re-activate on their own once the core change is available, with no further push here. The remaining 9.0.3 jobs will keep skipping the POST paths permanently; the rest of the endpoint coverage still runs there.

[LameuleFR]

Since PrestaShop/PrestaShop#41889 and #42124 are still open, the GET, POST and PATCH answer 500 on every core of the CI matrix, and this probe turns that into skips: only the empty-list bulk case and the 401 checks run, so the green CI does not exercise any endpoint. Could these three operations declare minVersion on the first core release that ships the fix, with markTestSkippedByMinVersion() in the tests (as in CountryEndpointTest), instead of the reflection check?

[LameuleFR]

A multipart PATCH reaches the command empty in a real request, since PHP only parses multipart bodies for POST and Symfony re-parses PATCH bodies only when urlencoded; the test passes because the test client injects the parameters. VirtualProductFile and ProductImage use a CQRSUpdate with method: CQRSUpdate::METHOD_POST and status: 200 for this reason, could you do the same? Also, EditAttachmentHandler writes names and descriptions unconditionally (no names ends in a TypeError from getLocalizedNames(), no descriptions clears them), so both look required on that operation until the core handler only applies the sent fields.

[LameuleFR]

A POST without file passes validation, then AddAttachmentHandler throws EmptyFileException, which is not mapped, so it answers 500. Could you require it in the Create group and keep it out of the read schema, like VirtualProductFile::$file?

Suggested change

	public ?File $file;
	#[ApiProperty(readable: false, writable: false)]
	#[Assert\NotNull(groups: ['Create'])]
	public ?File $file = null;

[LameuleFR]

BulkDeleteAttachmentsHandler collects each failure and throws BulkDeleteAttachmentsException at the end, which is not mapped, so an unknown id answers 500 (and the AttachmentNotFoundException entry never applies); [0] also ends in a 500 through the AttachmentConstraintException of AttachmentId. Could you add #[Assert\All([new Assert\Type('integer'), new Assert\Positive()])] and allowEmptyBody: false as in BulkCountries, map BulkDeleteAttachmentsException (422 seems the closest fit) and test both cases?

[LameuleFR]

The _decorator factory turns file_size into a human-readable size and products into a translated "N product(s)" label, which an API client cannot use, unlike the int fileSize of /attachments/{attachmentId}/information. Could you use prestashop.core.grid.data_factory.attachment and type both as int?

[boo-code]

All done, with one deviation:

• minVersion 9.2.1 on the get, create and update operations, the milestone of Expose the attachment id and accept scalar ids in attachment for-editing CQRS PrestaShop#42124 (develop gets it on merge-up); the reflection probe is gone and the tests use markTestSkippedByMinVersion('9.2.1'). The 9.2.x and develop legs stay red until it merges.
• The update is POST /attachments/{attachmentId}: CQRSUpdate, METHOD_POST, status 200, multipart. names (NotBlank) and descriptions (NotNull) are required in Create and Update.
• file: NotNull in Create, readable: false, writable: false. The deviation: it is public File $file; like VirtualProductFile::$file, not ?File $file = null. With the nullable default, 9.2.1 still normalizes "file": null into the POST response despite readable: false (measured). The cost is the one UPLOAD.md notes for VirtualProductFile: the part is documented as required on the update too.
• EmptyFileException mapped to 422 as well.
• Bulk: Assert\All([Type('integer'), Positive]), allowEmptyBody: false, BulkDeleteAttachmentsException -> 422, with tests for an unknown id, [0] and ["3abc"].
• List on prestashop.core.grid.data_factory.attachment; fileSize (bytes) and products are ints.

Not covered: replacing the file through the update. AttachmentFileUploader stores the file with move_uploaded_file(), which stores nothing for a test upload, so the replacement only unlinks a missing file and warns.

With PrestaShop/PrestaShop#42124 applied on a 9.2.1 core: 16/16, 137 assertions. The new tests on the previous resource: 6 failures.

[jolelievre]

PrestaShop/PrestaShop#43369 (9.2.1) reads the item type of an array from its @var int[] / @var Foo[] docblock, and documents the request body of a DELETE with allowEmptyBody: false, so the hand-written openapiContext can shrink. Once it is merged:

• The openapiContext type and items can go (the @var docblock is there) in BulkAttachments.php:56.

On older cores the arrays are documented without item type, the behaviour is the same. Keep the example in openapiContext when there is one (#[ApiProperty(openapiContext: ['example' => [1, 3]])]).