From 2ca71a29557bdbe696a033fe14215e56c15feab1 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:36:31 -0600 Subject: [PATCH 1/2] feat!: remove MediaUploadDelegate.uploadFile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../org/wordpress/gutenberg/GutenbergView.kt | 4 +- .../wordpress/gutenberg/MediaUploadServer.kt | 54 +++++------------- .../gutenberg/MediaUploadServerTest.kt | 48 +++++----------- .../gutenbergkit/DemoMediaUploadDelegate.kt | 2 +- .../Sources/EditorViewController.swift | 4 +- .../Sources/Media/MediaUploadDelegate.swift | 56 +++++++------------ .../Sources/Media/MediaUploadServer.swift | 20 +------ .../Media/MediaUploadServerTests.swift | 43 +++++--------- 8 files changed, 68 insertions(+), 163 deletions(-) diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt index fee46d45b..ef5764d95 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -136,8 +136,8 @@ class GutenbergView : FrameLayout { * and this view owns it for its lifetime — so you needn't retain it yourself, just * don't strongly retain this [GutenbergView] from your uploader. * - * Takes precedence over the deprecated [MediaUploadDelegate.uploadFile]: with an - * uploader set, that hook is never called. + * A [mediaUploadDelegate] can still transform the file first; only delivery moves + * to the uploader. */ var mediaUploader: MediaUploader? = null set(value) { diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt index 27c58f2f4..703ff8066 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -33,7 +33,7 @@ import okio.source * so every consumer — image sub-sizes, attachment links, error notices — * behaves identically to a non-native upload. */ -class MediaUploadResponse( +internal class MediaUploadResponse( /** The HTTP status code WordPress (or the host's upload service) returned. */ val statusCode: Int, /** @@ -70,23 +70,29 @@ sealed class ProcessedProxyFile { } /** - * Interface for customizing media upload behavior. + * Transforms media before GutenbergKit delivers it. * - * The native host app can provide an implementation to resize images, - * transcode video, or use its own upload service. + * A delegate only changes *bytes* — GutenbergKit still uploads the result to the + * configured site and owns the whole lifecycle (retries, cleanup). Because it never + * performs the upload itself, it cannot deliver media to the wrong place. Set + * [GutenbergView.mediaUploadDelegate] to resize images, transcode video, strip EXIF, + * etc. + * + * This is the safe, common extension point: most hosts want only this. To perform the + * upload yourself, implement [MediaUploader] instead. */ interface MediaUploadDelegate { /** - * Whether this delegate might handle a file with the given metadata — either - * processing it ([processFile]) or uploading it itself ([uploadFile]). + * Whether this delegate might transform a file with the given metadata. * * A cheap, metadata-only gate the server consults *before* materializing the * upload to a temp file. Return false to decline a file by type — e.g. an * image-only delegate returning false for a video — so the server forwards * the original upload to WordPress without first copying a file the delegate - * won't touch. Because it gates the temp-file copy needed by *both* - * [processFile] and [uploadFile], return true for any file the delegate will - * either process or upload itself. + * won't touch. + * + * Only consulted when no [MediaUploader] is set: an uploader takes over delivery + * for every file, so there is no passthrough to decline to. * * Defaults to true: every file is materialized and the full pipeline runs. A * true here is not a commitment — [processFile] may still return @@ -104,29 +110,6 @@ interface MediaUploadDelegate { */ suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile = ProcessedProxyFile.Original - /** - * Upload a processed file to the remote WordPress site. - * - * Return the raw WordPress response (status code + body), which GutenbergKit - * relays to the editor unchanged, or null to use the internal media client. A - * host that uploads to WordPress should return the exact response it received so - * the editor sees a complete attachment object. - * - * Returning a raw response splits one upload's HTTP across two owners: you - * perform the POST, but the editor drives the `post-process` retries and orphan - * cleanup behind it, through the WebView rather than your stack. It also receives - * no form fields, so an attachment uploaded this way lands unattached to its post. - * Implement [MediaUploader] instead — it owns the upload end-to-end and receives a - * [MediaUpload] carrying the fields. - */ - // No ReplaceWith: it takes a replacement *expression* the IDE substitutes for the - // call, and there is none that means "implement a different interface" — the - // quick-fix would drop the arguments and leave a type name where a - // MediaUploadResponse? was expected. The message carries the guidance instead. - @Deprecated( - "Implement MediaUploader instead — it owns the upload's retries and receives the editor's form fields." - ) - suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? = null } /** @@ -594,13 +577,6 @@ internal class MediaUploadServer( return UploadResult.Uploaded(MediaUploadResponse(201, hostUploader.upload(upload))) } - // The deprecated delegate path: the host performs the POST but returns the - // raw response, leaving the editor to drive post-process recovery behind it. - @Suppress("DEPRECATION") - uploadDelegate?.uploadFile(targetFile, targetMimeType, targetFilename)?.let { - return UploadResult.Uploaded(it) - } - // Unmodified — forward the original request body directly, skipping // multipart re-encoding. if (processed is ProcessedProxyFile.Original) { diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index 3e43d0902..e942939bc 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -245,11 +245,11 @@ class MediaUploadServerTest { } @Test - fun `an uploader takes precedence over the deprecated uploadFile hook`() { - // Both set: the uploader owns delivery and the deprecated hook must not run. - // The delegate still processes — only delivery moves to the uploader. + fun `a delegate still processes the file an uploader delivers`() { + // With both set, the delegate still processes — only delivery moves to + // the uploader. val uploader = RecordingUploader() - val delegate = MockUploadDelegate() + val delegate = ProcessOnlyDelegate() val client = MockInternalMediaClient() server.stop() server = MediaUploadServer( @@ -270,7 +270,6 @@ class MediaUploadServerTest { ) assertNotNull(uploader.received) - assertFalse(delegate.uploadFileCalled) assertTrue(delegate.processFileCalled) assertFalse(client.uploadCalled) } @@ -343,10 +342,11 @@ class MediaUploadServerTest { // MARK: - Upload with delegate @Test - fun `calls delegate processFile and uploadFile`() { - val delegate = MockUploadDelegate() + fun `processes with the delegate, then delivers through the internal client`() { + val delegate = TranscodingDelegate() + val client = MockInternalMediaClient() server.stop() - server = MediaUploadServer(uploadDelegate = delegate, internalClient = null, cacheDir = tempFolder.root) + server = MediaUploadServer(uploadDelegate = delegate, internalClient = client, cacheDir = tempFolder.root) val boundary = "test-boundary-123" val body = buildMultipartBody(boundary, "photo.jpg", "image/jpeg", "fake image data".toByteArray()) @@ -362,16 +362,14 @@ class MediaUploadServerTest { ) assertTrue("Expected 201 but got: ${response.statusLine}", response.statusLine.contains("201")) - assertTrue(delegate.processFileCalled) - assertTrue(delegate.uploadFileCalled) - assertEquals("image/jpeg", delegate.lastMimeType) - assertEquals("photo.jpg", delegate.lastFilename) + // The delegate only transforms; GutenbergKit performs the upload. + assertTrue(client.uploadCalled) // The server relays WordPress's raw response body verbatim. val json = JsonParser.parseString(response.body).asJsonObject - assertEquals(42, json.get("id").asInt) - assertEquals("https://example.com/photo.jpg", json.get("source_url").asString) - assertEquals("image", json.get("media_type").asString) + assertEquals(99, json.get("id").asInt) + assertEquals("https://example.com/doc.pdf", json.get("source_url").asString) + assertEquals("file", json.get("media_type").asString) } @Test @@ -896,26 +894,6 @@ class MediaUploadServerTest { // MARK: - Mocks - private class MockUploadDelegate : MediaUploadDelegate { - @Volatile var processFileCalled = false - @Volatile var uploadFileCalled = false - @Volatile var lastMimeType: String? = null - @Volatile var lastFilename: String? = null - - override suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile { - processFileCalled = true - lastMimeType = mimeType - return ProcessedProxyFile.Original - } - - @Suppress("OVERRIDE_DEPRECATION") - override suspend fun uploadFile(file: File, mimeType: String, filename: String): MediaUploadResponse? { - uploadFileCalled = true - lastFilename = filename - val json = """{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}""" - return MediaUploadResponse(201, json.toByteArray()) - } - } private class ProcessOnlyDelegate : MediaUploadDelegate { @Volatile var processFileCalled = false diff --git a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt index 572836e4c..dcad5644e 100644 --- a/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt +++ b/android/app/src/main/java/com/example/gutenbergkit/DemoMediaUploadDelegate.kt @@ -13,7 +13,7 @@ import java.io.IOException /** * Demo media upload delegate that resizes images to a maximum dimension of 2000px. * - * Only overrides [processFile] — [uploadFile] returns null so the default uploader is used. + * Only transforms the file; GutenbergKit performs the upload. */ class DemoMediaUploadDelegate : MediaUploadDelegate { companion object { diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index b804090d1..4be8eb1cc 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -149,8 +149,8 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// fixed identifier and must survive app relaunch, an offline queue spans sessions. /// Build the uploader once, hold it, and pass the same instance to each editor. /// - /// Takes precedence over the deprecated ``MediaUploadDelegate/uploadFile(at:mimeType:filename:)``: - /// with an uploader set, that hook is never called. + /// A ``mediaUploadDelegate`` can still transform the file first; only delivery + /// moves to the uploader. public private(set) var mediaUploader: (any MediaUploader)? // MARK: - Private Properties (Services) diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index a5bfb7d5b..64ccbaa59 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -7,13 +7,13 @@ import Foundation /// or WordPress REST error object (on failure) it would get from a direct /// upload, so every consumer — image sub-sizes, attachment links, error notices — /// behaves identically to a non-native upload. -public struct MediaUploadResponse: Sendable { +struct MediaUploadResponse: Sendable { /// The HTTP status code WordPress (or the host's upload service) returned. - public let statusCode: Int + let statusCode: Int /// The raw response body — a WordPress REST attachment on success, or a /// WordPress REST error object (`{ "code", "message", "data" }`) on failure. - public let body: Data + let body: Data /// The response headers to relay to the editor. /// @@ -22,9 +22,9 @@ public struct MediaUploadResponse: Sendable { /// metadata generation fataled, and the editor's api-fetch middleware reads /// it to retry `post-process` and clean up the orphan. Dropping it turns a /// recoverable upload into a permanent failure. - public let headers: [String: String] + let headers: [String: String] - public init(statusCode: Int, body: Data, headers: [String: String] = [:]) { + init(statusCode: Int, body: Data, headers: [String: String] = [:]) { self.statusCode = statusCode self.body = body self.headers = headers @@ -44,23 +44,27 @@ public enum ProcessedProxyFile: Sendable { case processed(URL, mimeType: String, filename: String) } -/// Protocol for customizing media upload behavior. +/// Transforms media before GutenbergKit delivers it. /// -/// The native host app can provide an implementation to resize images, -/// transcode video, or use its own upload service. Default implementations -/// pass files through unchanged and upload via the WordPress REST API. +/// A delegate only changes *bytes* — GutenbergKit still uploads the result to the +/// configured site and owns the whole lifecycle (retries, cleanup). Because it never +/// performs the upload itself, it cannot deliver media to the wrong place. Set +/// ``EditorViewController/mediaUploadDelegate`` to resize images, transcode video, +/// strip EXIF, etc. +/// +/// This is the safe, common extension point: most hosts want only this. To perform +/// the upload yourself, conform to ``MediaUploader`` instead. public protocol MediaUploadDelegate: AnyObject, Sendable { - /// Whether this delegate might handle a file with the given metadata — either - /// processing it (``processFile(at:mimeType:filename:)``) or uploading it - /// itself (``uploadFile(at:mimeType:filename:)``). + /// Whether this delegate might transform a file with the given metadata. /// /// A cheap, metadata-only gate the server consults *before* materializing the /// upload to a temp file. Return `false` to decline a file by type — e.g. an /// image-only delegate returning `false` for a video — so the server forwards /// the original upload to WordPress without first copying a file the delegate - /// won't touch. Because it gates the temp-file copy needed by *both* - /// `processFile` and `uploadFile`, return `true` for any file the delegate - /// will either process or upload itself. + /// won't touch. + /// + /// Only consulted when no ``MediaUploader`` is set: an uploader takes over + /// delivery for every file, so there is no passthrough to decline to. /// /// Defaults to `true`: every file is materialized and the full pipeline runs. /// A `true` here is not a commitment — `processFile` may still return @@ -74,23 +78,6 @@ public protocol MediaUploadDelegate: AnyObject, Sendable { /// file and its metadata. When the format changes, report the new mimeType /// and filename so WordPress stores it with the correct extension and type. func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile - - /// Upload a processed file to the remote WordPress site. - /// - /// Return the raw WordPress response (status code + body), which GutenbergKit - /// relays to the editor unchanged, or `nil` to use the internal media client. A - /// host that uploads to WordPress should return the exact response it - /// received so the editor sees a complete attachment object. - /// - /// - Warning: Returning a raw response splits one upload's HTTP across two - /// owners — you perform the `POST`, but GutenbergKit's editor drives the - /// `post-process` retries and orphan cleanup behind it, through the WebView - /// rather than your stack. It also receives no form fields, so an attachment - /// uploaded this way lands unattached to its post. Conform to ``MediaUploader`` - /// instead: it owns the upload end-to-end and receives a ``MediaUpload`` - /// carrying the fields. - @available(*, deprecated, message: "Conform to MediaUploader instead — it owns the upload's retries and receives the editor's form fields.") - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? } /// Default implementations. @@ -102,11 +89,6 @@ extension MediaUploadDelegate { public func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { .original } - - @available(*, deprecated, message: "Conform to MediaUploader instead — it owns the upload's retries and receives the editor's form fields.") - public func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { - nil - } } /// One of the editor's non-file form fields, as sent with a media upload. diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 634492f3f..9e0a53335 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -342,7 +342,7 @@ final class MediaUploadServer: Sendable { /// The uploader, delegate, or internal media client completed the upload; /// carries the raw WordPress response to relay. case uploaded(MediaUploadResponse) - /// The delegate didn't modify the file and `uploadFile` returned nil. + /// The delegate didn't modify the file, so the original body is forwarded. /// The caller should forward the original request body to WordPress. case passthrough } @@ -411,12 +411,7 @@ final class MediaUploadServer: Sendable { return .uploaded(MediaUploadResponse(statusCode: 201, body: attachment)) } - // The deprecated delegate path: the host performs the POST but returns the raw - // response, leaving the editor to drive post-process recovery behind it. - if let delegate = context.uploadDelegate, - let result = try await deprecatedUploadFile(delegate, uploadURL, uploadMimeType, uploadFilename) { - return .uploaded(result) - } else if let internalClient = context.internalClient { + if let internalClient = context.internalClient { // Unmodified — forward the original request body directly, skipping // multipart re-encoding. if case .original = processed { @@ -441,17 +436,6 @@ final class MediaUploadServer: Sendable { return fields } - /// Calls the deprecated `uploadFile` hook from one place. - /// - /// This deliberately leaves one deprecation warning in GutenbergKit's own build: - /// the marker exists to tell *hosts* to migrate, and supporting the hook until it - /// is removed means calling it. The warning marks the code that goes with it. - private static func deprecatedUploadFile( - _ delegate: any MediaUploadDelegate, _ url: URL, _ mimeType: String, _ filename: String - ) async throws -> MediaUploadResponse? { - try await delegate.uploadFile(at: url, mimeType: mimeType, filename: filename) - } - private static func errorResponse(status: Int, message: String) -> HTTPResponse { // Emit a WordPress-REST-style error object so the JS middleware normalizes // it (and surfaces `message`) the same way it does a relayed WordPress diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index b9b02288a..5b42e67d3 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -157,10 +157,11 @@ struct MediaUploadServerTests { #expect(httpResponse.value(forHTTPHeaderField: "Content-Type") == "text/plain") } - @Test("calls delegate and returns upload result") - func delegateProcessAndUpload() async throws { - let delegate = MockUploadDelegate() - let server = try await MediaUploadServer.start(uploadDelegate: delegate) + @Test("processes with the delegate, then delivers and relays verbatim") + func delegateProcessThenDeliver() async throws { + let delegate = ResizingDelegate() + let internalClient = MockInternalMediaClient() + let server = try await MediaUploadServer.start(uploadDelegate: delegate, internalClient: internalClient) defer { server.stop() } let boundary = UUID().uuidString @@ -178,17 +179,15 @@ struct MediaUploadServerTests { let httpResponse = try #require(response as? HTTPURLResponse) #expect(httpResponse.statusCode == 201) - #expect(delegate.processFileCalled) - #expect(delegate.uploadFileCalled) - #expect(delegate.lastMimeType == "image/jpeg") - #expect(delegate.lastFilename == "photo.jpg") + // The delegate only transforms; GutenbergKit performs the upload. + #expect(internalClient.uploadCalled) // The server relays WordPress's raw response body verbatim. let object = try JSONSerialization.jsonObject(with: data) let json = try #require(object as? [String: Any]) - #expect(json["id"] as? Int == 42) - #expect(json["source_url"] as? String == "https://example.com/photo.jpg") - #expect(json["media_type"] as? String == "image") + #expect(json["id"] as? Int == 99) + #expect(json["source_url"] as? String == "https://example.com/doc.pdf") + #expect(json["media_type"] as? String == "file") } @Test("uses passthrough when delegate does not modify file") @@ -452,8 +451,8 @@ struct MediaUploadServerTests { #expect(received.query == "?_embed=wp:featuredmedia") } - @Test("an uploader takes precedence over the deprecated uploadFile hook") - func uploaderWinsOverDeprecatedHook() async throws { + @Test("a delegate still processes the file an uploader delivers") + func delegateProcessesForUploader() async throws { let delegate = MockUploadDelegate() let uploader = RecordingUploader() let server = try await MediaUploadServer.start(uploadDelegate: delegate, uploader: uploader, internalClient: MockInternalMediaClient()) @@ -472,7 +471,6 @@ struct MediaUploadServerTests { // The delegate still processes; only delivery moves to the uploader. #expect(delegate.processFileCalled) - #expect(!delegate.uploadFileCalled) #expect(uploader.received != nil) } @@ -553,8 +551,8 @@ struct MediaUploadServerTests { @Test("still processes for a delegate the host has dropped its reference to") func processesForHostReleasedDelegate() async throws { - // The delegate is read at the admission gate and again at processFile and - // uploadFile, separated by a synchronous disk copy and an unbounded processFile. + // The delegate is read at the admission gate and again at processFile, separated + // by a synchronous disk copy and an unbounded processFile. // Held weakly, a host that dropped its reference changed the answer between // those reads: a file admitted for processing was forwarded unprocessed. The // host dropping it before the request is the same condition, deterministically. @@ -1019,14 +1017,10 @@ private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendabl private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false - private var _uploadFileCalled = false private var _lastMimeType: String? - private var _lastFilename: String? var processFileCalled: Bool { lock.withLock { _processFileCalled } } - var uploadFileCalled: Bool { lock.withLock { _uploadFileCalled } } var lastMimeType: String? { lock.withLock { _lastMimeType } } - var lastFilename: String? { lock.withLock { _lastFilename } } func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { lock.withLock { @@ -1035,15 +1029,6 @@ private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable } return .original } - - func uploadFile(at url: URL, mimeType: String, filename: String) async throws -> MediaUploadResponse? { - lock.withLock { - _uploadFileCalled = true - _lastFilename = filename - } - let json = #"{"id":42,"source_url":"https://example.com/photo.jpg","media_type":"image"}"# - return MediaUploadResponse(statusCode: 201, body: Data(json.utf8)) - } } private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { From 4db23a96fc5494f04bfda462249dd85978debc8c Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Wed, 9 Sep 2026 10:29:40 -0600 Subject: [PATCH 2/2] docs: correct the handlesFile contract and the delegate's stale upload docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups to 6bcc210b. 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) --- .../org/wordpress/gutenberg/GutenbergView.kt | 6 +++-- .../wordpress/gutenberg/MediaUploadServer.kt | 17 ++++++++------ .../gutenberg/MediaUploadServerTest.kt | 1 - .../Sources/EditorViewController.swift | 4 +++- .../Sources/Media/MediaUploadDelegate.swift | 8 ++++--- .../Sources/Media/MediaUploadServer.swift | 8 +++---- .../Media/MediaUploadServerTests.swift | 23 +++---------------- 7 files changed, 29 insertions(+), 38 deletions(-) diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt index ef5764d95..6a8bd1858 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/GutenbergView.kt @@ -113,8 +113,10 @@ class GutenbergView : FrameLayout { var requestInterceptor: GutenbergRequestInterceptor = DefaultGutenbergRequestInterceptor() /** - * Optional delegate for customizing media upload behavior (resize, transcode, - * custom upload). + * Optional delegate for transforming media before upload (resize, transcode, + * strip EXIF). + * + * To perform the upload yourself, set [mediaUploader] instead. * * Provide this **before the editor loads** — typically right after * construction (e.g. in the `AndroidView` factory). It is captured once, when diff --git a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt index 703ff8066..abe448e51 100644 --- a/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt +++ b/android/Gutenberg/src/main/java/org/wordpress/gutenberg/MediaUploadServer.kt @@ -34,7 +34,10 @@ import okio.source * behaves identically to a non-native upload. */ internal class MediaUploadResponse( - /** The HTTP status code WordPress (or the host's upload service) returned. */ + /** + * The HTTP status code WordPress returned, or 201 for an upload a + * [MediaUploader] delivered. + */ val statusCode: Int, /** * The raw response body — a WordPress REST attachment on success, or a @@ -91,8 +94,9 @@ interface MediaUploadDelegate { * the original upload to WordPress without first copying a file the delegate * won't touch. * - * Only consulted when no [MediaUploader] is set: an uploader takes over delivery - * for every file, so there is no passthrough to decline to. + * With a [MediaUploader] set this can't decline the upload itself — an uploader + * delivers every file, so there is no passthrough to fall to — but it still gates + * [processFile]: a declined file reaches the uploader unprocessed. * * Defaults to true: every file is materialized and the full pipeline runs. A * true here is not a commitment — [processFile] may still return @@ -109,7 +113,6 @@ interface MediaUploadDelegate { * stores it with the correct extension and type. */ suspend fun processFile(file: File, mimeType: String, filename: String): ProcessedProxyFile = ProcessedProxyFile.Original - } /** @@ -363,8 +366,8 @@ internal class MediaUploadServer( // Ask the delegate — from metadata alone — whether it will touch a file // like this. If not, forward the original upload to WordPress directly, - // skipping a full temp-file copy of a file the delegate won't process or - // upload (e.g. a video handed to an image-only delegate). + // skipping a full temp-file copy of a file the delegate won't process + // (e.g. a video handed to an image-only delegate). // An uploader takes over delivery for *every* file, so with one set there is no // passthrough to fall to and the gate can't decline the upload outright. It // still decides whether processFile runs, though — a declined file is handed to @@ -584,7 +587,7 @@ internal class MediaUploadServer( } val result = internalClient?.upload(targetFile, targetMimeType, targetFilename, extraParts, query) - ?: error("No upload delegate or internal media client configured") + ?: error("No media uploader or internal media client configured") return UploadResult.Uploaded(result) } finally { // The processed file (if the delegate produced a new one) is ours to diff --git a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt index e942939bc..a842c2bf4 100644 --- a/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt +++ b/android/Gutenberg/src/test/java/org/wordpress/gutenberg/MediaUploadServerTest.kt @@ -894,7 +894,6 @@ class MediaUploadServerTest { // MARK: - Mocks - private class ProcessOnlyDelegate : MediaUploadDelegate { @Volatile var processFileCalled = false diff --git a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift index 4be8eb1cc..952a740fc 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorViewController.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorViewController.swift @@ -104,7 +104,9 @@ public final class EditorViewController: UIViewController, GutenbergEditorContro /// Used by `EditorViewController.warmup()` to reduce first-render latency. private let isWarmupMode: Bool - /// Customizes media file processing and upload behavior. + /// Delegate for transforming media before upload — resize, transcode, strip EXIF. + /// + /// To perform the upload yourself, pass a ``mediaUploader`` instead. /// /// Supplied at `init`, with the rest of the editor's configuration, because that is /// when it takes effect: the delegate is captured into the page's initial diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift index 64ccbaa59..f9aa87ec0 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadDelegate.swift @@ -8,7 +8,8 @@ import Foundation /// upload, so every consumer — image sub-sizes, attachment links, error notices — /// behaves identically to a non-native upload. struct MediaUploadResponse: Sendable { - /// The HTTP status code WordPress (or the host's upload service) returned. + /// The HTTP status code WordPress returned, or 201 for an upload a + /// ``MediaUploader`` delivered. let statusCode: Int /// The raw response body — a WordPress REST attachment on success, or a @@ -63,8 +64,9 @@ public protocol MediaUploadDelegate: AnyObject, Sendable { /// the original upload to WordPress without first copying a file the delegate /// won't touch. /// - /// Only consulted when no ``MediaUploader`` is set: an uploader takes over - /// delivery for every file, so there is no passthrough to decline to. + /// With a ``MediaUploader`` set this can't decline the upload itself — an + /// uploader delivers every file, so there is no passthrough to fall to — but it + /// still gates `processFile`: a declined file reaches the uploader unprocessed. /// /// Defaults to `true`: every file is materialized and the full pipeline runs. /// A `true` here is not a commitment — `processFile` may still return diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift index 9e0a53335..76b2fa0dc 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaUploadServer.swift @@ -29,10 +29,10 @@ final class MediaUploadServer: Sendable { /// Creates and starts a new upload server. /// /// - Parameters: - /// - uploadDelegate: Optional delegate for customizing file processing and upload. + /// - uploadDelegate: Optional delegate for transforming files before upload. /// - uploader: Optional host uploader that performs the upload on its own stack. /// - internalClient: GutenbergKit's own client for the configured site. Delivers - /// uploads when no host uploader or delegate does, and every media delete. + /// uploads when no host uploader does, and every media delete. /// - maxRequestBodySize: The maximum allowed request body size in bytes. /// Requests exceeding this limit receive a 413 response. Defaults to 4 GB. static func start( @@ -339,7 +339,7 @@ final class MediaUploadServer: Sendable { /// Result of the delegate processing + upload pipeline. private enum UploadResult { - /// The uploader, delegate, or internal media client completed the upload; + /// The uploader or internal media client completed the upload; /// carries the raw WordPress response to relay. case uploaded(MediaUploadResponse) /// The delegate didn't modify the file, so the original body is forwarded. @@ -549,7 +549,7 @@ enum UploadError: Error, LocalizedError { var errorDescription: String? { switch self { - case .noUploader: "No upload delegate or internal media client configured" + case .noUploader: "No media uploader or internal media client configured" case .streamReadFailed: "Failed to read upload stream" case .streamWriteFailed: "Failed to write upload to disk" } diff --git a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift index 5b42e67d3..3452098ab 100644 --- a/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift +++ b/ios/Tests/GutenbergKitTests/Media/MediaUploadServerTests.swift @@ -453,7 +453,7 @@ struct MediaUploadServerTests { @Test("a delegate still processes the file an uploader delivers") func delegateProcessesForUploader() async throws { - let delegate = MockUploadDelegate() + let delegate = ProcessOnlyDelegate() let uploader = RecordingUploader() let server = try await MediaUploadServer.start(uploadDelegate: delegate, uploader: uploader, internalClient: MockInternalMediaClient()) defer { server.stop() } @@ -529,9 +529,9 @@ struct MediaUploadServerTests { @Test("retains the delegate for the server's lifetime, and releases it after") func retainsDelegateForServerLifetime() async throws { - weak var weakDelegate: MockUploadDelegate? + weak var weakDelegate: ProcessOnlyDelegate? do { - let delegate = MockUploadDelegate() + let delegate = ProcessOnlyDelegate() weakDelegate = delegate let server = try await MediaUploadServer.start(uploadDelegate: delegate) defer { server.stop() } @@ -1014,23 +1014,6 @@ private final class TranscodingDelegate: MediaUploadDelegate, @unchecked Sendabl } } -private final class MockUploadDelegate: MediaUploadDelegate, @unchecked Sendable { - private let lock = NSLock() - private var _processFileCalled = false - private var _lastMimeType: String? - - var processFileCalled: Bool { lock.withLock { _processFileCalled } } - var lastMimeType: String? { lock.withLock { _lastMimeType } } - - func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { - lock.withLock { - _processFileCalled = true - _lastMimeType = mimeType - } - return .original - } -} - private final class ProcessOnlyDelegate: MediaUploadDelegate, @unchecked Sendable { private let lock = NSLock() private var _processFileCalled = false