Skip to content

feat!: remove MediaUploadDelegate.uploadFile - #629

Open
jkmassel wants to merge 2 commits into
feat/media-uploader-protocolfrom
feat/remove-upload-file-hook
Open

feat!: remove MediaUploadDelegate.uploadFile#629
jkmassel wants to merge 2 commits into
feat/media-uploader-protocolfrom
feat/remove-upload-file-hook

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Stacked on #628. Sixth of ten PRs splitting #621. Almost entirely deletions.

What?

Removes uploadFile from MediaUploadDelegate on both platforms. MediaUploader (#628) replaces it.

Why?

Returning a raw response split one upload's HTTP across two owners — the host performed the POST, the editor drove the post-process retries and orphan cleanup behind it — and the hook received no form fields, so an attachment it uploaded landed unattached to its post. Neither is fixable while the hook returns a raw response.

What is left is a clean division: a delegate transforms bytes and GutenbergKit owns delivery and its retries; a MediaUploader owns delivery and its retries entirely. There is no longer an in-between where the host performs the upload but the editor retries it.

How?

  • The protocol requirement, its default implementation, and the delivery branch that called it, on both platforms.
  • handlesFile no longer gates the temp copy for two callers, only for processFile. With a MediaUploader set it can't decline the upload itself — an uploader delivers every file, so there is no passthrough to fall to — but it still gates processFile, so a declined file reaches the uploader unprocessed. Pinned by an uploader sees a file the delegate's metadata gate would have declined on both platforms.
  • MediaUploadResponse drops to internal on both platforms: uploadFile was the only public API that named it. This is load-bearing for migration safety — Swift requires an explicit return type, so a host's stale uploadFile cannot compile without naming the type. Leave it public and the same removal breaks hosts silently instead.
  • The deprecation warning feat: add MediaUploader, for a host that owns the whole upload #628 left in GutenbergKit's own build goes away with the hook.

A second commit corrects the docs the removal left stale: the mediaUploadDelegate property summaries — what Xcode Quick Help and IDE hover show — still advertised "custom upload" on both platforms and pointed nowhere at MediaUploader, and UploadError.noUploader's message still named the delegate as an alternative to the internal client.

Testing Instructions

Tests for the removed path are rewritten rather than deleted — the delegate now processes and the internal client delivers, which is what the remaining path does.

  • swift test — 974 tests in 57 suites, green
  • xcodebuild test -scheme GutenbergKit-Package -sdk iphonesimulator — 585 tests in 36 suites, green. Covers the UIKit-gated code the host swift test compiles as empty, including EditorViewController.
  • :Gutenberg:testDebugUnitTest — 672 tests, 0 failures
  • :app:assembleDebug — Android demo builds
  • swift build --build-tests — zero warnings
  • SwiftLint and Detekt clean

Breaking change

Hosts implementing uploadFile must conform to MediaUploader instead. Hosts that only implement processFile / handlesFile are unaffected. Migration needed in WordPress-iOS, WordPress-Android and Jetpack.

The break is a hard compile error on both platforms, not a silent redirect — Android on override of nothing, iOS on MediaUploadResponse no longer being visible. uploadFile shipped in v0.20.0-alpha.0 and no stable tag.

@jkmassel jkmassel added [Type] Breaking Change For PRs that introduce a change that will break existing functionality Android iOS labels Sep 5, 2026
@jkmassel jkmassel self-assigned this Sep 5, 2026
@wpmobilebot

wpmobilebot commented Sep 5, 2026

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/629")

Built from d8b85e2

@jkmassel
jkmassel force-pushed the feat/remove-upload-file-hook branch from f5cbf24 to 1eb400b Compare September 8, 2026 16:12
`MediaUploader` replaces it. Returning a raw response split one upload's
HTTP across two owners — the host performed the `POST`, the editor drove
the `post-process` retries and orphan cleanup behind it — and the hook
received no form fields, so an attachment it uploaded landed unattached to
its post. Neither is fixable while the hook returns a raw response, which
is what the replacement changes.

What is left is a clean division: a delegate transforms bytes and
GutenbergKit owns delivery and its retries; a `MediaUploader` owns
delivery and its retries entirely. There is no longer an in-between where
the host performs the upload but the editor retries it.

`handlesFile` no longer gates the temp copy for two callers, only for
`processFile` — and only when no uploader is set, since an uploader takes
over delivery for every file.

`MediaUploadResponse` drops to internal on both platforms: `uploadFile`
was the only public API that named it.

BREAKING CHANGE: hosts implementing `uploadFile` must conform to
`MediaUploader` instead. Hosts that only implement `processFile` /
`handlesFile` are unaffected.
@jkmassel
jkmassel force-pushed the feat/remove-upload-file-hook branch from 1eb400b to 6bcc210 Compare September 9, 2026 00:49
…d docs

Review follow-ups to 6bcc210. No behavior change.

`handlesFile`'s new doc said it is "only consulted when no `MediaUploader` is
set". It is always consulted (`MediaUploadServer.swift:143`, `.kt:374`), and it
still gates `processFile` (`.swift:306`, `.kt:534`) — a declined file reaches
the uploader unprocessed. The implementation comment 160 lines away and the
`an uploader sees a file the delegate's metadata gate would have declined` test
on both platforms already said so. Replaced with wording lifted from that
comment.

Removing `uploadFile` also left the docs a host actually reads still
advertising it:

- The `mediaUploadDelegate` property summaries — what Xcode Quick Help and IDE
  hover show — said "customizing media file processing and upload behavior"
  (iOS) and "(resize, transcode, custom upload)" (Android). Both now describe
  transformation and point at `mediaUploader` for the upload case.
- `MediaUploadResponse.statusCode` claimed the status could come from "the
  host's upload service". `MediaUploader.upload` returns `Data`, so the host
  path supplies a literal 201.
- `MediaUploadServer`'s parameter docs, the `UploadResult.uploaded` doc, and
  Android's "won't process or upload" comment, whose iOS twin already read
  "won't process".

Two non-doc changes ride along:

- `UploadError.noUploader`'s message named a role the delegate no longer has:
  "No upload delegate or internal media client configured" becomes "No media
  uploader or ...". It reaches the editor in a 500 body; nothing asserts on it.
- iOS's `MockUploadDelegate` became a duplicate of `ProcessOnlyDelegate` once
  `uploadFile` went. Android already consolidated on `ProcessOnlyDelegate`;
  iOS now matches.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jkmassel
jkmassel marked this pull request as ready for review September 9, 2026 16:41
@jkmassel
jkmassel requested a review from dcalhoun September 9, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android iOS [Type] Breaking Change For PRs that introduce a change that will break existing functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants