-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: process New Block Editor media uploads with app Media settings #25824
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
dcalhoun
wants to merge
8
commits into
trunk
Choose a base branch
from
feat/process-gutenberg-kit-media-uploads
base: trunk
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+313
−5
Open
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
ea1ad1f
feat: add gbkOptimizeMediaUploads feature flag
dcalhoun 80fdab4
build: use GutenbergKit pr-build/357 snapshot
dcalhoun 2d6bdf1
feat: process GutenbergKit media uploads with app Media settings
dcalhoun cf46c70
test: cover GBKMediaUploadProcessor media processing
dcalhoun f373395
feat: surface media upload optimization in Experimental Features
dcalhoun 347f95b
refactor: set media upload delegate unconditionally
dcalhoun be8615d
Revert "feat: surface media upload optimization in Experimental Featu…
dcalhoun c908b31
Revert "feat: add gbkOptimizeMediaUploads feature flag"
dcalhoun File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
174 changes: 174 additions & 0 deletions
174
Tests/KeystoneTests/Tests/Features/Media/GBKMediaUploadProcessorTests.swift
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| import Foundation | ||
| import ImageIO | ||
| import Testing | ||
| import UniformTypeIdentifiers | ||
| import WordPressShared | ||
|
|
||
| @testable import WordPress | ||
|
|
||
| struct GBKMediaUploadProcessorTests { | ||
|
|
||
| // MARK: - Images | ||
|
|
||
| @Test func imageIsResizedWhenOptimizationEnabled() async throws { | ||
| let settings = makeSettings() | ||
| settings.imageOptimizationEnabled = true | ||
| settings.maxImageSizeSetting = 200 | ||
| let processor = makeProcessor(settings: settings) | ||
| let url = try fixtureURL("test-image-device-photo-gps.jpg") | ||
|
|
||
| let result = try await processor.processFile(at: url, mimeType: "image/jpeg", filename: url.lastPathComponent) | ||
|
|
||
| guard case .processed(let outputURL, let mimeType, let filename) = result else { | ||
| Issue.record("Expected a processed file") | ||
| return | ||
| } | ||
| defer { cleanUp(outputURL) } | ||
| let size = try imageSize(at: outputURL) | ||
| #expect(max(size.width, size.height) == 200) | ||
| #expect(mimeType == "image/jpeg") | ||
| #expect(filename.hasPrefix("test-image-device-photo-gps")) | ||
| } | ||
|
|
||
| @Test func imageIsUntouchedWhenProcessingWouldBeNoOp() async throws { | ||
| let settings = makeSettings() | ||
| settings.imageOptimizationEnabled = false | ||
| settings.removeLocationSetting = false | ||
| let processor = makeProcessor(settings: settings) | ||
| let url = try fixtureURL("test-image-device-photo-gps.jpg") | ||
|
|
||
| let result = try await processor.processFile(at: url, mimeType: "image/jpeg", filename: url.lastPathComponent) | ||
|
|
||
| guard case .original = result else { | ||
| Issue.record("Expected the original file to pass through") | ||
| return | ||
| } | ||
| } | ||
|
|
||
| @Test func gpsDataIsStrippedWhenRemoveLocationEnabled() async throws { | ||
| let settings = makeSettings() | ||
| settings.imageOptimizationEnabled = false | ||
| settings.removeLocationSetting = true | ||
| let processor = makeProcessor(settings: settings) | ||
| let url = try fixtureURL("test-image-device-photo-gps.jpg") | ||
|
|
||
| let result = try await processor.processFile(at: url, mimeType: "image/jpeg", filename: url.lastPathComponent) | ||
|
|
||
| guard case .processed(let outputURL, _, _) = result else { | ||
| Issue.record("Expected a processed file") | ||
| return | ||
| } | ||
| defer { cleanUp(outputURL) } | ||
| #expect(try imageProperties(at: url)[kCGImagePropertyGPSDictionary] != nil) | ||
| #expect(try imageProperties(at: outputURL)[kCGImagePropertyGPSDictionary] == nil) | ||
| } | ||
|
|
||
| @Test func heicIsConvertedToJPEG() async throws { | ||
| let settings = makeSettings() | ||
| settings.imageOptimizationEnabled = false | ||
| settings.removeLocationSetting = false | ||
| let processor = makeProcessor(settings: settings) | ||
| let url = try fixtureURL("iphone-photo.heic") | ||
|
|
||
| let result = try await processor.processFile(at: url, mimeType: "image/heic", filename: url.lastPathComponent) | ||
|
|
||
| guard case .processed(let outputURL, let mimeType, let filename) = result else { | ||
| Issue.record("Expected a processed file") | ||
| return | ||
| } | ||
| defer { cleanUp(outputURL) } | ||
| #expect(mimeType == "image/jpeg") | ||
| #expect(filename.hasSuffix(".jpg") || filename.hasSuffix(".jpeg")) | ||
| } | ||
|
|
||
| // MARK: - GIFs and other files | ||
|
|
||
| @Test func gifPassesThroughUntouched() async throws { | ||
| let processor = makeProcessor(settings: makeSettings()) | ||
| let url = try fixtureURL("test-gif.gif") | ||
|
|
||
| let result = try await processor.processFile(at: url, mimeType: "image/gif", filename: url.lastPathComponent) | ||
|
|
||
| guard case .original = result else { | ||
| Issue.record("Expected the original file to pass through") | ||
| return | ||
| } | ||
| } | ||
|
|
||
| @Test func disallowedFileExtensionThrows() async throws { | ||
| let processor = GBKMediaUploadProcessor( | ||
| videoDurationLimit: nil, | ||
| allowableFileExtensions: ["pdf"], | ||
| makeMediaSettings: makeSettingsFactory(makeSettings()) | ||
| ) | ||
| let url = FileManager.default.temporaryDirectory.appendingPathComponent("\(UUID().uuidString).txt") | ||
| try "plain text".write(to: url, atomically: true, encoding: .utf8) | ||
| defer { cleanUp(url) } | ||
|
|
||
| await #expect(throws: MediaURLExporter.URLExportError.self) { | ||
| try await processor.processFile(at: url, mimeType: "text/plain", filename: url.lastPathComponent) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Videos | ||
|
|
||
| @Test func videoExceedingDurationLimitThrows() async throws { | ||
| let processor = GBKMediaUploadProcessor( | ||
| videoDurationLimit: 1, | ||
| allowableFileExtensions: [], | ||
| makeMediaSettings: makeSettingsFactory(makeSettings()) | ||
| ) | ||
| let url = try fixtureURL("test-video-device-gps.m4v") | ||
|
|
||
| await #expect(throws: (any Error).self) { | ||
| try await processor.processFile(at: url, mimeType: "video/mp4", filename: url.lastPathComponent) | ||
| } | ||
| } | ||
|
|
||
| // MARK: - Helpers | ||
|
|
||
| private func makeProcessor(settings: MediaSettings) -> GBKMediaUploadProcessor { | ||
| GBKMediaUploadProcessor( | ||
| videoDurationLimit: nil, | ||
| allowableFileExtensions: [], | ||
| makeMediaSettings: makeSettingsFactory(settings) | ||
| ) | ||
| } | ||
|
|
||
| private func makeSettings() -> MediaSettings { | ||
| MediaSettings(database: EphemeralKeyValueDatabase()) | ||
| } | ||
|
|
||
| private func makeSettingsFactory(_ settings: MediaSettings) -> @Sendable () -> MediaSettings { | ||
| nonisolated(unsafe) let settings = settings | ||
| return { settings } | ||
| } | ||
|
|
||
| private func fixtureURL(_ filename: String) throws -> URL { | ||
| let bundle = Bundle(for: BundleAnchor.self) | ||
| let name = (filename as NSString).deletingPathExtension | ||
| let ext = (filename as NSString).pathExtension | ||
| let url = try #require(bundle.url(forResource: name, withExtension: ext)) | ||
| return url | ||
| } | ||
|
|
||
| private func imageProperties(at url: URL) throws -> [CFString: Any] { | ||
| let source = try #require(CGImageSourceCreateWithURL(url as CFURL, nil)) | ||
| let properties = try #require(CGImageSourceCopyPropertiesAtIndex(source, 0, nil) as? [CFString: Any]) | ||
| return properties | ||
| } | ||
|
|
||
| private func imageSize(at url: URL) throws -> CGSize { | ||
| let properties = try imageProperties(at: url) | ||
| let width = try #require(properties[kCGImagePropertyPixelWidth] as? CGFloat) | ||
| let height = try #require(properties[kCGImagePropertyPixelHeight] as? CGFloat) | ||
| return CGSize(width: width, height: height) | ||
| } | ||
|
|
||
| private func cleanUp(_ url: URL) { | ||
| try? FileManager.default.removeItem(at: url) | ||
| } | ||
| } | ||
|
|
||
| /// Anchor for resolving the test bundle from Swift Testing suites. | ||
| private final class BundleAnchor {} |
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
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
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
129 changes: 129 additions & 0 deletions
129
WordPress/Classes/ViewRelated/NewGutenberg/GBKMediaUploadProcessor.swift
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| import Foundation | ||
| import GutenbergKit | ||
| import UniformTypeIdentifiers | ||
| import WordPressData | ||
|
|
||
| /// Processes media files picked in the GutenbergKit editor before upload, | ||
| /// applying the app's Media settings (image optimization, max upload size, | ||
| /// image quality, video resolution, and location stripping). | ||
| /// | ||
| /// Assigned to `GutenbergKit.EditorViewController.mediaUploadDelegate`, which | ||
| /// holds it weakly and invokes it off the main actor, so the type is `Sendable` | ||
| /// and snapshots the `Blog`-derived values it needs at initialization. | ||
| final class GBKMediaUploadProcessor: MediaUploadDelegate, Sendable { | ||
| private let videoDurationLimit: TimeInterval? | ||
| private let allowableFileExtensions: Set<String> | ||
| private let makeMediaSettings: @Sendable () -> MediaSettings | ||
|
|
||
| /// Image types the WordPress REST API reliably accepts. Other image | ||
| /// formats (e.g. HEIC) are converted to JPEG during processing, mirroring | ||
| /// `ItemProviderMediaExporter`. | ||
| private static let webSafeImageTypes: Set<UTType> = [.png, .jpeg, .gif, .svg] | ||
|
|
||
| @MainActor | ||
| convenience init(blog: Blog) { | ||
| // HEIC isn't supported when uploading an image, so we filter it out, | ||
| // mirroring `MediaImportService`. | ||
| var allowedFileTypes = blog.allowedFileTypes | ||
| allowedFileTypes.remove("heic") | ||
|
|
||
| self.init( | ||
| videoDurationLimit: blog.videoDurationLimit, | ||
| allowableFileExtensions: allowedFileTypes | ||
| ) | ||
| } | ||
|
|
||
| init( | ||
| videoDurationLimit: TimeInterval?, | ||
| allowableFileExtensions: Set<String>, | ||
| makeMediaSettings: @escaping @Sendable () -> MediaSettings = { MediaSettings() } | ||
| ) { | ||
| self.videoDurationLimit = videoDurationLimit | ||
| self.allowableFileExtensions = allowableFileExtensions | ||
| self.makeMediaSettings = makeMediaSettings | ||
| } | ||
|
|
||
| // MARK: - MediaUploadDelegate | ||
|
|
||
| func processFile(at url: URL, mimeType: String, filename: String) async throws -> ProcessedProxyFile { | ||
| let expected = try MediaURLExporter.expectedExport(with: url) | ||
| let settings = makeMediaSettings() | ||
|
|
||
| switch expected { | ||
| case .gif: | ||
| // GIFs are uploaded unchanged; processing would only copy the file. | ||
| return .original | ||
| case .other: | ||
| // Non-media files are uploaded unchanged, but enforce the site's | ||
| // allowed file extensions, mirroring `MediaURLExporter.exportURL`. | ||
| if let fileExtension = url.typeIdentifierFileExtension, | ||
| !MediaImportService.defaultAllowableFileExtensions.contains(fileExtension), | ||
| !allowableFileExtensions.isEmpty, | ||
| !allowableFileExtensions.contains(fileExtension) | ||
| { | ||
| throw MediaURLExporter.URLExportError.unsupportedFileType | ||
| } | ||
| return .original | ||
| case .image: | ||
| // Skip processing when it would be a no-op: optimization and | ||
| // location stripping disabled, and the format is web-safe. | ||
| if !settings.imageOptimizationEnabled, | ||
| !settings.removeLocationSetting, | ||
| let type = url.typeIdentifier.flatMap(UTType.init), | ||
| Self.webSafeImageTypes.contains(type) | ||
| { | ||
| return .original | ||
| } | ||
| case .video: | ||
| // Always process video to apply the export preset, duration | ||
| // limit, and location stripping. | ||
| break | ||
| } | ||
|
|
||
| let export = try await makeExporter(for: url, settings: settings).export() | ||
|
|
||
| guard let mimeType = export.url.typeIdentifier.flatMap(UTType.init)?.preferredMIMEType else { | ||
| throw MediaURLExporter.URLExportError.unknownFileUTI | ||
| } | ||
| return .processed(export.url, mimeType: mimeType, filename: export.url.lastPathComponent) | ||
| } | ||
|
|
||
| // MARK: - Exporter configuration | ||
|
|
||
| /// Builds an exporter configured from the app's Media settings, mirroring | ||
| /// the option mapping in `MediaImportService`. | ||
| private func makeExporter(for url: URL, settings: MediaSettings) -> MediaURLExporter { | ||
| let exporter = MediaURLExporter(url: url) | ||
| // GutenbergKit deletes the processed file after uploading it, so the | ||
| // export is written to a temporary directory rather than the uploads | ||
| // directory tracked by `MediaFileManager`. | ||
| exporter.mediaDirectoryType = .temporary | ||
|
|
||
| var imageOptions = MediaImageExporter.Options() | ||
| imageOptions.maximumImageSize = maximumImageSize(from: settings) | ||
| imageOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting | ||
| imageOptions.imageCompressionQuality = settings.imageQualityForUpload.doubleValue | ||
| if let type = url.typeIdentifier.flatMap(UTType.init), !Self.webSafeImageTypes.contains(type) { | ||
| imageOptions.exportImageType = UTType.jpeg.identifier | ||
| } | ||
| exporter.imageOptions = imageOptions | ||
|
|
||
| var videoOptions = MediaVideoExporter.Options() | ||
| videoOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting | ||
| videoOptions.exportPreset = settings.maxVideoSizeSetting.videoPreset | ||
| videoOptions.durationLimit = videoDurationLimit | ||
| exporter.videoOptions = videoOptions | ||
|
|
||
| var urlOptions = MediaURLExporter.Options() | ||
| urlOptions.allowableFileExtensions = allowableFileExtensions | ||
| urlOptions.stripsGeoLocationIfNeeded = settings.removeLocationSetting | ||
| exporter.urlOptions = urlOptions | ||
|
|
||
| return exporter | ||
| } | ||
|
|
||
| private func maximumImageSize(from settings: MediaSettings) -> CGFloat? { | ||
| let maxUploadSize = settings.imageSizeForUpload | ||
| return maxUploadSize < Int.max ? CGFloat(maxUploadSize) : nil | ||
| } | ||
| } |
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.