Add multipart/form-data support via request data extractors by aviator-ua · Pull Request #21 · auto1-oss/service-api-handler-bundle

aviator-ua · 2026-07-23T18:01:04Z

Summary

Adds multipart/form-data support to the argument resolver by extracting request-payload
building into a set of tagged request data extractors, selected per endpoint by request
format. This decouples payload assembly from ServiceRequestResolver and makes it extensible.

What changed

New RequestDataExtractorInterface — supports(EndpointInterface) + extract(Request, EndpointInterface): array, returning the merged payload passed to the serializer's denormalize() call.
DefaultRequestDataExtractor — decodes the request body via the endpoint's request format and merges it with route attributes and query params (previous behaviour). Registered with low priority (-100) as the fallback.
MultipartRequestDataExtractor — handles endpoints whose request format is multipart. Merges request->request, uploaded files (wrapped as UploadedFileStream around a PSR-17 stream), attributes and query params. Registered with high priority (100).
ServiceRequestResolver now receives a !tagged_iterator auto1.api_handler.request_data_extractor and delegates payload extraction to the first supporting extractor.
Service wiring for both extractors in Resources/config/services.yml.
Removed the unused DecodeDenormalizeAwareSerializerInterface.
Tests for both extractors and updated ServiceRequestResolverTest.

Dependencies

The MultipartRequestDataExtractor relies on a PSR-17 StreamFactoryInterface (optional, wired
via @?) and on UploadedFileStream / the Multipart namespace from
auto1-oss/service-api-components-bundle.

⚠️ Blocker before merge

composer.json currently pins the companion bundle to a fork dev branch:

"auto1-oss/service-api-components-bundle": "dev-support-multiform-data",
"repositories": [{ "type": "vcs", "url": "git@github.com:aviator-ua/service-api-components-bundle.git" }]

This must be reverted to a released constraint (e.g. ^1.x) once the corresponding
service-api-components-bundle change is merged and tagged upstream. This PR should not be
merged until then.

aivus · 2026-07-27T13:56:19Z

+            }
+
+            if ($value instanceof UploadedFile) {
+                $stream = $this->streamFactory->createStreamFromFile($value->getRealPath(), 'r');


Blocking: getRealPath() can return false, causing a TypeError (500) instead of a 400.

When an upload fails — UPLOAD_ERR_INI_SIZE, UPLOAD_ERR_PARTIAL, UPLOAD_ERR_NO_TMP_DIR — Symfony still hands you an UploadedFile, but the underlying path doesn't exist, so getRealPath() returns false. Under declare(strict_types=1) that's an uncaught TypeError on a string parameter, and it escapes the catch (ExceptionInterface) in ServiceRequestResolver, so the client gets a 500 rather than a BadRequestHttpException. With post_max_size=256M in CI and typical prod upload limits, this is reachable from the outside with a single oversized file.

Two changes:

if (!$value->isValid()) { throw new BadRequestHttpException( sprintf('Upload failed for field "%s": %s', $key, $value->getErrorMessage()) ); } $stream = $this->streamFactory->createStreamFromFile($value->getPathname(), 'r');

Prefer getPathname() over getRealPath() — it doesn't stat the filesystem and doesn't resolve symlinks (see also the macOS test note in MultipartRequestDataExtractorTest).

Minor, same line: the stream handle is never closed, so you hold one open descriptor per uploaded file until GC. Request-scoped, so probably fine, but worth being deliberate about if large multi-file uploads are expected.

Fixed. wrapFiles() now checks isValid() first and throws BadRequestHttpException with the field name and getErrorMessage(), and uses getPathname() instead of getRealPath() — which also fixed the macOS test expectation as you predicted.

On the open handle: that's intentional — the stream is the payload and must outlive the extractor so the controller can read it. PSR-7 streams close their resource on __destruct, so the descriptor's lifetime is bound to the DTO, which is request-scoped.

aivus · 2026-07-27T13:56:34Z

+            if ($value instanceof UploadedFile) {
+                $stream = $this->streamFactory->createStreamFromFile($value->getRealPath(), 'r');
+                $wrapped[$key] = new UploadedFileStream($stream, $value);
+            }


There's no else branch, so anything that isn't an UploadedFile is silently dropped from the result. In practice that's null for an optional file input the client didn't fill — the key vanishes from the payload entirely rather than arriving as null.

That's probably what you want (the DTO keeps its default), but it's implicit. Either make it explicit:

$wrapped[$key] = $value instanceof UploadedFile ? new UploadedFileStream(..., $value) : null;

or leave the drop and add a one-line comment saying unfilled file inputs are intentionally omitted.

Kept the drop (an explicit null would overwrite DTO defaults during denormalization) and added the one-line comment stating that unfilled optional file inputs are intentionally omitted.

aivus · 2026-07-27T13:56:49Z

+            $request->request->all(),
+            $this->wrapFiles($request->files->all()),


Multipart PUT/PATCH will silently produce an empty payload.

PHP only populates $_POST / $_FILES for POST requests. For a multipart PUT or PATCH, $request->request->all() and $request->files->all() are both empty, and the body is never parsed — so the endpoint denormalizes [] + attributes + query and the caller gets a confusingly empty DTO instead of an error.

If the bundle is expected to support non-POST multipart endpoints, this needs handling (parse the raw body, or reject explicitly). If not, at minimum throw on !$request->isMethod('POST') here so the failure is loud, and document the constraint in the README.

Went with the loud failure: extract() now rejects any non-POST method with a 400 explaining that PHP doesn't parse multipart bodies for other methods, and the README documents the POST-only constraint in the new multipart section.

aivus · 2026-07-27T13:57:07Z

+        if (null === $this->streamFactory) {
+            throw new LogicException(
+                sprintf(
+                    'A PSR-17 "%s" must be wired to handle multipart/form-data endpoints. '
+                    . 'Install a PSR-7 implementation (e.g. guzzlehttp/psr7, nyholm/psr7) '
+                    . 'and register its stream factory.',
+                    StreamFactoryInterface::class
+                )
+            );
+        }


This throws for any multipart endpoint when no PSR-17 factory is wired — including one that only carries text fields and never touches an upload. Since the factory is wired optionally (@?), that turns a soft dependency into a hard one for the whole multipart format.

Moving the null check into wrapFiles(), at the point where an UploadedFile is actually encountered, makes the dependency genuinely optional and matches the DI wiring:

if ($value instanceof UploadedFile) { if (null === $this->streamFactory) { throw new LogicException(/* ... */); } // ... }

testExtractRequiresFactoryEvenWhenNoFilesPresent locks the current behaviour in deliberately, so this is a design call rather than a bug — but I think the lazier check is the better contract. The exception message itself is excellent, by the way; keep it.

Agreed — the null check moved into wrapFiles() at the point an UploadedFile is encountered, and testExtractRequiresFactoryEvenWhenNoFilesPresent was inverted into testExtractDoesNotRequireFactoryWhenNoFilesPresent.

On top of that, there's now a compile-time check (see reply on services.yml), so the runtime LogicException is just the backstop.

aivus · 2026-07-27T13:57:26Z

+
+    public function supports(EndpointInterface $endpoint): bool
+    {
+        return self::FORMAT === $endpoint->getRequestFormat();


Nit / question: supports() keys purely off the endpoint's declared format and never looks at the actual Content-Type. If a client posts JSON to an endpoint declared as multipart, this extractor still wins and returns an empty payload rather than a 400.

Also, self::FORMAT = 'multipart' arguably belongs next to EndpointInterface::getRequestFormat() in service-api-components-bundle, alongside the other format constants — otherwise the string is defined in one repo and consumed in another.

supports() can't see the request (it only receives the endpoint), but extract() now validates the Content-Type header and rejects non-multipart/form-data requests with a 400.

The constant point is resolved: EndpointInterface::FORMAT_MULTIPART already exists in the components bundle next to the other format constants, so the local FORMAT constant is gone and supports() compares against it.

aivus · 2026-07-27T13:59:25Z

+        $this->streamFactory
+            ->expects(self::once())
+            ->method('createStreamFromFile')
+            ->with($targetTmp, 'r')


This assertion is platform-dependent and will fail on macOS.

$targetTmp comes from tempnam(sys_get_temp_dir(), ...), which on macOS returns a path under /var/folders/.... /var is a symlink to /private/var, so the getRealPath() call in the extractor produces /private/var/folders/... and this ->with($targetTmp, 'r') expectation won't match. CI runs ubuntu-latest, so it passes there and fails only for developers on Macs.

Switching the extractor to getPathname() (see the comment on MultipartRequestDataExtractor.php) fixes this test as a side effect. Otherwise expect realpath($targetTmp) here.

Fixed via the getPathname() switch in the extractor.

aivus · 2026-07-27T13:59:49Z

+        $this->assertSame($targetAttributes['targetAttributeKey'], $result['targetAttributeKey']);
+        $this->assertInstanceOf(UploadedFileStream::class, $result[$targetFileFieldKey]);
+
+        unlink($targetTmp);


If any assertion above fails, this unlink() never runs and the temp file leaks. Move cleanup to tearDown():

private ?string $tmpFile = null; protected function tearDown(): void { if (null !== $this->tmpFile && file_exists($this->tmpFile)) { unlink($this->tmpFile); } }

Cleanup moved to tearDown(); temp files are created via a helper that tracks them in an array, so multi-file tests are covered too.

aivus · 2026-07-27T14:00:08Z

+        $this->expectException(LogicException::class);
+        $extractor->extract($request, $this->endpoint);
+    }
+}


Coverage gaps in this file, in rough priority order:

wrapFiles() recursion is untested — no case with nested files[] arrays (['docs' => [$file1, $file2]]). That branch is the only real logic in the class and nothing exercises it.

No invalid-upload case — an UploadedFile constructed with a non-existent path / an error code, covering the getRealPath() === false path discussed on the extractor.

No case for a null entry in $request->files (unfilled optional file input), which currently disappears from the payload.

Also testExtractThrowsWhenStreamFactoryMissing and testExtractRequiresFactoryEvenWhenNoFilesPresent assert essentially the same thing; the second only adds text fields. If the null check moves into wrapFiles() as suggested, the second should invert into "does not throw when no files are present".

All added: nested files[] recursion (testExtractWrapsNestedFileArrays), invalid upload → 400 (testExtractThrowsOnInvalidUpload), unfilled optional input omitted (testExtractOmitsUnfilledOptionalFileInput), plus method/content-type guard tests.

The duplicate pair was restructured as you suggested: factory-missing now requires an actual file, and the no-files case asserts it does not throw.

aivus · 2026-07-27T14:00:24Z

Nice cleanup — the Prophecy → PHPUnit mock migration and the getCut() helper read well, and testResolvePicksFirstSupportingExtractor covers the new dispatch properly.

Two things:

Missing newline at end of file (\ No newline at end of file in the diff) — PSR-12.

The removed getDataForTestSupports() provider had no consuming test method, so dropping it is correct — just confirming that was intentional and not an accidentally deleted test.

One coverage note: testResolveDecodeException is gone, and nothing replaces it. Decode failures are now raised inside DefaultRequestDataExtractor, but DefaultRequestDataExtractorTest doesn't cover the throwing decoder either — so "malformed body → BadRequestHttpException", which used to be tested, is currently untested anywhere. Worth adding back at one level or the other.

Newline fixed; dropping the orphaned provider was intentional.

The decode-failure coverage is restored at both levels: testResolveDecodeException (extractor throws a serializer exception → BadRequestHttpException) and DefaultRequestDataExtractorTest::testExtractPropagatesDecodeException.

aivus · 2026-07-27T14:00:47Z

+    "psr/http-message": "^1.1|^2.0",
+    "psr/http-factory": "^1.0",


Separate from the fork-pin blocker you already called out:

psr/http-factory in require is defensible — MultipartRequestDataExtractor type-hints StreamFactoryInterface directly. psr/http-message is only used by StreamInterface in the test, and it's already a transitive dependency of psr/http-factory, so it's redundant here; require-dev would be more accurate.

Also worth adding, given the @? optional wiring:

"suggest": { "nyholm/psr7": "For multipart/form-data endpoints (provides a PSR-17 stream factory)" }

psr/http-message moved to require-dev, suggest entry added.

The fork pin stays until the components-bundle PR is merged and tagged, then it flips back to the upstream constraint before this merges.

- Reject invalid uploads with 400 instead of TypeError; use getPathname() instead of getRealPath() (also fixes macOS test failures) - Reject non-POST methods and non-multipart content types with 400 - Check the stream factory lazily when a file is actually wrapped - Fail container compilation when a controller-served multipart endpoint is registered but no PSR-17 stream factory is available - Autoconfigure RequestDataExtractorInterface implementations with the request data extractor tag - Decode "0" request bodies in DefaultRequestDataExtractor - Move psr/http-message to require-dev, suggest nyholm/psr7 - Document multipart endpoints, custom extractors and the constructor BC break in README - Extend test coverage: nested file arrays, invalid uploads, unfilled optional inputs, method/content-type guards, decode failures, compiler pass; clean up temp files in tearDown()

- DefaultRequestDataExtractor: reject scalar/null decoded bodies with a Serializer UnexpectedValueException so they resolve to 400 instead of an uncaught TypeError (500) - MultipartRequestDataExtractor: check the real wire method so method overrides on a wire POST are accepted; match the Content-Type mime case-insensitively and also accept application/x-www-form-urlencoded; reject non-empty form bodies PHP failed to parse (post_max_size) - MultipartStreamFactoryCompilerPass: read endpoints already baked into the registry definition by EndpointProviderCompilerPass instead of instantiating every tagged provider a second time; run the pass at negative priority to guarantee ordering; document the compile-time coverage boundary in the class docblock and README - Tests: cover the new behaviors, rename CUT variables to $target, break multi-call chains one call per line, name the fopen mode literal

Add multipart/form-data support via request data extractors

015b69f

aivus reviewed Jul 28, 2026

View reviewed changes

aviator-ua added 2 commits July 29, 2026 18:58

aviator-ua mentioned this pull request Jul 31, 2026

Add phpcs and phpstan tooling from template-php #22

Open

		$request->request->all(),
		$this->wrapFiles($request->files->all()),

Uh oh!

Conversation

aviator-ua commented Jul 23, 2026

Summary

What changed

Dependencies

⚠️ Blocker before merge

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants