Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/628")Built from 204040e |
0ab9ee5 to
fadbd45
Compare
fadbd45 to
589f32f
Compare
039cbed to
549b518
Compare
adalpari
left a comment
There was a problem hiding this comment.
It looks good to me.
Claude found this nuances on Android. Not a blocker, though.
Uploader-only hosts silently get no upload server — android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt:707 (same on iOS, EditorViewController.swift:488)
The start guard still bails on empty authHeader/siteApiRoot. With an uploader set, mediaUploader is then never called, uploads fall back to the WebView path, and nothing is logged. The guard’s rationale (“nothing to upload through”) no longer applies once an uploader exists.
Android cancellation gap remains in passthroughResponse — android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt:403
The PR closes the gap in processAndUpload but not here, where iOS has a second Task.checkCancellation(). A cancelled request can still enqueue the passthrough POST and orphan an attachment.
| uploadServer = MediaUploadServer( | ||
| uploadDelegate = mediaUploadDelegate, | ||
| internalClient = internalClient, | ||
| uploader = mediaUploader, |
There was a problem hiding this comment.
Finding from Claude:
With mediaUploader set, a server that doesn't come up still falls back to the WebView uploading directly, so the uploader is bypassed without any signal to the host. That happens here when cleartext to localhost isn't permitted (:716, the default at targetSdk 28+ without a network-security entry) or this constructor throws (:742), and on iOS when MediaUploadServer.start throws or times out (EditorViewController.swift:508). WordPress-Android isn't affected, since its base-config permits cleartext, but other hosts would be.
#632 traps the missing-credentials case because falling back "would silently drop" the uploader. Should these paths follow suit for an uploader host? A check fits the cleartext case, which is configuration like #632's, and a runtime start failure could fail uploads rather than bypass the uploader.
| fields = formFields(extraParts), | ||
| query = query | ||
| ) | ||
| return UploadResult.Uploaded(MediaUploadResponse(201, hostUploader.upload(upload))) |
There was a problem hiding this comment.
Finding from Claude:
If a host's own withTimeout fires inside upload, its TimeoutCancellationException is a CancellationException, so processAndRespond rethrows it as teardown (:507). The socket closes without a response, and after the server's 5s idle timeout the editor shows "Could not get a valid response from the server." iOS returns a 500 with the message in the same case.
Rethrowing only when this coroutine is actually cancelled would fix it, replacing the catch body at :507:
} catch (e: kotlin.coroutines.cancellation.CancellationException) {
currentCoroutineContext().ensureActive()
Log.e(TAG, "Upload failed", e)
return errorResponse(500, e.message ?: "Upload failed")
}Repro: local, uncommitted test on #629's head (d8b85e24)
private class TimingOutUploader : MediaUploader {
override suspend fun upload(upload: MediaUpload): ByteArray = withTimeout(50) { awaitCancellation() }
}A POST /upload with this uploader gets no status line, and the connection closes after 5s. The same request with an uploader that throws IOException gets a 500 carrying its message in 6ms.
| /// can't be recovered, force-delete the orphan | ||
| /// (`DELETE /wp/v2/media/<id>?force=true`) before you `throw`, or it stays on the | ||
| /// site — neither GutenbergKit nor the editor cleans up behind you. | ||
| func upload(_ upload: MediaUpload) async throws -> Data |
There was a problem hiding this comment.
Finding from Claude:
A throw is the only way to report failure, and every throw reaches the editor as a 500 upload_error carrying error.localizedDescription (MediaUploadServer.swift:280; Android uses e.message). A host that throws a plain Swift error for WordPress's 403 rest_cannot_create gets "The operation couldn't be completed. (…)" in the editor's failure notice. A LocalizedError gets WordPress's message through, but its status and error code are lost either way, and #629 removes uploadFile, the one hook that relayed them.
Document the LocalizedError behavior here, or give hosts a way to hand back WordPress's status and body?
Repro: local, uncommitted tests on #629's head (d8b85e24)
An uploader throwing WordPressRejection(code: "rest_cannot_create", message: "Sorry, you are not allowed to upload this file type."):
- As a plain
Error: status 500,codeisupload_error, andmessagestarts with "The operation couldn" and omits WordPress's message. - As a
LocalizedErrorreturning that message: status 500,codeisupload_error, andmessageis WordPress's message.
| /// Everything a ``MediaUploader`` needs to reproduce a native upload: the file to | ||
| /// send, its metadata, the editor's non-file form fields, and the request's query. | ||
| public struct MediaUpload: Sendable { | ||
| /// The file to upload — already processed, if a ``MediaUploadDelegate`` ran. |
There was a problem hiding this comment.
Finding from Claude:
The protocol docs suggest a background session or an offline queue, but fileURL is a temp file deleted as soon as upload returns, throws, or is cancelled (MediaUploadServer.swift:172 and :331-335; Android's finally at MediaUploadServer.kt:519). A host that hands the URL to a persistent queue can find it gone by the time the job runs. upload(_:) could also use a line on cancellation: honor it, and delete an attachment created after it, since nothing else will.
| /// The file to upload — already processed, if a ``MediaUploadDelegate`` ran. | |
| /// The file to upload — already processed, if a ``MediaUploadDelegate`` ran. | |
| /// | |
| /// A temporary file, deleted as soon as ``MediaUploader/upload(_:)`` returns, | |
| /// throws, or is cancelled. Copy it first if your upload needs it after that. |
| /// | ||
| /// Takes precedence over the deprecated ``MediaUploadDelegate/uploadFile(at:mimeType:filename:)``: | ||
| /// with an uploader set, that hook is never called. | ||
| public var mediaUploader: (any MediaUploader)? { |
There was a problem hiding this comment.
Finding from Claude:
This inherits the ownership question from the #625 thread, so the decision there should cover both properties. An uploader is the likelier of the two to reference the editor's owner and close the cycle.
The off-main release point applies here too. A local, uncommitted test on #629's head (d8b85e24) stops the server on the main thread while upload is still running: the uploader outlives stop(), then deallocates off the main thread once upload returns. The same holds for a delegate still in processFile. An idle server does release it on main, so releaseConnectionHandler covers only the case with no upload in flight.
| */ | ||
| var mediaUploader: MediaUploader? = null | ||
| set(value) { | ||
| check(!hasStartedLoading) { lateMediaAssignmentMessage("mediaUploader") } |
There was a problem hiding this comment.
Finding from Claude:
Nit: the hasStartedLoading docs (:155-157 here, EditorViewController.swift:107-109) still name only the delegate, though a late mediaUploader write now throws (traps on iOS) too.
549b518 to
dbaf85b
Compare
Performing a media upload — and retrying it — should be a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does. Both go to the same configured site; the only difference is who executes the requests. `MediaUploadDelegate.uploadFile` doesn't offer that. A host performs the `POST /wp/v2/media` and returns the raw response it received — then the editor, reading that response, drives the `post-process` retries and the orphan cleanup behind it, through the WebView rather than the host's stack. A host that took over uploads to run them through its own networking still didn't own the retries. It also receives no form fields, so an attachment it uploads lands unattached to its post. Add `MediaUploader`, which owns the upload end to end: - `upload(_:)` returns the finished attachment or throws. There is no raw response left for the editor to retry behind it, so the host drives its own post-process recovery and force-deletes its own orphan on terminal failure. - It receives a `MediaUpload` carrying the file, its metadata, the editor's non-file form fields (`post`, additionalData) and the request query (`?_embed`) — everything needed to reproduce a native request. - Fields are a `MediaUploadField` list rather than a dictionary, so repeated names (a `field[]` array) survive verbatim and in order. Additive for hosts: `uploadFile` still works and is marked deprecated, pointing them at the replacement, and an uploader takes precedence when both are set. Internally the upload server's startup gate widens to admit an uploader as well as a delegate. GutenbergKit's own build keeps one deprecation warning at the call site that supports the old hook — the marker exists to tell hosts to migrate, and supporting the hook until it is removed means calling it. With an uploader set, the delegate's metadata gate can no longer decline a file: the gate exists to skip a temp copy for a file the delegate won't touch, but an uploader takes over delivery for *every* file, so passing through would silently bypass it. Covered on both platforms. `MediaUploadServerTest` crosses Detekt's LargeClass threshold; baselined rather than split, which is its own change.
…ound it
Follow-ups to the MediaUploader commit: a behavior bug in the metadata gate, a
cross-platform divergence, a missing cancellation check on Android, four
documentation defects, two test gaps, and a shadowed local.
- `processFile` ran on a file the delegate's metadata gate had declined. Widening
the gate to `uploader != nil || delegateWantsFile` left `processFile` called
unconditionally, so an image-only delegate paired with an uploader was handed
the `.mov` it had just said it won't touch — breaking the contract
`handlesFile` documents. `delegateWantsFile` is now carried into
`processAndUpload` and gates `processFile`. With an uploader set the file is
still delivered; it just skips processing on the way.
- iOS evaluated `handlesFile` eagerly while Android's `&&` short-circuited past
it, so the same host saw one callback per upload on iOS and zero on Android.
Android now binds it eagerly too: asked exactly once per upload on both.
- Android had no pre-flight cancellation check before handing work to the host
uploader, where iOS has `Task.checkCancellation()`. Added
`currentCoroutineContext().ensureActive()`, so a torn-down editor no longer
starts an upload whose attachment nobody would clean up.
- The recovery recipe omitted `post-process`'s required `action` parameter. Core
registers `action` as required, so a host following the doc verbatim would 400
five times and then run the doc's *other* instruction —
`DELETE /wp/v2/media/<id>?force=true` — destroying an attachment
`wp_update_image_subsizes()` would have recovered.
- `mediaUploader`'s doc had been appended to `mediaUploadDelegate`'s `///` block,
merging the two: `mediaUploadDelegate` shipped with no documentation and
`mediaUploader` opened by describing a delegate. Confirmed with
`swiftc -emit-symbol-graph` (`mediaUploadDelegate => None`); both now bind
their own 11 lines.
- `formFields` and `deprecatedUploadFile` were inserted between
`attachmentId(fromPath:)`'s doc and its declaration — merging into it on iOS,
dropping it outright on Android — costing the "deliberately narrow, not a
general REST proxy" rationale. Moved below their only caller, per AGENTS.md's
call-order rule.
- `ReplaceWith("MediaUploader")` takes a replacement *expression*; applying the
quick-fix drops all three arguments and leaves a type name where a
`MediaUploadResponse?` was expected. Removed, with a note so it doesn't return.
- Nothing pinned the Android gate or the uploader/deprecated-hook precedence:
deleting `&& mediaUploader == null` or reordering the two delivery paths left
the suite green. Three tests added; both mutations now fail.
- `DecliningDelegate` duplicated the pre-existing `DeclineByMetadataDelegate`
minus its `processFileCalled` recorder — the one probe that catches the
`processFile` bug above. Merged, and the declined-file test now asserts it.
- Two locals named `uploader` shadowed the new `MediaUploader` property, silently
(kotlinc has no diagnostic for it, detekt no rule). Renamed to `client`.
- `RecordingUploader`'s fixture carried no `title`, so the repo's only worked
example of an uploader result was a body that trips `transformAttachment`.
dbaf85b to
204040e
Compare
…loader `startUploadServer()` asks "did the host supply a media handler" twice — once before starting, once after the bind returns, because `stopMediaHandling()` can land while that `await` is suspended. The two reads had drifted. #628 widened the first to `delegate != nil || uploader != nil` and left the second checking the delegate alone. So a host that passed only a `mediaUploader` cleared the entry check, bound a loopback listener, then failed the post-bind check and stopped the server it had just started. `uploadServer` stayed nil, `buildEditorConfiguration` advertised `nativeUploadPort: nil`, and `api-fetch.js` fell through to the plain WebView path. The host's `upload(_:)` was **never called, for any file** — no error, no log. Uploads appeared to work; they just never reached the host's background session, offline queue, or retry policy, which is the whole reason to supply an uploader. Both reads now go through one `hasMediaHandling`, so they cannot disagree again. That is the actual defect — two hand-maintained copies of one predicate — and it is the same failure `MediaServerCredentials` was extracted for, where a check "diverged silently between iOS and Android once". `uploadServer` and `startUploadServer()` become internal so the suite can reach them. The test is parameterized over uploader-only, processor-only and both. Mutation-checked: restoring the old post-bind guard fails **only** the uploader-only case, which is the regression and nothing else. Android already pinned this gate (`GutenbergViewUploadServerTest`, "the upload server starts for an uploader with no delegate"); iOS had no equivalent, which is why the drift survived three commits green.
… class bound (#685) * refactor!: rename MediaUploadDelegate to MediaProcessor The protocol no longer uploads anything — the previous commit removed `uploadFile`, leaving `handlesFile` and `processFile`. "UploadDelegate" now describes the one thing it can't do, and next to `MediaUploader` the two names read as variations on the same job rather than the two halves of a deliberate split. `MediaProcessor` says what is left: it transforms bytes, GutenbergKit delivers them. Mechanical throughout — the property becomes `mediaProcessor`, the server parameter `processor`, the file `MediaHandlers.swift` (it holds both protocols now), and Android's demo `DemoMediaProcessor`. Prose follows the types. The `weak_delegate` suppression added when the property became strong goes away with the name: the rule was arguably right that a strongly-held "delegate" is a smell, and the answer was that this was never a delegate. BREAKING CHANGE: `mediaUploadDelegate` is now `mediaProcessor`, and `MediaUploadDelegate` is `MediaProcessor`. Conformances need no changes beyond the name. * refactor(ios)!: drop the class requirement from the media protocols `MediaProcessor` and `MediaUploader` were both `AnyObject`-bound, and `EditorViewController` holds both strongly. A conformer that holds the view controller back therefore closes a retain cycle ARC cannot break: the editor is never freed, so `deinit` never runs, so `uploadServer.stop()` — its only caller — never runs either, and a bound loopback `NWListener` outlives the editing session. Nothing needed class-boundness. There is no `weak`, `===`, or `ObjectIdentifier` use against either protocol anywhere in the tree, and every existing conformer is a class, which conforms unchanged. Dropping the requirement lets a host conform with a value type capturing only what the work needs — the shape that avoids the cycle, and the one a class-bound `Delegate` discouraged. This does not make the cycle impossible: a struct that stores the view controller cycles just the same. The docs say so rather than implying the type system settles it. * fix(ios): name every media handler in the leak census, not just the first `countServerStarted` resolved its name as `processor.map { … } ?? uploader.map { … }`, so with both supplied it always named the processor. The retainer is as likely to be the uploader — and after the class bound came off `MediaProcessor`, the processor it names may be a value type holding nothing at all, which is the one shape that provably cannot close the cycle the fault is reporting. A host following the docs hits this on the recommended shape: a leaf processor for the transform plus an uploader on the coordinator that owns the editor. The fault named the leaf, so the reader audits an object with no stored references, finds nothing, and concludes the census is broken. Names every handler that was supplied, and softens the assertion from "is its own media handler" to "is one of its own media handlers" — with two names it is a candidate list, not an accusation. DEBUG-only, and still behind the `count >= liveServerLeakThreshold` guard, so `String(describing: type(of:))` stays off the start path. * fix(ios): start the upload server for a host that supplies only an uploader `startUploadServer()` asks "did the host supply a media handler" twice — once before starting, once after the bind returns, because `stopMediaHandling()` can land while that `await` is suspended. The two reads had drifted. #628 widened the first to `delegate != nil || uploader != nil` and left the second checking the delegate alone. So a host that passed only a `mediaUploader` cleared the entry check, bound a loopback listener, then failed the post-bind check and stopped the server it had just started. `uploadServer` stayed nil, `buildEditorConfiguration` advertised `nativeUploadPort: nil`, and `api-fetch.js` fell through to the plain WebView path. The host's `upload(_:)` was **never called, for any file** — no error, no log. Uploads appeared to work; they just never reached the host's background session, offline queue, or retry policy, which is the whole reason to supply an uploader. Both reads now go through one `hasMediaHandling`, so they cannot disagree again. That is the actual defect — two hand-maintained copies of one predicate — and it is the same failure `MediaServerCredentials` was extracted for, where a check "diverged silently between iOS and Android once". `uploadServer` and `startUploadServer()` become internal so the suite can reach them. The test is parameterized over uploader-only, processor-only and both. Mutation-checked: restoring the old post-bind guard fails **only** the uploader-only case, which is the regression and nothing else. Android already pinned this gate (`GutenbergViewUploadServerTest`, "the upload server starts for an uploader with no delegate"); iOS had no equivalent, which is why the drift survived three commits green. * test: finish the rename in the media suites' own vocabulary The rename swept the helper types and left the names around them. 101 sites across four files: iOS test functions and `@Test` display strings, Kotlin backtick names, `let delegate = ProcessOnlyProcessor()` bindings that contradicted themselves on one line, `weakDelegate`, and a `// MARK: - Upload with delegate` header over code the production file had already renamed to `// MARK: - Processor Pipeline`. These are the strings CI prints. A red build named `retainsDelegateForServerLifetime` or `processes with the delegate, then delivers through the internal client` for a codebase where no symbol contains the word — Kotlin backticks are literally the JUnit report strings — so the first move on a failure was to grep for an API this stack deleted. Safe as a plain substring replacement: none of the four files reference a genuine delegate. `HttpServerDelegate` and `EditorViewControllerDelegate` live in other test files and are untouched. Two names would have read as stutters after a mechanical pass, so they say what the test does instead: `processesThenDelivers` and `processorRunsForUploader`. Test counts are unchanged — 590 iOS, and Android green on `--rerun-tasks` — so this renames tests rather than adding or dropping any. Note it does reset Buildkite Test Analytics history for the renamed cases, which is the deliberate cost. * test(ios): pin that a value type can conform to MediaProcessor Dropping `: AnyObject` is what the second commit here exists to deliver, and nothing exercised it — all eleven conformers in the tree were classes, so the boxed-existential path was never walked: copied into `UploadContext`, captured by the `@Sendable` handler closure, read again at `processFile`. Re-imposing the class bound, or breaking that path, would have compiled and passed green and surfaced only in a host's build. It now fails at compile time: error: non-class type 'ValueTypeProcessor' cannot conform to class protocol 'MediaProcessor' `ValueTypeProcessor` is `Sendable` without `@unchecked` — also the point, since that is the shape the protocol's documentation now recommends and the escape hatch it describes. The assertions run through `MockInternalMediaClient`'s recorded metadata rather than state on the processor, because a `struct` witnessing a non-mutating requirement cannot record anything. The transcoded mimeType and filename reaching the client could only come from `processFile` having actually run, so this pins invocation, not just storage.
… class bound (#685) * refactor!: rename MediaUploadDelegate to MediaProcessor The protocol no longer uploads anything — the previous commit removed `uploadFile`, leaving `handlesFile` and `processFile`. "UploadDelegate" now describes the one thing it can't do, and next to `MediaUploader` the two names read as variations on the same job rather than the two halves of a deliberate split. `MediaProcessor` says what is left: it transforms bytes, GutenbergKit delivers them. Mechanical throughout — the property becomes `mediaProcessor`, the server parameter `processor`, the file `MediaHandlers.swift` (it holds both protocols now), and Android's demo `DemoMediaProcessor`. Prose follows the types. The `weak_delegate` suppression added when the property became strong goes away with the name: the rule was arguably right that a strongly-held "delegate" is a smell, and the answer was that this was never a delegate. BREAKING CHANGE: `mediaUploadDelegate` is now `mediaProcessor`, and `MediaUploadDelegate` is `MediaProcessor`. Conformances need no changes beyond the name. * refactor(ios)!: drop the class requirement from the media protocols `MediaProcessor` and `MediaUploader` were both `AnyObject`-bound, and `EditorViewController` holds both strongly. A conformer that holds the view controller back therefore closes a retain cycle ARC cannot break: the editor is never freed, so `deinit` never runs, so `uploadServer.stop()` — its only caller — never runs either, and a bound loopback `NWListener` outlives the editing session. Nothing needed class-boundness. There is no `weak`, `===`, or `ObjectIdentifier` use against either protocol anywhere in the tree, and every existing conformer is a class, which conforms unchanged. Dropping the requirement lets a host conform with a value type capturing only what the work needs — the shape that avoids the cycle, and the one a class-bound `Delegate` discouraged. This does not make the cycle impossible: a struct that stores the view controller cycles just the same. The docs say so rather than implying the type system settles it. * fix(ios): name every media handler in the leak census, not just the first `countServerStarted` resolved its name as `processor.map { … } ?? uploader.map { … }`, so with both supplied it always named the processor. The retainer is as likely to be the uploader — and after the class bound came off `MediaProcessor`, the processor it names may be a value type holding nothing at all, which is the one shape that provably cannot close the cycle the fault is reporting. A host following the docs hits this on the recommended shape: a leaf processor for the transform plus an uploader on the coordinator that owns the editor. The fault named the leaf, so the reader audits an object with no stored references, finds nothing, and concludes the census is broken. Names every handler that was supplied, and softens the assertion from "is its own media handler" to "is one of its own media handlers" — with two names it is a candidate list, not an accusation. DEBUG-only, and still behind the `count >= liveServerLeakThreshold` guard, so `String(describing: type(of:))` stays off the start path. * fix(ios): start the upload server for a host that supplies only an uploader `startUploadServer()` asks "did the host supply a media handler" twice — once before starting, once after the bind returns, because `stopMediaHandling()` can land while that `await` is suspended. The two reads had drifted. #628 widened the first to `delegate != nil || uploader != nil` and left the second checking the delegate alone. So a host that passed only a `mediaUploader` cleared the entry check, bound a loopback listener, then failed the post-bind check and stopped the server it had just started. `uploadServer` stayed nil, `buildEditorConfiguration` advertised `nativeUploadPort: nil`, and `api-fetch.js` fell through to the plain WebView path. The host's `upload(_:)` was **never called, for any file** — no error, no log. Uploads appeared to work; they just never reached the host's background session, offline queue, or retry policy, which is the whole reason to supply an uploader. Both reads now go through one `hasMediaHandling`, so they cannot disagree again. That is the actual defect — two hand-maintained copies of one predicate — and it is the same failure `MediaServerCredentials` was extracted for, where a check "diverged silently between iOS and Android once". `uploadServer` and `startUploadServer()` become internal so the suite can reach them. The test is parameterized over uploader-only, processor-only and both. Mutation-checked: restoring the old post-bind guard fails **only** the uploader-only case, which is the regression and nothing else. Android already pinned this gate (`GutenbergViewUploadServerTest`, "the upload server starts for an uploader with no delegate"); iOS had no equivalent, which is why the drift survived three commits green. * test: finish the rename in the media suites' own vocabulary The rename swept the helper types and left the names around them. 101 sites across four files: iOS test functions and `@Test` display strings, Kotlin backtick names, `let delegate = ProcessOnlyProcessor()` bindings that contradicted themselves on one line, `weakDelegate`, and a `// MARK: - Upload with delegate` header over code the production file had already renamed to `// MARK: - Processor Pipeline`. These are the strings CI prints. A red build named `retainsDelegateForServerLifetime` or `processes with the delegate, then delivers through the internal client` for a codebase where no symbol contains the word — Kotlin backticks are literally the JUnit report strings — so the first move on a failure was to grep for an API this stack deleted. Safe as a plain substring replacement: none of the four files reference a genuine delegate. `HttpServerDelegate` and `EditorViewControllerDelegate` live in other test files and are untouched. Two names would have read as stutters after a mechanical pass, so they say what the test does instead: `processesThenDelivers` and `processorRunsForUploader`. Test counts are unchanged — 590 iOS, and Android green on `--rerun-tasks` — so this renames tests rather than adding or dropping any. Note it does reset Buildkite Test Analytics history for the renamed cases, which is the deliberate cost. * test(ios): pin that a value type can conform to MediaProcessor Dropping `: AnyObject` is what the second commit here exists to deliver, and nothing exercised it — all eleven conformers in the tree were classes, so the boxed-existential path was never walked: copied into `UploadContext`, captured by the `@Sendable` handler closure, read again at `processFile`. Re-imposing the class bound, or breaking that path, would have compiled and passed green and surfaced only in a host's build. It now fails at compile time: error: non-class type 'ValueTypeProcessor' cannot conform to class protocol 'MediaProcessor' `ValueTypeProcessor` is `Sendable` without `@unchecked` — also the point, since that is the shape the protocol's documentation now recommends and the escape hatch it describes. The assertions run through `MockInternalMediaClient`'s recorded metadata rather than state on the processor, because a `struct` witnessing a non-mutating requirement cannot record anything. The transcoded mimeType and filename reaching the client could only come from `processFile` having actually run, so this pins invocation, not just storage.
Stacked on #627. Fifth of ten PRs splitting #621, and the heart of it.
A host that doesn't adopt
MediaUploadersees no behavior change.MediaUploadDelegate.uploadFilestill works and still returns what it returned; it gains a deprecation warning pointing at the replacement, and #629 removes it. What changes shape is internal: the upload server's startup gate now admits an uploader as well as a delegate, and the pipeline threads the delegate's metadata answer through to processing.What?
A
MediaUploaderprotocol that takes over performing a media upload on the host's own stack, and owns its whole lifecycle: retries, recovery, and cleanup.Why?
Performing a media upload — and retrying it — should be a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does. Both go to the same configured site; the only difference is who executes the requests.
MediaUploadDelegate.uploadFiledoesn't offer that. A host performs thePOST /wp/v2/mediaand returns the raw response it received — then the editor, reading that response, drives thepost-processretries and the orphan cleanup behind it, through the WebView rather than the host's stack. A host that took over uploads to run them through its own networking still didn't own the retries. It also receives no form fields, so an attachment it uploads lands unattached to its post.Neither is fixable while the hook returns a raw response, which is what the replacement changes.
How?
MediaUploaderupload(_:)returns the finished attachment or throws. There is no raw response left for the editor to retry behind it, so the host drives its ownpost-processrecovery and force-deletes its own orphan on terminal failure.MediaUploadCarries the file, its metadata, the editor's non-file form fields (
post, additionalData) and the request query (?_embed) — everything needed to reproduce a native request.MediaUploadFieldFields are an ordered list rather than a dictionary, so repeated names (a
field[]array) survive verbatim and in order. A named type rather than a tuple: tuples are not nominal, so a tuple-typed property would permanently blockEquatable/Hashable/Codablesynthesis onMediaUpload, and that is not fixable later without a source break for every host.Precedence
uploadFilestill works and is marked deprecated, pointing hosts at the replacement; an uploader takes precedence when both are set. #629 removes the old hook, so hosts get a migration window rather than a flag day.This deliberately leaves one deprecation warning in GutenbergKit's own build, at the call site that supports the old hook (
MediaUploadServer.swift:397). The marker exists to tell hosts to migrate, and supporting the hook until it is removed means calling it. It goes away with #629.What the metadata gate decides
handlesFileis the delegate's cheap, type-only veto, consulted before the upload is copied to a temp file. With an uploader set it no longer decides whether the upload happens — an uploader takes over delivery for every file, so there is no passthrough left to decline to.It still decides whether
processFileruns. A file the delegate declined by type is delivered to the uploader unprocessed, rather than handed to a delegate that just said it won't touch a file like that — an image-only delegate never sees a.mov.handlesFileis consulted exactly once per upload, on both platforms.Cancellation before delivery
Android now checks for cancellation immediately before handing the file to the uploader, matching the iOS check #626 added. A host uploader that isn't cancellation-cooperative — a background service, a work queue — would otherwise finish a
POSTfor an editor that is already gone, leaving an attachment that, by this protocol's own contract, nobody cleans up.Recovery is the host's, and it needs one parameter that isn't obvious
When
POST /wp/v2/mediafatals in server-side post-processing it returns a 5xx carrying the attachment ID inx-wp-upload-attachment-id— the attachment exists but is unfinished. GutenbergKit is out of the network for an uploader host, so the editor-side middleware never sees that 5xx and cannot recover it.The protocol doc spells out the recipe, including the part that bites:
POST /wp/v2/media/<id>/post-processneeds a body of{"action": "create-image-subsizes"}. Core registersactionas required, so a recovery loop that omits it returns 400 on every attempt and then force-deletes an attachmentwp_update_image_subsizes()would have finished.Testing Instructions
Five tests on iOS and six on Android. Both platforms cover delivery, form fields and query, precedence over
uploadFile, the declined-file case, and a terminal throw surfacing without GutenbergKit re-delivering. Android adds two for the startup gate — thatmediaUploaderalone brings the server up, and that assigning it after load traps — which iOS cannot cover from a test target, becauseEditorViewControlleris behind#if canImport(UIKit).swift-ios-simulator-tests,android-test-android-library, and both E2E suitesswift test— 974 tests, 0 failures:Gutenberg:testDebugUnitTest— 672 tests, 0 failuresxcodebuild— compiles the UIKit-gatedEditorViewController, which the host test target does not:Gutenberg:compileDebugUnitTestKotlinemits no warningsTo exercise the recovery path against a real fatal,
make wp-env-media-failure MODE=alwaysforceswp_generate_attachment_metadata()to fail — seedocs/code/local-wordpress.md. Note that aMediaUploaderhost is responsible for the retry loop itself, so this exercises the host's implementation, not GutenbergKit's.MediaUploadServerTestcrosses Detekt'sLargeClassthreshold; baselined rather than split, which is its own change.