Skip to content

feat(ios): add HTTPRequestHandler, and serve media uploads from one - #631

Closed
jkmassel wants to merge 1 commit into
refactor/media-processor-renamefrom
refactor/media-upload-handler-object
Closed

jkmassel wants to merge 1 commit into
refactor/media-processor-renamefrom
refactor/media-upload-handler-object

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Stacked on #630. Eighth of ten PRs splitting #621. No behavior change — the review is "did any body change?"

What?

One commit, two parts:

1. HTTPRequestHandler in GutenbergKitHTTP

An HTTPServer.start overload taking a handler object rather than a closure. Purely additive: the closure overload is unchanged and forwards to the same code path, so no existing caller, test, or the debug server is affected.

2. MediaUploadServer serves from one

The statics threading an UploadContext become a Handler struct with stored properties.

Why?

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 MediaUploadServer → HTTPServer → handler → MediaUploadServer, 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.

HTTPRequestHandler is deliberately not AnyObject-constrained so a handler can be a struct holding only what it needs — not because a struct is safe by construction. A value type is not protection: the server captures the handler into a heap node, so a struct storing the server's owner closes owner → HTTPServer → handler → owner exactly as a class would. Both shapes work, under the same leaf discipline HTTPServerDelegate already documents — a handler must not strongly hold the object that owns the server. Handler stores no reference back to the MediaUploadServer, which is why the helpers outside it stay static.

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.

How?

handleRequest becomes handle; the functions that use the dependencies become instance methods, and the ones that don't (attachmentId, relayResponse, uploadErrorResponse, formFields) stay static. UploadContext goes away — Handler is what it was. Helpers outside the handler (errorResponse, writeStream, sanitizeFilename, uploadsTempDirectory) are qualified rather than moved.

The restructure is confined to one source file — MediaUploadServer.swift, 296/299 lines of it pure churn — plus a one-line comment in its test naming the retain loop. Read that file as "did any body change?"; the API design is the other four files.

Testing Instructions

  • swift test — host suite green, unchanged
  • One new test for the HTTPServer overload
  • iOS Simulator xcodebuild
  • SwiftLint clean

@jkmassel jkmassel added [Type] Task Issues or PRs that have been broken down into an individual action to take iOS labels Sep 5, 2026
@jkmassel jkmassel self-assigned this Sep 5, 2026
@wpmobilebot

wpmobilebot commented Sep 5, 2026 •

Copy link
Copy Markdown

XCFramework Build

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

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

Built from 98a51be

@jkmassel
jkmassel force-pushed the refactor/media-upload-handler-object branch from 5783bdb to 45e002d Compare September 8, 2026 16:12
@jkmassel
jkmassel force-pushed the refactor/media-upload-handler-object branch from 45e002d to 912d1cd Compare September 9, 2026 00:55
@jkmassel
jkmassel force-pushed the refactor/media-upload-handler-object branch from 912d1cd to 51aed19 Compare September 9, 2026 19:47
@jkmassel
jkmassel force-pushed the refactor/media-upload-handler-object branch from 51aed19 to 12431aa Compare September 15, 2026 22:39
@jkmassel
jkmassel force-pushed the refactor/media-upload-handler-object branch from 12431aa to fa3a2de Compare September 16, 2026 19:41
@jkmassel
jkmassel force-pushed the refactor/media-upload-handler-object branch 3 times, most recently from e9371b9 to fe6fc51 Compare September 17, 2026 16:29
`MediaUploadServer` handled requests through static functions threading an
`UploadContext` parameter through every call, because the closure form of
`HTTPServer.start` can't capture the object that owns the server: the
closure has to exist before the server does, and capturing `self` would
form `MediaUploadServer -> HTTPServer -> handler -> MediaUploadServer`, so
`deinit` — and its `stop()` — would never run.

Add an `HTTPRequestHandler` protocol to `GutenbergKitHTTP` and a `start`
overload that takes one, then serve `MediaUploadServer` from it. The
dependencies become stored properties on a `Handler` struct and the request
logic becomes instance methods. The closure overload is unchanged and
forwards to the same code path, so the addition 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.

The protocol is deliberately not `AnyObject`-constrained so a handler *can*
be a struct holding only what it needs — not because a struct is safe by
construction. A value type is not protection: the server captures the
handler into a heap node, so a struct storing the server's owner closes the
same ring a class would. Both shapes work, under the same leaf discipline
`HTTPServerDelegate` already documents — a handler must not strongly hold
the object that owns the server. `Handler` stores no reference back to the
`MediaUploadServer`, which is why the helpers outside it stay static.

Mechanically: `handleRequest` becomes `handle`, the functions that use the
dependencies become instance methods, and the ones that don't
(`attachmentId`, `relayResponse`, `uploadErrorResponse`, `formFields`) stay
static. `UploadContext` goes away — `Handler` is what it was. Helpers
outside the handler (`errorResponse`, `writeStream`, `sanitizeFilename`,
`uploadsTempDirectory`) are qualified rather than moved.

No behavior change.
@jkmassel
jkmassel force-pushed the refactor/media-upload-handler-object branch from fe6fc51 to 98a51be Compare September 17, 2026 16:34
@jkmassel jkmassel changed the title refactor(ios): serve media uploads from a handler object, not statics feat(ios): add HTTPRequestHandler, and serve media uploads from one Sep 17, 2026
@jkmassel jkmassel added the [Type] Enhancement A suggestion for improvement. label Sep 17, 2026
@jkmassel jkmassel removed the [Type] Task Issues or PRs that have been broken down into an individual action to take label Sep 17, 2026
@jkmassel
jkmassel marked this pull request as ready for review September 17, 2026 16:46
@jkmassel jkmassel closed this Sep 17, 2026
@jkmassel
jkmassel deleted the refactor/media-upload-handler-object branch October 1, 2026 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

iOS [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants