Refactor uploadFile

[eshimischi]

Add correct filename and extension

Summary by CodeRabbit

• Bug Fixes

  • Multipart uploads no longer inject unintended keys into file/field parts, fixing malformed multipart payloads.

• New Features / UX

  • File upload now requires an explicit filename up-front and always includes both the provided filename and a derived file name for uploaded files.
  • Uploads now require a channel identifier to be supplied.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Warning

Rate limit exceeded

@eshimischi has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 49 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between dfd66ce and 825f718.

📒 Files selected for processing (2)

• src/Client/Endpoint/FilesEndpoint.php
• test/Client/Endpoint/FilesEndpointTest.php

📝 Walkthrough

Walkthrough

Removed automatic injection of a 'name' key into array-valued multipart fields containing 'contents'. Updated FilesEndpoint::uploadFile signature to require filename as the first parameter and make channel_id non-nullable; file multipart parts now include name (derived from path) and filename (from the new parameter).

Changes

Cohort / File(s)	Summary
Multipart Stream Generation src/Client/MultipartTrait.php	createMultipartStream no longer merges ['name' => $name] into array-valued fields that include a contents key; such values are appended unchanged. (1 line modified)
File Upload Endpoint src/Client/Endpoint/FilesEndpoint.php	uploadFile signature changed to require string $filename first and string $channel_id (non-nullable). Multipart construction for file parts now sets name (from pathinfo(..., PATHINFO_FILENAME)) and filename (from the new first parameter) inside the files element; query uses the new filename source. (signature and multipart construction updated)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• feat: add multipart/form-data support for file uploads #297: Edits multipart upload handling and touches the same MultipartTrait/createMultipartStream logic.

Poem

I nibble bytes and stitch the seam,
No extra names within the stream,
Files now ask their name up front,
I hop through code with careful stomp,
A rabbit cheer for tidy I/O. 🐇✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Refactor uploadFile' accurately describes the main change—a refactoring of the uploadFile method to ensure correct filename and extension handling.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @src/Client/MultipartTrait.php:
- Around line 29-30: The change removed automatic injection of the 'name' key
for array-valued multipart elements causing GuzzleHttp MultipartStream to fail;
restore backward compatibility in MultipartTrait by ensuring any array $value
with 'contents' gets a 'name' key when missing (i.e., before pushing to
$elements, if is_array($value) && isset($value['contents']) and
!isset($value['name']) then set $value['name'] to the appropriate field name),
so callers that only provide 'contents'/'filename' continue to work;
alternatively update all endpoints that build multipart arrays (LDAPEndpoint,
UsersEndpoint, SAMLEndpoint, SystemEndpoint, TeamsEndpoint, PluginsEndpoint,
EmojiEndpoint) to include 'name' at construction time, but prefer restoring the
conditional injection in MultipartTrait for immediate compatibility.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 40705a0 and cca8f8e.

📒 Files selected for processing (1)

• src/Client/MultipartTrait.php

🧰 Additional context used

📓 Path-based instructions (2)

**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.php: Run static analysis with vendor/bin/phpstan analyse
Use Rector for code quality fixes with composer rector:dry-run to preview and composer rector to apply fixes

Files:

• src/Client/MultipartTrait.php

src/Client/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Regenerate API client from OpenAPI spec using php bin/api-generate after spec changes

Files:

• src/Client/MultipartTrait.php

[eshimischi]

In order to upload correct filename with proper extension into Mattermost

[coderabbitai]

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Client/Endpoint/FilesEndpoint.php (1)

66-67: Remove redundant query parameters; use multipart/form-data approach exclusively.

The code adds channel_id and filename as query parameters (lines 66-67), then also includes channel_id in the multipart form data (line 82) and sets a multipart Content-Type header (line 89). The Mattermost API v4 file upload endpoint supports two mutually exclusive approaches: either multipart/form-data with parameters in the form data, or raw body with parameters in the query string. This implementation mixes both, which is not a valid request pattern.

Remove lines 66-67 to use only the multipart/form-data approach, which is already correctly implemented in lines 75-90 and is the recommended pattern for backward compatibility.

🤖 Fix all issues with AI agents

In @src/Client/Endpoint/FilesEndpoint.php:
- Line 78: pathinfo() is being called on $files which per the docblock can be a
string, resource, or Psr\Http\Message\StreamInterface; to fix, guard the call by
checking the type of $files and only call pathinfo($files, PATHINFO_FILENAME)
when is_string($files), otherwise fall back to using the provided $filename
parameter (or require/validate a separate form name) and if neither is available
throw/return a clear error; update the logic around $files and $filename where
$file_name is assigned so resources/streams use $filename (or a validated
alternative) and StreamInterface cases are handled explicitly.
- Line 79: The multipart form entry in FilesEndpoint.php sets the form field
"name" incorrectly to the filename variable; update the $multipartFields
assignment for 'files' in the send/upload routine so the 'name' key is the
static form field identifier "files" (i.e. set 'name' => 'files' instead of
using $file_name) and keep 'contents' => $files and 'filename' => $filename to
match other endpoints and produce Content-Disposition: form-data; name="files".

🧹 Nitpick comments (1)

src/Client/Endpoint/FilesEndpoint.php (1)

81-83: Remove redundant null check.

Since $channel_id is now a non-nullable string parameter (line 59), it cannot be null. This check is redundant and should be removed.

♻️ Suggested cleanup

-if ($channel_id !== null) {
-    $multipartFields['channel_id'] = $channel_id;
-}
+$multipartFields['channel_id'] = $channel_id;

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cca8f8e and 5ee35a0.

📒 Files selected for processing (1)

• src/Client/Endpoint/FilesEndpoint.php

🧰 Additional context used

📓 Path-based instructions (4)

**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.php: Run static analysis with vendor/bin/phpstan analyse
Use Rector for code quality fixes with composer rector:dry-run to preview and composer rector to apply fixes

Files:

• src/Client/Endpoint/FilesEndpoint.php

src/Client/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Regenerate API client from OpenAPI spec using php bin/api-generate after spec changes

Files:

• src/Client/Endpoint/FilesEndpoint.php

src/Client/{Endpoint,Model}/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/Client/{Endpoint,Model}/**/*.php: Do not manually edit generated files in src/Client/Endpoint/ and src/Client/Model/
Ensure generated code in src/Client/Endpoint/ and src/Client/Model/ is included in Rector validation for consistent code quality

Files:

• src/Client/Endpoint/FilesEndpoint.php

src/Client/Endpoint/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Implement endpoint groups in src/Client/Endpoint/ with methods for specific API operations

Files:

• src/Client/Endpoint/FilesEndpoint.php

🧬 Code graph analysis (1)

src/Client/Endpoint/FilesEndpoint.php (1)

src/Client.php (1)

• files (156-159)

🔇 Additional comments (1)

src/Client/Endpoint/FilesEndpoint.php (1)

5-8: This file is already generated by php bin/api-generate (commit 5ee35a0). No manual edits were made to this file that would be lost on regeneration. If there are code quality concerns with the current implementation, they should be addressed by updating the OpenAPI specification and regenerating, or by fixing the generation logic itself, not by treating this as a manually-edited file.

Likely an incorrect or invalid review comment.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In @src/Client/Endpoint/FilesEndpoint.php:
- Around line 77-80: The code currently calls pathinfo($files,
PATHINFO_FILENAME) which is invalid when $files is a resource, StreamInterface,
or raw binary; replace deriving name from $files with using the provided
$filename (or validate $files is a path string before calling pathinfo). Update
the block that sets $multipartFields['files'] (the code referencing $files,
pathinfo and $filename in FilesEndpoint.php) to use $filename (or a sanitized
fallback) for the 'name' field and avoid calling pathinfo on non-string inputs;
add a simple type check (is_string) if you still want to support path inputs and
otherwise skip pathinfo.

🧹 Nitpick comments (1)

src/Client/Endpoint/FilesEndpoint.php (1)

81-83: Redundant null check — $channel_id is non-nullable.

Since $channel_id is now typed as string (non-nullable), the if ($channel_id !== null) check is always true. This can be simplified.

♻️ Suggested simplification

-        if ($channel_id !== null) {
-            $multipartFields['channel_id'] = $channel_id;
-        }
+        $multipartFields['channel_id'] = $channel_id;

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ee35a0 and dfd66ce.

📒 Files selected for processing (1)

• src/Client/Endpoint/FilesEndpoint.php

🧰 Additional context used

📓 Path-based instructions (4)

**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.php: Run static analysis with vendor/bin/phpstan analyse
Use Rector for code quality fixes with composer rector:dry-run to preview and composer rector to apply fixes

Files:

• src/Client/Endpoint/FilesEndpoint.php

src/Client/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Regenerate API client from OpenAPI spec using php bin/api-generate after spec changes

Files:

• src/Client/Endpoint/FilesEndpoint.php

src/Client/{Endpoint,Model}/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

src/Client/{Endpoint,Model}/**/*.php: Do not manually edit generated files in src/Client/Endpoint/ and src/Client/Model/
Ensure generated code in src/Client/Endpoint/ and src/Client/Model/ is included in Rector validation for consistent code quality

Files:

• src/Client/Endpoint/FilesEndpoint.php

src/Client/Endpoint/**/*.php

📄 CodeRabbit inference engine (CLAUDE.md)

Implement endpoint groups in src/Client/Endpoint/ with methods for specific API operations

Files:

• src/Client/Endpoint/FilesEndpoint.php

[eshimischi]

@cedricziel not sure about a proper way to change openapi.json, would you please help with it