Skip to content

feat: Support file uploads in feature tests (#7987) - #10574

Open
wakqasahmed wants to merge 2 commits into
codeigniter4:4.8from
wakqasahmed:feature/issue-7987-test-file-uploads
Open

wakqasahmed wants to merge 2 commits into
codeigniter4:4.8from
wakqasahmed:feature/issue-7987-test-file-uploads

Conversation

@wakqasahmed

Copy link
Copy Markdown
Contributor

Description

I added withFiles() so a feature test can attach MockUploadedFile objects to its next request. The request exposes them through getFile() and file validation, sets a multipart content type, and clears the files before the following request. The mock uses a local file for move() and store() while normal uploads still use PHP's move_uploaded_file().

The route-level test covers file access, uploaded validation, form fields, the content type, and moving the file. A second request confirms that the attached file does not carry over. The existing upload move tests also pass.

Fixes #7987.

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage (focused tests pass; coverage was not measured locally)
  • User guide updated
  • Conforms to style guide

@carson-codeigniter4 carson-codeigniter4 Bot added the 4.8 PRs that target the `4.8` branch. label Sep 19, 2026

@wakqasahmed wakqasahmed left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two behavior issues found, reproduced with isolated PHP 8.4.25 tests: a preceding JSON request overrides the upload request's multipart header, and omitting the mock's optional MIME type causes getClientMimeType() to throw. Inline comments include reproduction and expected behavior.

The production upload path still uses move_uploaded_file(); no high-confidence security vulnerability identified. Review covered file collection injection, validity/move semantics, request state, compatibility, documentation, and focused test coverage. Full CI and coverage remain CI responsibilities.

$request = $this->setupHeaders($request);
if ($this->uploadedFiles !== []) {
$this->setPrivateProperty($request, 'files', new MockFileCollection($this->uploadedFiles));
$request->setHeader('Content-Type', 'multipart/form-data');

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Preserve multipart behavior after request-body formatting

A feature test that first calls withBodyFormat('json')->post(...), then withFiles(['document' => $file])->post(...), sends the second request with Content-Type: application/json even though getFile('document') is populated. I reproduced this in a route-level test: expected multipart/form-data:present, actual application/json:present. bodyFormat persists, and the later setRequestBody() call overwrites this header and JSON-encodes the form fields. Controllers or filters expecting multipart therefore reject an otherwise valid upload test.

Make attached files select multipart consistently during body setup, and add a regression for JSON request -> upload request -> ordinary request. The upload's files and multipart override should apply only to that upload request, without leaving upload state on the following request.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e10a3a7. Body formatting now skips requests with uploaded files, preserving multipart for that request while the following request still uses the persistent JSON format. The new route-level regression covers JSON → upload → ordinary requests.

{
public function __construct(
string $path,
string $originalName,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P2] Give omitted mock MIME metadata a string value

The new constructor permits new MockUploadedFile($path, 'document.txt'), but this leaves originalMimeType as null. Calling the inherited getClientMimeType(): string then throws TypeError: Return value must be of type string, null returned; I reproduced this with an existing local fixture that passes isValid(). A normal PHP upload supplies a string for this metadata, so tests using the documented optional constructor arguments cannot exercise otherwise valid code that reads the client MIME type.

Normalize an omitted MIME type to a string in the mock, while preserving explicitly supplied client MIME values, and cover construction without the third argument. The result should be usable through getClientMimeType() without an exception.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e10a3a7. The mock normalizes omitted client MIME metadata to an empty string and preserves an explicit MIME value. The regression covers both cases and calls getClientMimeType().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4.8 PRs that target the `4.8` branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant