Conversation
6 tasks done
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/631")Built from 98a51be |
jkmassel
force-pushed
the
refactor/media-upload-handler-object
branch
from
September 8, 2026 16:12
5783bdb to
45e002d
Compare
jkmassel
force-pushed
the
refactor/media-upload-handler-object
branch
from
September 9, 2026 00:55
45e002d to
912d1cd
Compare
jkmassel
force-pushed
the
refactor/media-upload-handler-object
branch
from
September 9, 2026 19:47
912d1cd to
51aed19
Compare
8 of 10 tasks
jkmassel
force-pushed
the
refactor/media-upload-handler-object
branch
from
September 15, 2026 22:39
51aed19 to
12431aa
Compare
jkmassel
force-pushed
the
refactor/media-upload-handler-object
branch
from
September 16, 2026 19:41
12431aa to
fa3a2de
Compare
jkmassel
force-pushed
the
refactor/media-upload-handler-object
branch
3 times, most recently
from
September 17, 2026 16:29
e9371b9 to
fe6fc51
Compare
`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
force-pushed
the
refactor/media-upload-handler-object
branch
from
September 17, 2026 16:34
fe6fc51 to
98a51be
Compare
jkmassel
marked this pull request as ready for review
September 17, 2026 16:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
HTTPRequestHandlerinGutenbergKitHTTPAn
HTTPServer.startoverload 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.
MediaUploadServerserves from oneThe statics threading an
UploadContextbecome aHandlerstruct with stored properties.Why?
The closure form of
startcan't capture the object that owns the server: the closure has to exist before the server does, and retrofittingselfwould formMediaUploadServer → HTTPServer → handler → MediaUploadServer, so the owner'sdeinit— and itsstop()— would never run. A consumer with dependencies to hold therefore ends up with static functions threading a context parameter through every call, which is howMediaUploadServeris written today.HTTPRequestHandleris deliberately notAnyObject-constrained so a handler can be astructholding only what it needs — not because astructis safe by construction. A value type is not protection: the server captures the handler into a heap node, so astructstoring the server's owner closesowner → HTTPServer → handler → ownerexactly as a class would. Both shapes work, under the same leaf disciplineHTTPServerDelegatealready documents — a handler must not strongly hold the object that owns the server.Handlerstores no reference back to theMediaUploadServer, which is why the helpers outside it stay static.Request handling is mandatory, so it can't be a defaulted
HTTPServerDelegatemethod the way optional customization points are — hence an overload rather than a new delegate requirement.How?
handleRequestbecomeshandle; the functions that use the dependencies become instance methods, and the ones that don't (attachmentId,relayResponse,uploadErrorResponse,formFields) stay static.UploadContextgoes away —Handleris 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, unchangedHTTPServeroverloadxcodebuild