Skip to content

Split the media delegate into MediaProcessor and MediaUploader - #621

Draft
jkmassel wants to merge 32 commits into
fix/register-core-media-upload-middlewarefrom
refactor/media-processor-uploader
Draft

jkmassel wants to merge 32 commits into
fix/register-core-media-upload-middlewarefrom
refactor/media-processor-uploader

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Stacked on #594. Makes performing a media upload — and retrying it — a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does (say, to run it through its own networking so it can log every request). Both go to the same configured site; the only difference is who executes the requests. There's no in-between where the host performs the upload but GutenbergKit retries it.

Summary

Two protocols replace the single MediaUploadDelegate:

  • MediaProcessor — transform the file before upload; GutenbergKit performs the upload and owns its retries. The common, safe extension point.
  • MediaUploader — perform the upload yourself (your own networking, logging, retry policy, a background session) and own its whole lifecycle: retries, recovery, and cleanup. Receives a MediaUpload value carrying the file, its metadata, the editor's non-file form fields (post, additionalData), and the request query (?_embed).

EditorViewController / GutenbergView own whichever handler you set — their properties are strong on both platforms, so you can assign one and drop your own reference (just don't strongly retain the editor back).

Breaking: hosts migrate mediaUploadDelegatemediaProcessor / mediaUploader.

The problem with the old design

When an upload fatals in server-side post-processing, it has to be retried: core retries POST …/post-process up to 5×, and cleans up the orphan if that fails.

MediaUploadDelegate.uploadFile let a host perform the upload itself by returning the raw response it received. But that split one upload's HTTP across two owners: the host performed the POST /wp/v2/media, then core — reading that raw response — drove the post-process retries (and the orphan cleanup) behind it. A host that took over uploads to run them through its own stack still didn't own the retries; those went out through the browser, not the host. Delivery and its retries were owned by different parties.

The fix

Make the upload and its retries one unit with one owner:

  • MediaProcessor (handlesFile, processFile) only transforms the file. It never performs the upload, so GutenbergKit performs it and owns the retries. The extension point almost every host wants.
  • MediaUploader (upload(_:)) performs the upload on the host's own stack and owns the whole lifecycle. upload returns the finished attachment or throws — there's no raw response for core to retry behind it, so the host drives its own post-process recovery and force-deletes its own orphan on terminal failure. The MediaUpload it receives carries everything needed to reproduce a native request — the file, the editor's post / additionalData fields, and the ?_embed query — so a host upload attaches to its post instead of landing as an unattached orphan.

So it's all-or-nothing: the host performs the upload and its retries, or GutenbergKit does — never a split. An uploader and GutenbergKit's built-in default both target the same configured site; the choice is only who executes the requests.

Media deletes always relay to the default uploader (the configured site) — every attachment lives there, even one a host uploader delivered, so there's no per-host delete path. The server starts if either handler is set and builds a default uploader whenever site credentials are present (it delivers GutenbergKit's own uploads and relays every delete). Because deletes need it, a mediaUploader set without site credentials is a configuration error and traps at startup rather than starting a server whose every delete would 500. MediaUploadResponse is now internal — it's no longer on any public API.

Accepted Risk / Out of Scope

  • The configured-site delete relay isn't scoped. GutenbergKit relays core's cleanup DELETE to the configured site, but the relay can't distinguish it from any other DELETE the WebView sends — a client-side-compromised editor (a supply-chain-tampered JS bundle, or editor XSS) holding the loopback token could force-delete arbitrary media there. We accept this: such a script already has broad write access via allowed methods, and a server-side compromise (a malicious plugin) deletes media directly without the editor. An earlier revision carried a per-session ledger to scope the relay; it's dropped as not worth the cost for a client-side-only threat.

Test Plan

  • iOS host suite green
  • Android unit suites green — MediaUploadServer, GutenbergView
  • SwiftLint + Detekt clean
  • iOS demo app builds (xcodebuild, Xcode 26.4.1); Android demo builds (Detekt compiles it)
  • Migrate mediaUploadDelegate in WordPress-iOS / WordPress-Android / Jetpack

Related

dcalhoun and others added 20 commits August 20, 2026 15:26
The local `mediaUploadMiddleware` shadowed the same-named export from
`@wordpress/api-fetch`, so the file read as though core's post-process
retry behavior was registered when only the draft post ID stripping was.

Rename it to `stripDraftPostIdMiddleware` to describe what it does. No
behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
When `wp_generate_attachment_metadata()` fails server-side (commonly a PHP
memory_limit or max_execution_time fatal on large images), WordPress
returns a 5xx carrying an `x-wp-upload-attachment-id` header. Core's
`mediaUploadMiddleware` recovers from this by retrying
`POST /wp/v2/media/<id>/post-process` up to five times, then deleting the
orphaned attachment if every attempt fails.

That middleware was never registered, so these uploads surfaced as failures
that left an orphaned gray attachment behind and duplicated the attachment
on retry.

`apiFetch.use` unshifts, so registration order is the reverse of execution
order. Core's middleware is registered before the native one so that it
runs after it, and below auth, namespacing, and the root URL so the
`post-process` requests it issues through `next` stay authenticated and
correctly addressed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`new RegExp('(' + [].join('|') + ')')` is `/()/`, which matches every
string, so `alreadyHasSiteNamespace` was unconditionally true whenever a
site configured no namespace — as self-hosted sites do.

That was load-bearing rather than merely benign: it suppressed a rewrite
that would otherwise interpolate `siteApiNamespace[0]` — `undefined` for an
empty namespace — into every path, producing `/wp/v2/undefinedposts`. Gate
the rewrite on a configured namespace so the guard no longer has to, and
escape the namespaces so each is matched literally.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
The native upload path circumvented core's post-process retry entirely, so
a metadata fatal on a delegate-handled upload stayed a permanent failure
with an orphaned attachment left behind. Two things blocked it:

- `relayResponse` rebuilt the response with a hardcoded Content-Type,
  dropping `x-wp-upload-attachment-id` — the header core's middleware needs
  to identify the attachment to retry. Relay it (via an allowlist, since
  the body is re-sent with a recomputed length) and expose it through CORS,
  without which the WebView cannot read it cross-origin regardless.
- `nativeMediaUploadMiddleware` always parsed the body, so it never yielded
  the `Response` that core's middleware inspects. Honor `parse: false` by
  resolving or rejecting with the `Response` itself.

`MediaUploadResponse` gains a `headers` property on both platforms,
defaulted so existing host callers are unaffected.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
The retry depends on reading `x-wp-upload-attachment-id`, which is only
possible same-origin or where the server exposes it via CORS. Record which
combinations recover, and why there is no client-side fallback, so the iOS
direct-upload case is not mistaken for a bug in this registration.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
Reproduces the server-side image processing fatal the post-process retry
recovers from, so the middleware can be exercised locally without a large
image or a resource-starved host.

Adapted from the approach in WordPress/gutenberg#17858, with the random
failure rate replaced by an explicit mode (`recover`/`always`/`off`) so both
the recovery and the exhaust-and-delete paths are reproducible. The mode is
an option rather than per-request state, since the upload and each retry are
separate requests and a native-server upload carries no browser cookie.

The plugin sets the 500 itself: a real fatal under FPM surfaces as a 500,
but the Playground runtime returns 200, which the retry would ignore.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
Reading or flipping the mode meant a curl invocation with an inline
credentials lookup, which is easy to get wrong mid-debug and silently
no-ops if it fails — leaving a passing upload that looks like the retry
never fired.

Wrap it in `make wp-env-media-failure [MODE=off|recover|always]`, matching
the existing VAR=value convention and the thin-target-plus-bin-script
pattern. Each precondition reports the fix that applies to it: missing
credentials, a rejected 401 (stale after a Playground restart), an
unreachable server, and an unregistered endpoint.

Also document the orphaned-server and 401 cases in troubleshooting; both
came up repeatedly while testing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
A bundled Android build serves the editor from the site's host without a
port (`http://10.0.2.2`), which was not in the allowlist, so its REST
requests were rejected before reaching WordPress.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
Testing a bundled Android build against wp-env disproved the claim that
Android direct uploads are same-origin and therefore recover.
`GutenbergView` derives the asset domain from the site's host, and `host`
drops the port — so the editor at `http://10.0.2.2` is cross-origin with a
site at `http://10.0.2.2:8888`, and the attachment ID header stays
unreadable.

The claim holds only when the site runs on the scheme's default port, as
production sites do. Say that, rather than implying every Android site
recovers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`always` mode fatalled on every sub-size pass, and core's `force=true`
delete path runs sub-size handling too — so the editor's orphan cleanup
fatalled as well, leaving the orphan behind. The retry logic was correct;
the simulator refused the cleanup it had correctly requested.

Exempt deletes, including the `POST` + `X-Http-Method-Override: DELETE`
form api-fetch sends, so the bare request method alone is not enough to
identify one.

Also document that a simulated fatal aborts the request before WordPress
adds CORS headers, so these responses surface as CORS errors rather than
readable 500s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
When every post-process retry fails, core's middleware deletes the orphaned
attachment. A cross-origin editor cannot make that request: api-fetch
tunnels DELETE as a POST carrying `X-HTTP-Method-Override`, and core's
`rest_allowed_cors_headers` omits that header, so the browser blocks it at
preflight and the orphan survives.

Route media deletions through the loopback upload server instead, which
sets its own CORS policy and already permits DELETE. The middleware
intercepts before api-fetch's `httpV1` adds the override header, so what
reaches the native server is a plain DELETE.

Both servers gain a single narrow route — `DELETE /media/<id>` with a
numeric ID — rather than a general proxy, matching the existing
`POST /upload`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`handleDelete` went straight to the default uploader, unlike `handleUpload`
which offers the work to the delegate first. A host whose `uploadFile`
uploads to its own media service holds an ID only it can resolve, so
deleting through the default uploader would address the wrong site.

Add `deleteFile(attachmentId:)` to `MediaUploadDelegate` on both platforms,
defaulted to nil so existing hosts are unaffected, and try it before falling
back. Rename the handler to `handleMediaDelete`, since it deletes an
attachment rather than an upload and no longer mirrors `handleUpload`'s
signature.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
Drop the two notes explaining how the simulator produces its 500 and why a
fatal response reads as a CORS error — implementation detail that belongs in
the plugin, not the guide. Drop the orphaned-server and stale-credential
troubleshooting entries; those are environment problems to address on their
own. Also drop a comment restating what the adjacent condition already says,
and reword the make target's help text.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`nativeMediaUploadMiddleware` mixed dispatch with the whole upload
implementation, so adding the deletion path left the two handled
asymmetrically — one extracted, one inline.

Extract `nativeMediaUpload` alongside `nativeMediaDelete`, both returning
null when a request is not theirs, leaving the middleware as a short
dispatcher. No behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ULMqNwTEWty4MeNrr94MuF
`handleMediaDelete` caught only `IOException`, so a delegate's `deleteFile`
throwing anything else — `IllegalStateException`, a JSON error — escaped to
`HttpServer.resolveResponse` and returned a plain-text 500. The editor's
`nativeMediaDelete` then failed on `response.json()` and reported
`invalid_json` rather than the delegate's actual failure.

Catch `Exception` and rethrow `CancellationException`, matching
`passthroughResponse` and iOS's untyped `catch`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr
`relayResponse` prepended `Content-Type: application/json` to an array of the
response's own headers, and `HTTPResponse` serializes every entry it is given.
A delegate returning its own `Content-Type` therefore put the header on the
wire twice, which URLSession surfaces as "application/json, text/plain".
Android's map merge already overrode instead, so the two platforms disagreed
on the same public API.

Skip the default when the response already carries the name, matching Android.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr
`deleteFile` is called for every deletion, including attachments the delegate
declined at upload time — an attachment ID carries no MIME type or filename,
so there is no `handlesFile` gate to apply. A delegate answering for one of
those leaves the real WordPress attachment undeleted, which is the orphan the
cleanup exists to remove.

Returning nil already falls through to the default uploader; document that as
the signal for an unrecognized ID.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr
The credentials path was interpolated into the `node -e` source as a
single-quoted JS string literal, so a checkout under a path containing a quote
or backslash produced a SyntaxError stack trace instead of the intended
"could not read authHeader" message.

Pass it through `process.argv` and single-quote the script body so the shell
does not expand it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EusJFgEHAvZNESsD3xHbvr
`relayResponse` merged the JSON default with the response's own headers via
Kotlin's map merge, which only overrides on an exact key match. A delegate
returning `content-type` therefore produced a two-entry map, and
`serializeResponse` writes every entry, putting the header on the wire twice —
the WebView sees "application/json, text/plain".

This is the same defect `0bf40ad3` fixed on iOS, which the map merge was
believed to already handle. Skip the default when the response carries the name
under any casing, matching iOS and the case-insensitive lookups `HttpServer`
already uses.

Add the Android counterpart to the iOS `delegateContentTypeWins` test. It
asserts on the raw header lines rather than the parsed map, which lowercases
keys into a map and would collapse the duplicate — hiding the very bug.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LVdVBubbCDp7mRXSWkcHHm
The native upload middleware runs below core's mediaUploadMiddleware, which
forwards every upload as `parse: false` and reads `x-wp-upload-attachment-id`
off a rejected Response to retry post-process. Logging the initial 5xx at
error level reported a failure before recovery ran, so every upload that
silently recovered still emitted an error. Reject without logging, matching
the nativeMediaDelete sibling — the initial 5xx is a handoff to core's retry,
not an outcome.
@jkmassel jkmassel added [Type] Breaking Change For PRs that introduce a change that will break existing functionality iOS Android labels Sep 3, 2026
@jkmassel jkmassel self-assigned this Sep 3, 2026
@wpmobilebot

wpmobilebot commented Sep 3, 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/621")

Built from 2bbc65d

@jkmassel
jkmassel force-pushed the refactor/media-processor-uploader branch 4 times, most recently from 9fe481e to 9f00177 Compare September 3, 2026 21:50
Make performing a media upload -- and retrying it -- a single, all-or-nothing
responsibility: either GutenbergKit performs the upload and owns its retries, or
the host does (say, to run it through its own networking so it can log every
request). Both go to the same configured site; the only difference is who executes
the requests. There is no in-between where the host performs the upload but
GutenbergKit retries it.

When an upload fatals in server-side post-processing it has to be retried: core
retries POST .../post-process up to 5x, and cleans up the orphan if that fails.
The old `MediaUploadDelegate.uploadFile` let a host perform the upload itself by
returning the raw response it received -- which split one upload's HTTP across two
owners: the host performed the POST /wp/v2/media, then core, reading that raw
response, drove the post-process retries (and the orphan cleanup) behind it. A
host that took over uploads to run them through its own stack still didn't own the
retries; those went out through the browser, not the host. Delivery and its
retries were owned by different parties.

Make the upload and its retries one unit with one owner:

- `MediaProcessor` (handlesFile, processFile) only transforms the file. It never
  performs the upload, so GutenbergKit performs it and owns the retries -- the
  extension point almost every host wants.
- `MediaUploader` (upload) performs the upload on the host's own stack (its
  networking, logging, retry policy, a background session) and owns the whole
  lifecycle. `upload` returns the finished attachment or throws: there's no raw
  response for core to retry behind it, so the host drives its own post-process
  recovery and force-deletes its own orphan on terminal failure.

All-or-nothing: the host performs the upload and its retries, or GutenbergKit does
-- never a split. An uploader and GutenbergKit's built-in default both target the
same configured site; the choice is only who executes the requests.

Media deletes always relay to the default uploader (the configured site): every
attachment lives there, even one a host uploader delivered, so there is no per-host
delete path. The relay is left unscoped -- core issues its cleanup DELETE there,
but the relay can't tell it from any other DELETE the WebView sends, so a
client-side-compromised editor holding the loopback token could force-delete media
on the site. Accepted -- such a script already has broad write access, and a
server-side compromise deletes media directly without the editor. An earlier
revision carried a per-session ledger to scope the relay; dropped as not worth the
cost for a client-side-only threat.

`EditorViewController`/`GutenbergView` expose `mediaProcessor` + `mediaUploader`
in place of `mediaUploadDelegate`; the server starts if either is set and builds a
default uploader whenever site credentials are present (it delivers GutenbergKit's
own uploads and relays every media delete). `MediaUploadResponse` drops to internal
-- it is no longer on any public API. Both demos and all tests move to the new
protocols.

Breaking change: hosts must migrate `mediaUploadDelegate` (WordPress-iOS/Android,
Jetpack). iOS and Android suites green; SwiftLint and Detekt clean.
MediaUpload.fields was a last-wins map, so a repeated field name (e.g. a
`field[]` array) collapsed to its final value before a host MediaUploader
saw it — diverging from GutenbergKit's own upload path, which preserves
repeats. Pass an ordered list of (name, value) pairs on both platforms.
UploadContext held the processor and uploader `weak`, and each was read
twice per request — once at the admission gate, once at delivery. Those
reads are separated by a synchronous disk copy and an unbounded
`processFile`, so a host that released its handler in that window (the
user closing the editor mid-transcode) changed the answer between them:
a file admitted for processing was forwarded unprocessed, and an upload
gated on a host uploader was delivered by GutenbergKit itself — creating
an attachment on the configured site that the host never learns about,
behind an uploader documented as keeping GutenbergKit "out of the
network entirely".

Hold all three strongly, as Android already does. The `weak` was
load-bearing when `EditorViewController.mediaUploadDelegate` was itself
`weak` and this was the only strong path; 4a03bcc made those properties
strong and left it behind. It no longer prevents a cycle — a host object
retaining the view controller already forms
`EditorViewController -> mediaUploader -> EditorViewController` through
the view controller's own strong property, which this container can
neither create nor prevent.

Immutable strong references also make the two reads agree by
construction, so `handlesFile` admission and delivery can't disagree.
UploadContext becomes a struct and drops its `@unchecked Sendable`
opt-out: both protocols are `Sendable` and InternalMediaClient is
`@unchecked Sendable`, so it is implicitly Sendable.

`doesNotStronglyRetainProcessor` pinned the vestigial invariant, so it is
replaced by `retainsProcessorForServerLifetime`, asserting both halves —
the server owns its processor while it runs, and releases it when the
server goes away. `uploaderReleasedMidRequestStillDelivers` covers the
bug directly; against the previous commit it fails with the real
symptom, the host uploader bypassed and passthroughUpload called.
Both delivery paths could put bytes on the wire after the editor was
gone. `EditorViewController.deinit` calls `stop()`, which cancels the
in-flight connection tasks, but Swift cancellation is cooperative:
`writeStream` is an uninterruptible read loop and a host's `processFile`
need not check at all, so a handler can reach delivery well after
teardown. Whether the request then actually reached WordPress rested
entirely on URLSession noticing the cancellation.

That is not a guarantee the server can rely on. `URLSessionProtocol` is
public and documented for dependency injection, and the obvious
conformance for a host wrapping a callback-based stack —
`withCheckedThrowingContinuation` around a completion handler — has no
cancellation awareness at all. Such a host would upload deterministically
after teardown, and the response is discarded either way, leaving an
attachment on the site that nothing cleans up.

Check cancellation explicitly before delivery in `processAndUpload` and
before `passthroughUpload`, so the guarantee comes from this file rather
than from the HTTP client's behaviour. CancellationError is already
handled quietly by `uploadErrorResponse`, and HTTPServer drops the
response for a cancelled task.
`passthroughResponse` and `handleMediaDelete` took the whole
UploadContext and touched only `internalClient`. Pass that directly.

On the delete path this is more than tidiness. Every attachment lives on
the configured site — even one a host uploader delivered — so a deletion
always relays through the internal client, never the uploader. That was
a convention the signature let you break; now it is a fact the compiler
enforces.

`handleUpload` and `processAndUpload` keep the context: they genuinely
need all three, and spelling them out would push processAndUpload to
nine parameters.
The closure form of `start` can't capture the object that owns the
server: the closure has to exist before the server does, and retrofitting
`self` would form `owner -> HTTPServer -> handler -> owner`, so the
owner's deinit — and its `stop()` — would never run. A consumer with
dependencies to hold therefore ends up with static functions threading a
context parameter through every call, which is how MediaUploadServer is
written today.

Add an `HTTPRequestHandler` protocol and a `start` overload that takes
one. The dependencies become stored properties and the request logic
becomes instance methods. The protocol is deliberately not
`AnyObject`-constrained: a struct conformer cannot participate in a
reference cycle at all, so the ownership question doesn't arise. A final
class works too, under the same leaf discipline HTTPServerDelegate
already documents.

The closure overload is unchanged and forwards to the same code path, so
this is purely additive — no existing caller, test, or the debug server
is affected. Request handling is mandatory, so it can't be a defaulted
HTTPServerDelegate method the way optional customization points are;
hence an overload rather than a new delegate requirement.
Every request function was `private static` taking an UploadContext, for
one reason: the handler closure has to exist before MediaUploadServer
does, so it couldn't capture `self`, and capturing it later would form
`MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer` and
stop `deinit` from ever running `stop()`.

Move them onto a `Handler` struct conforming to the HTTPRequestHandler
protocol added in the previous commit. The dependencies become stored
properties, so the five request functions become instance methods and
drop their context parameter; UploadContext is deleted, since the handler
now *is* the context. A struct can't participate in a reference cycle, so
the constraint that forced the statics is gone rather than worked around.

The statics that remain — errorResponse, relayResponse, attachmentId,
formFields, sanitizeFilename, writeStream, cleanOrphanedUploads — are
pure functions of their arguments. `static` there is not a workaround; it
is the honest signal that they depend on nothing, which is now a
meaningful distinction rather than an artifact of the closure.

No behaviour change and no test changes: the only entry point is
`MediaUploadServer.start`, whose signature is untouched. Reviewing with
`--color-moved` will help — most of the diff is the request block moving
into the struct and gaining a level of indentation.
MediaUpload.fields was `[(name: String, value: String)]` on iOS. Tuples
are not nominal types, so a tuple-typed stored property permanently
blocks synthesized Equatable, Hashable and Codable on MediaUpload —
inside GutenbergKit as well as for hosts, and retroactively, so no later
conformance can recover it. That matters for the offline queue the
MediaUploader docs advertise as a motivating use case: a host that wants
to persist a pending upload's fields has to hand-roll a mirror type.

Unlike the missing public memberwise init on MediaUpload — which stays
internal, matching how this library treats outbound types, and which
could be added later without breaking anyone — this one is not fixable
additively. Changing `fields` after release is a source break for every
host, so it happens now or not at all.

Introduce MediaUploadField (Sendable, Hashable, Codable, public init) and
use it on both platforms. Android had no equivalent defect — Kotlin's
Pair is nominal — but the same change lands there for parity, and
`field.name`/`field.value` reads better than `first`/`second` in a host's
upload code. Kotlin data class destructuring means the multipart writers
are unchanged.

MediaUpload itself is deliberately left non-Codable: it carries a
`fileURL` pointing at a GutenbergKit temp file that will not exist after
a relaunch, so a serialized MediaUpload would be a trap. A host queueing
an upload should copy the bytes and persist the fields, which this type
now supports.
Android refused a mediaUploader when either `siteApiRoot` or `authHeader`
was missing; iOS checked only `authHeader`. So a host with valid
credentials but no site root crashed on Android and started a server on
iOS — the same configuration, opposite outcomes, on a pair of fields that
are both required for the internal media client to reach the configured
site at all.

Gate iOS on both. The types differ — `siteApiRoot` is a `URL` on iOS and
a `String` on Android — so the equivalent of Android's `isEmpty()` is
"not absolute": a URL with no scheme or host cannot address the site, and
every media request built from it fails at the URLSession layer.

Also covers the arm nothing tested. `GutenbergViewUploadServerTest` only
exercised the missing-authHeader case; add its siteApiRoot sibling. There
is no iOS equivalent because `precondition` takes the test process down,
where Kotlin's `check` throws catchably.

The iOS comment claimed every delete "would 500" without credentials.
That is right for a missing site root, where okhttp/URLSession reject the
schemeless URL, but a missing auth header relays WordPress's 401 instead.
Say "would fail", which is true of both.
The previous commit claimed this policy was untestable on iOS because
`precondition` takes the test process down. That was wrong: Swift Testing
has exit tests, which run the body in a child process.

The real obstacle was narrower. Exit tests are unavailable on iOS and the
simulator ("Exit tests are not available on this platform"), and the
policy lived in EditorViewController, which is `#if canImport(UIKit)` and
therefore absent from the macOS host — so the one platform that can run
exit tests couldn't see the code. The intersection was empty because of
where the code sat, not because of the tool.

Move the decision into MediaServerCredentials, outside the UIKit gate,
and have startUploadServer call it. The predicate and the fail-fast are
now both reachable from the host suite: six tests pin the predicate
(including the two arms of the site-root check that a `URL` makes
different from Android's `String`), and two exit tests pin the trap
itself. Neutering the precondition fails both, so they are not vacuous.

This also gives the crash policy a named home. It diverged silently
between iOS and Android once already; a host-testable predicate is
harder to let drift again.
`formFields` decodes each non-file form value as UTF-8, which substitutes
U+FFFD on malformed input. That is lossless today, but only because of an
invariant nothing in the code states or enforces: the sole client is the
editor's browser FormData. The server binds to loopback behind a
per-session token; a FormData string value is a USVString, already
well-formed at append time; and its only way to carry arbitrary bytes is
a Blob, which always gets a filename and is filtered out of `extraParts`.

Write that down on both platforms, including the part that makes it
matter — if it stops holding, the two platforms are lossy *differently*
(for ED A0 80, Swift's maximal-subpart rule yields three replacement
characters where Java's decoder yields one), so there is no single
behaviour that could be documented instead.

Also reword the raw-bytes comment on the re-encode path. "So a non-UTF-8
value is forwarded verbatim" read as though malformed values were
expected, which made the two delivery paths look contradictory. The
actual hazard is the failable `String(data:encoding:)` returning nil and
an obvious `?? ""` dropping the whole value; the reason to keep bytes is
that the re-encode should stay byte-identical to the passthrough it
stands in for.

Cover the partition rather than the decode, since the partition is what
makes the invariant true: a request carrying a second, Blob-shaped part
whose bytes are not valid UTF-8 must not surface that part in `fields`.
Both tests fail when the filename filter is relaxed, so neither is
vacuous. Neither asserts what becomes of that second part — it is
currently dropped rather than relayed, which is a separate open question.
@jkmassel
jkmassel force-pushed the fix/register-core-media-upload-middleware branch from 910fc77 to b6d05cb Compare September 5, 2026 02:52
@jkmassel
jkmassel force-pushed the fix/register-core-media-upload-middleware branch from b6d05cb to 09d33ef Compare September 8, 2026 16:11
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.

3 participants