Accept file URLs for support ticket attachments - #1623
Open
jkmassel wants to merge 1 commit into
Open
Conversation
Attachments cross the bindings as filesystem paths and the request executor opens each one directly, so a caller holding a file URL has to convert it. URL.path() percent-encodes by default, which produces a path that doesn't exist on disk and fails the request with MediaFileNotFound — a filename with a space is enough, and non-ASCII names are escaped too. Adds attachmentURLs: [URL] initializers to CreateSupportTicketParams and AddMessageToSupportConversationParams so the wrapper owns the conversion. The existing attachments: [String] initializers are unchanged. WordPress-iOS hit this on the media path and then again on support attachments; moving the conversion here means each consumer doesn't get its own chance to be wrong.
Open
3 tasks
Collaborator
XCFramework BuildThis PR's XCFramework is available for testing. Add to your .package(url: "https://github.com/automattic/wordpress-rs", branch: "pr-build/1623")Built from c75ae8b |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds
attachmentURLs: [URL]initializers toCreateSupportTicketParamsandAddMessageToSupportConversationParams, so the Swift wrapper owns the URL → filesystem path conversion instead of leaving it to each caller.Summary
SafeRequestExecutoropens each one directly.URLhas to convert it, andURL.path()— the obvious choice — percent-encodes by default. That hands the executor a path that doesn't exist on disk, and the request fails withMediaFileNotFound.Why this belongs in the library
The conversion isn't a caller preference — there is exactly one correct answer, and the library is the only place that knows the string is about to be
opened rather than put in a URL. Leaving it to callers means every consumer gets its own chance to be wrong, and the failure is invisible until a user picks a file whose name happens to need encoding.WordPress-iOS hit this in production on the media path (WordPress-iOS#26005) and then again on support attachments (WordPress-iOS#26008) — same defect, two call sites, found only by grepping for the shape after the first one. That's the argument for moving it down here.
The attachment filename is user data, not app-generated. On iOS it's whatever PhotosUI exports, which preserves the asset's original name — and macOS names screenshots
Screen Shot 2026-09-08 at 10.31.15.png.Changes
native/swift/Sources/wordpress-api/WPComExtensions.swift: two convenience initializers taking
attachmentURLs: [URL], mirroring the generated memberwise parameter lists so no field is lost.attachmentURLsdeliberately has no default value. Giving it one would make a call passing no attachments match both this initializer and the generated one.The existing
attachments: [String]initializers are unchanged — a caller that already has paths keeps working, and this is additive.Test plan
SupportTicketAttachmentTests— seven cases across both params types: a filename with a space, one with a%, one non-ASCII, controls needing no encoding, an empty attachment list, and a pass-through check that every other field survives the convenience initializer.SupportTicketsCompatTestsso the api-compatibility suite pins the new signatures.swift test --filter SupportTicketAttachmentTests— 7/7 pass.make lint-swiftclean.Notes
Two things I did not do here, both worth a second opinion:
MediaCreateParamshas the same shapeIt's the third
RequiresMultipartFormtype and the one that caused the original production bug. It's awp_apitype rather than a WP.com one, so it'd land in a different file, and WordPress-iOS#26005 is currently fixing it caller-side. Happy to add it here instead if you'd rather have all of them consistent. (ReplyToUnifiedConversationParamsis the fourth; no consumer in WordPress-iOS today.)The parameter lists can drift
A convenience initializer that forwards to a generated memberwise initializer has to be updated whenever the Rust record gains a field, and nothing catches it — the new field just isn't reachable through the URL-based initializer. There's a comment saying so, but if you'd rather avoid the maintenance entirely, the drift-free alternative is a named conversion on
URLthat every params type can use:That covers all four types at once and never needs syncing, but it doesn't stop a caller from reaching for
path()anyway. I went with the initializers because preventing the mistake seemed worth more than the upkeep — say the word if you disagree.