From a113d6f9adbc5f22a68e350fe30135596cef6a15 Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Wed, 25 Feb 2026 11:14:34 +0700 Subject: [PATCH 01/13] Remove sideload upload serialization The per-attachment serial constraint on thumbnail sideloads added unnecessary latency during bulk uploads. The existing maxConcurrentUploads limit already bounds total concurrency, and image processing naturally staggers upload start times. --- .../upload-media/src/store/private-actions.ts | 62 ------- .../src/store/private-selectors.ts | 39 ---- .../upload-media/src/store/test/actions.ts | 175 ++++++++++++++++++ .../upload-media/src/store/test/selectors.ts | 12 ++ 4 files changed, 187 insertions(+), 101 deletions(-) diff --git a/packages/upload-media/src/store/private-actions.ts b/packages/upload-media/src/store/private-actions.ts index b06af25f2d6d43..1099b2fb35a03a 100644 --- a/packages/upload-media/src/store/private-actions.ts +++ b/packages/upload-media/src/store/private-actions.ts @@ -43,7 +43,6 @@ import type { PauseQueueAction, QueueItem, QueueItemId, - ResumeItemAction, ResumeQueueAction, RevokeBlobUrlsAction, SideloadAdditionalData, @@ -63,7 +62,6 @@ type ActionCreators = { addSideloadItem: typeof addSideloadItem; removeItem: typeof removeItem; pauseItem: typeof pauseItem; - resumeItemByPostId: typeof resumeItemByPostId; prepareItem: typeof prepareItem; processItem: typeof processItem; finishOperation: typeof finishOperation; @@ -94,32 +92,6 @@ type ThunkArgs = { registry: WPDataRegistry; }; -/** - * Determines if an upload should be paused to avoid race conditions. - * - * When sideloading thumbnails, we need to pause uploads if another - * upload to the same post is already in progress. - * - * @param item Queue item to check. - * @param operation Current operation type. - * @param select Store selectors. - * @return Whether the upload should be paused. - */ -function shouldPauseForSideload( - item: QueueItem, - operation: OperationType | undefined, - select: Selectors -): boolean { - if ( - operation !== OperationType.Upload || - ! item.parentId || - ! item.additionalData.post - ) { - return false; - } - return select.isUploadingToPost( item.additionalData.post as number ); -} - interface AddItemArgs { // It should always be a File, but some consumers might still pass Blobs only. file: File | Blob; @@ -306,16 +278,6 @@ export function processItem( id: QueueItemId ) { ? item.operations[ 0 ][ 1 ] : undefined; - // If we're sideloading a thumbnail, pause upload to avoid race conditions. - // It will be resumed after the previous upload finishes. - if ( shouldPauseForSideload( item, operation, select ) ) { - dispatch< PauseItemAction >( { - type: Type.PauseItem, - id, - } ); - return; - } - /* * If the next operation is an upload, check concurrency limit. * If at capacity, the item remains queued and will be processed @@ -516,28 +478,6 @@ export function pauseItem( id: QueueItemId ) { }; } -/** - * Resumes processing for a given post/attachment ID. - * - * This function looks up paused uploads by post ID and resumes them. - * It's typically called after a sideload completes to resume paused - * thumbnail uploads. - * - * @param postOrAttachmentId Post or attachment ID. - */ -export function resumeItemByPostId( postOrAttachmentId: number ) { - return async ( { select, dispatch }: ThunkArgs ) => { - const item = select.getPausedUploadForPost( postOrAttachmentId ); - if ( item ) { - dispatch< ResumeItemAction >( { - type: Type.ResumeItem, - id: item.id, - } ); - dispatch.processItem( item.id ); - } - }; -} - /** * Removes a specific item from the queue. * @@ -850,11 +790,9 @@ export function sideloadItem( id: QueueItemId ) { signal: item.abortController?.signal, onFileChange: ( [ attachment ] ) => { dispatch.finishOperation( id, { attachment } ); - dispatch.resumeItemByPostId( post as number ); }, onError: ( error ) => { dispatch.cancelItem( id, error ); - dispatch.resumeItemByPostId( post as number ); }, } ); }; diff --git a/packages/upload-media/src/store/private-selectors.ts b/packages/upload-media/src/store/private-selectors.ts index 20a91bab629ac9..e62e599cdd23e1 100644 --- a/packages/upload-media/src/store/private-selectors.ts +++ b/packages/upload-media/src/store/private-selectors.ts @@ -3,7 +3,6 @@ */ import { type BatchId, - ItemStatus, OperationType, type QueueItem, type QueueItemId, @@ -51,44 +50,6 @@ export function isBatchUploaded( state: State, batchId: BatchId ): boolean { return batchItems.length === 0; } -/** - * Determines whether an upload is currently in progress given a post or attachment ID. - * - * @param state Upload state. - * @param postOrAttachmentId Post ID or attachment ID. - * - * @return Whether upload is currently in progress for the given post or attachment. - */ -export function isUploadingToPost( - state: State, - postOrAttachmentId: number -): boolean { - return state.queue.some( - ( item ) => - item.currentOperation === OperationType.Upload && - item.additionalData.post === postOrAttachmentId - ); -} - -/** - * Returns the next paused upload for a given post or attachment ID. - * - * @param state Upload state. - * @param postOrAttachmentId Post ID or attachment ID. - * - * @return Paused item. - */ -export function getPausedUploadForPost( - state: State, - postOrAttachmentId: number -): QueueItem | undefined { - return state.queue.find( - ( item ) => - item.status === ItemStatus.Paused && - item.additionalData.post === postOrAttachmentId - ); -} - /** * Determines whether uploading is currently paused. * diff --git a/packages/upload-media/src/store/test/actions.ts b/packages/upload-media/src/store/test/actions.ts index 20524bf85ef83f..ccef65b52df3cd 100644 --- a/packages/upload-media/src/store/test/actions.ts +++ b/packages/upload-media/src/store/test/actions.ts @@ -336,6 +336,181 @@ describe( 'actions', () => { } ); } ); + describe( 'concurrent sideloads', () => { + it( 'does not pause sideload items targeting the same post', async () => { + // Configure mediaSideload so sideload uploads can proceed. + unlock( registry.dispatch( uploadStore ) ).updateSettings( { + mediaSideload: jest.fn(), + } ); + + // Add a parent item first. + unlock( registry.dispatch( uploadStore ) ).addItem( { + file: jpegFile, + } ); + const parentItem = unlock( + registry.select( uploadStore ) + ).getAllItems()[ 0 ]; + + // Add two sideload items targeting the same post. + unlock( registry.dispatch( uploadStore ) ).addSideloadItem( { + file: jpegFile, + parentId: parentItem.id, + additionalData: { post: 100, image_size: 'thumbnail' }, + operations: [ OperationType.Upload ], + } ); + unlock( registry.dispatch( uploadStore ) ).addSideloadItem( { + file: jpegFile, + parentId: parentItem.id, + additionalData: { post: 100, image_size: 'medium' }, + operations: [ OperationType.Upload ], + } ); + + // Resume the queue to trigger processing. + await unlock( registry.dispatch( uploadStore ) ).resumeQueue(); + + const items = unlock( + registry.select( uploadStore ) + ).getAllItems(); + const sideloadItems = items.filter( + ( item ) => item.parentId === parentItem.id + ); + + // Neither sideload item should be paused. + for ( const item of sideloadItems ) { + expect( item.status ).not.toBe( ItemStatus.Paused ); + } + } ); + + it( 'allows multiple sideloads to the same attachment to upload concurrently', async () => { + const mediaSideload = jest.fn(); + + unlock( registry.dispatch( uploadStore ) ).updateSettings( { + mediaSideload, + maxConcurrentUploads: 5, + } ); + + // Add a parent item first. + unlock( registry.dispatch( uploadStore ) ).addItem( { + file: jpegFile, + } ); + const parentItem = unlock( + registry.select( uploadStore ) + ).getAllItems()[ 0 ]; + + // Add 3 sideload items to same post. + for ( const size of [ 'thumbnail', 'medium', 'large' ] ) { + unlock( registry.dispatch( uploadStore ) ).addSideloadItem( { + file: jpegFile, + parentId: parentItem.id, + additionalData: { post: 200, image_size: size }, + operations: [ OperationType.Upload ], + } ); + } + + // Resume the queue. + await unlock( registry.dispatch( uploadStore ) ).resumeQueue(); + + // All 3 sideloads should have started (not serialized). + expect( mediaSideload ).toHaveBeenCalledTimes( 3 ); + } ); + + it( 'respects maxConcurrentUploads for sideloads', async () => { + const mediaSideload = jest.fn(); + + unlock( registry.dispatch( uploadStore ) ).updateSettings( { + mediaSideload, + maxConcurrentUploads: 2, + } ); + + // Use a fake parentId so the parent item does not consume + // an upload slot. Only sideload items compete for slots. + const fakeParentId = 'fake-parent-id'; + + // Add 4 sideload items. + for ( const size of [ + 'thumbnail', + 'medium', + 'large', + 'medium_large', + ] ) { + unlock( registry.dispatch( uploadStore ) ).addSideloadItem( { + file: jpegFile, + parentId: fakeParentId, + additionalData: { post: 300, image_size: size }, + operations: [ OperationType.Upload ], + } ); + } + + // Resume the queue. + await unlock( registry.dispatch( uploadStore ) ).resumeQueue(); + + // Only 2 should have started due to concurrency limit. + expect( mediaSideload ).toHaveBeenCalledTimes( 2 ); + } ); + + it( 'starts pending sideloads after one finishes', async () => { + let onFileChangeCallback: + | ( ( + attachments: Array< Record< string, unknown > > + ) => void ) + | undefined; + const mediaSideload = jest.fn( ( { onFileChange } ) => { + // Capture the first callback to simulate completion later. + if ( ! onFileChangeCallback ) { + onFileChangeCallback = onFileChange; + } + } ); + + unlock( registry.dispatch( uploadStore ) ).updateSettings( { + mediaSideload, + maxConcurrentUploads: 1, + } ); + + // Use a fake parentId so the parent item does not consume + // an upload slot. + const fakeParentId = 'fake-parent-id'; + + // Add 2 sideload items. + unlock( registry.dispatch( uploadStore ) ).addSideloadItem( { + file: jpegFile, + parentId: fakeParentId, + additionalData: { post: 400, image_size: 'thumbnail' }, + operations: [ OperationType.Upload ], + } ); + unlock( registry.dispatch( uploadStore ) ).addSideloadItem( { + file: jpegFile, + parentId: fakeParentId, + additionalData: { post: 400, image_size: 'medium' }, + operations: [ OperationType.Upload ], + } ); + + // Resume the queue. + await unlock( registry.dispatch( uploadStore ) ).resumeQueue(); + + // Only 1 should have started due to maxConcurrentUploads=1. + expect( mediaSideload ).toHaveBeenCalledTimes( 1 ); + + // Complete the first upload to trigger the pending one. + onFileChangeCallback?.( [ + { id: 400, url: 'https://example.com/img.jpg' }, + ] ); + + // Allow async dispatch to propagate. + await new Promise( ( resolve ) => setTimeout( resolve, 0 ) ); + + // The second sideload should now have started. + expect( mediaSideload ).toHaveBeenCalledTimes( 2 ); + } ); + + it( 'resumeItemByPostId is not on private dispatch', () => { + const privateDispatch = unlock( registry.dispatch( uploadStore ) ); + expect( + ( privateDispatch as Record< string, unknown > ) + .resumeItemByPostId + ).toBeUndefined(); + } ); + } ); + describe( 'cancelItem', () => { beforeEach( () => { ( vipsCancelOperations as jest.Mock ).mockClear(); diff --git a/packages/upload-media/src/store/test/selectors.ts b/packages/upload-media/src/store/test/selectors.ts index 67c11445c9559a..21718032b0f004 100644 --- a/packages/upload-media/src/store/test/selectors.ts +++ b/packages/upload-media/src/store/test/selectors.ts @@ -374,6 +374,18 @@ describe( 'selectors', () => { } ); } ); + describe( 'removed selectors', () => { + it( 'isUploadingToPost is no longer exported', () => { + const privateSelectors = require( '../private-selectors' ); + expect( privateSelectors.isUploadingToPost ).toBeUndefined(); + } ); + + it( 'getPausedUploadForPost is no longer exported', () => { + const privateSelectors = require( '../private-selectors' ); + expect( privateSelectors.getPausedUploadForPost ).toBeUndefined(); + } ); + } ); + describe( 'hasPendingItemsByParentId', () => { it( 'should return true if there are items with matching parent ID', () => { const state: State = { From e847348ca6c23e168c3894bb3feef6d9b0d0b187 Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Tue, 17 Mar 2026 15:49:06 -0700 Subject: [PATCH 02/13] Polish concurrent sideload PR - Add CHANGELOG entry for concurrent sideload behavior change - Remove pauseItem from ActionCreators type (no thunk calls it) - Simplify first concurrent sideload test to use fake parentId like the other tests in the same describe block --- packages/upload-media/CHANGELOG.md | 4 ++++ .../upload-media/src/store/private-actions.ts | 1 - packages/upload-media/src/store/test/actions.ts | 15 +++++---------- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/upload-media/CHANGELOG.md b/packages/upload-media/CHANGELOG.md index 2092d419eb5afc..66d3a1403181cb 100644 --- a/packages/upload-media/CHANGELOG.md +++ b/packages/upload-media/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Enhancement + +- Remove sideload upload serialization: thumbnail uploads now run concurrently, governed by `maxConcurrentUploads` instead of being queued one-at-a-time per attachment ([#75257](https://github.com/WordPress/gutenberg/pull/75257)). + ## 0.27.0 (2026-03-18) ## 0.26.0 (2026-03-04) diff --git a/packages/upload-media/src/store/private-actions.ts b/packages/upload-media/src/store/private-actions.ts index 1099b2fb35a03a..039cfa312648f0 100644 --- a/packages/upload-media/src/store/private-actions.ts +++ b/packages/upload-media/src/store/private-actions.ts @@ -61,7 +61,6 @@ type ActionCreators = { addItem: typeof addItem; addSideloadItem: typeof addSideloadItem; removeItem: typeof removeItem; - pauseItem: typeof pauseItem; prepareItem: typeof prepareItem; processItem: typeof processItem; finishOperation: typeof finishOperation; diff --git a/packages/upload-media/src/store/test/actions.ts b/packages/upload-media/src/store/test/actions.ts index ccef65b52df3cd..5eced721412fd9 100644 --- a/packages/upload-media/src/store/test/actions.ts +++ b/packages/upload-media/src/store/test/actions.ts @@ -343,24 +343,19 @@ describe( 'actions', () => { mediaSideload: jest.fn(), } ); - // Add a parent item first. - unlock( registry.dispatch( uploadStore ) ).addItem( { - file: jpegFile, - } ); - const parentItem = unlock( - registry.select( uploadStore ) - ).getAllItems()[ 0 ]; + // Use a fake parentId so we only test sideload scheduling. + const fakeParentId = 'fake-parent-id'; // Add two sideload items targeting the same post. unlock( registry.dispatch( uploadStore ) ).addSideloadItem( { file: jpegFile, - parentId: parentItem.id, + parentId: fakeParentId, additionalData: { post: 100, image_size: 'thumbnail' }, operations: [ OperationType.Upload ], } ); unlock( registry.dispatch( uploadStore ) ).addSideloadItem( { file: jpegFile, - parentId: parentItem.id, + parentId: fakeParentId, additionalData: { post: 100, image_size: 'medium' }, operations: [ OperationType.Upload ], } ); @@ -372,7 +367,7 @@ describe( 'actions', () => { registry.select( uploadStore ) ).getAllItems(); const sideloadItems = items.filter( - ( item ) => item.parentId === parentItem.id + ( item ) => item.parentId === fakeParentId ); // Neither sideload item should be paused. From 7afe338e6631703d944f61c059005339bfce3c1f Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 12:16:45 -0700 Subject: [PATCH 03/13] Fix race condition in concurrent sideload metadata updates Use MySQL advisory locks (GET_LOCK/RELEASE_LOCK) to serialize the read-modify-write cycle for attachment metadata during concurrent sideloads. Without this, parallel sideloads for the same attachment can overwrite each other's metadata changes. The lock is scoped per-attachment and held only during the metadata update (not during file upload). Object cache is invalidated before reading to ensure fresh data from the database. --- ...-gutenberg-rest-attachments-controller.php | 118 +++++++++++++----- 1 file changed, 89 insertions(+), 29 deletions(-) diff --git a/lib/media/class-gutenberg-rest-attachments-controller.php b/lib/media/class-gutenberg-rest-attachments-controller.php index 802c424db3edcf..32343c00c25e3e 100644 --- a/lib/media/class-gutenberg-rest-attachments-controller.php +++ b/lib/media/class-gutenberg-rest-attachments-controller.php @@ -476,43 +476,60 @@ public function sideload_item( WP_REST_Request $request ) { $image_size = $request['image_size']; - $metadata = wp_get_attachment_metadata( $attachment_id, true ); - - if ( ! $metadata ) { - $metadata = array(); + // Acquire a per-attachment advisory lock to prevent concurrent sideloads + // from overwriting each other's metadata changes (read-modify-write race). + if ( ! $this->acquire_metadata_lock( $attachment_id ) ) { + return new WP_Error( + 'rest_upload_lock_timeout', + __( 'Could not acquire metadata lock for this attachment. Please try again.', 'gutenberg' ), + array( 'status' => 503 ) + ); } - if ( 'original' === $image_size ) { - $metadata['original_image'] = wp_basename( $path ); - } elseif ( 'scaled' === $image_size ) { - // The current attached file is the original; record it as original_image. - $current_file = get_attached_file( $attachment_id, true ); - $metadata['original_image'] = wp_basename( $current_file ); + try { + // Invalidate the object cache to read the latest metadata from the database. + wp_cache_delete( $attachment_id, 'post_meta' ); - // Update the attached file to point to the scaled version. - update_attached_file( $attachment_id, $path ); + $metadata = wp_get_attachment_metadata( $attachment_id, true ); - $size = wp_getimagesize( $path ); + if ( ! $metadata ) { + $metadata = array(); + } - $metadata['width'] = $size ? $size[0] : 0; - $metadata['height'] = $size ? $size[1] : 0; - $metadata['filesize'] = wp_filesize( $path ); - $metadata['file'] = _wp_relative_upload_path( $path ); - } else { - $metadata['sizes'] = $metadata['sizes'] ?? array(); + if ( 'original' === $image_size ) { + $metadata['original_image'] = wp_basename( $path ); + } elseif ( 'scaled' === $image_size ) { + // The current attached file is the original; record it as original_image. + $current_file = get_attached_file( $attachment_id, true ); + $metadata['original_image'] = wp_basename( $current_file ); - $size = wp_getimagesize( $path ); + // Update the attached file to point to the scaled version. + update_attached_file( $attachment_id, $path ); - $metadata['sizes'][ $image_size ] = array( - 'width' => $size ? $size[0] : 0, - 'height' => $size ? $size[1] : 0, - 'file' => wp_basename( $path ), - 'mime-type' => $type, - 'filesize' => wp_filesize( $path ), - ); - } + $size = wp_getimagesize( $path ); - wp_update_attachment_metadata( $attachment_id, $metadata ); + $metadata['width'] = $size ? $size[0] : 0; + $metadata['height'] = $size ? $size[1] : 0; + $metadata['filesize'] = wp_filesize( $path ); + $metadata['file'] = _wp_relative_upload_path( $path ); + } else { + $metadata['sizes'] = $metadata['sizes'] ?? array(); + + $size = wp_getimagesize( $path ); + + $metadata['sizes'][ $image_size ] = array( + 'width' => $size ? $size[0] : 0, + 'height' => $size ? $size[1] : 0, + 'file' => wp_basename( $path ), + 'mime-type' => $type, + 'filesize' => wp_filesize( $path ), + ); + } + + wp_update_attachment_metadata( $attachment_id, $metadata ); + } finally { + $this->release_metadata_lock( $attachment_id ); + } $response_request = new WP_REST_Request( WP_REST_Server::READABLE, @@ -531,4 +548,47 @@ public function sideload_item( WP_REST_Request $request ) { return $response; } + + /** + * Acquires a MySQL advisory lock for attachment metadata updates. + * + * Prevents concurrent sideload requests from overwriting each other's + * metadata changes by serializing the read-modify-write cycle. + * + * @param int $attachment_id Attachment ID. + * @param int $timeout Lock timeout in seconds. Default 5. + * @return bool True if the lock was acquired, false on timeout. + */ + private function acquire_metadata_lock( int $attachment_id, int $timeout = 5 ): bool { + global $wpdb; + + $lock_name = $wpdb->prefix . 'attachment_meta_' . $attachment_id; + $lock_name = substr( $lock_name, 0, 64 ); + + // GET_LOCK returns 1 on success, 0 on timeout, NULL on error. + $result = $wpdb->get_var( + $wpdb->prepare( 'SELECT GET_LOCK(%s, %d)', $lock_name, $timeout ) + ); + + return '1' === $result; + } + + /** + * Releases a MySQL advisory lock for attachment metadata updates. + * + * @param int $attachment_id Attachment ID. + * @return bool True if the lock was released, false otherwise. + */ + private function release_metadata_lock( int $attachment_id ): bool { + global $wpdb; + + $lock_name = $wpdb->prefix . 'attachment_meta_' . $attachment_id; + $lock_name = substr( $lock_name, 0, 64 ); + + $result = $wpdb->get_var( + $wpdb->prepare( 'SELECT RELEASE_LOCK(%s)', $lock_name ) + ); + + return '1' === $result; + } } From 391e1b7600fc04294bb5b5cbdbb704a393d71492 Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 12:21:12 -0700 Subject: [PATCH 04/13] Clean up uploaded file on metadata lock timeout Delete the sideloaded file from disk when the advisory lock cannot be acquired, preventing orphaned files that would never be linked to attachment metadata. --- lib/media/class-gutenberg-rest-attachments-controller.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/media/class-gutenberg-rest-attachments-controller.php b/lib/media/class-gutenberg-rest-attachments-controller.php index 32343c00c25e3e..a39b7b03c7acc9 100644 --- a/lib/media/class-gutenberg-rest-attachments-controller.php +++ b/lib/media/class-gutenberg-rest-attachments-controller.php @@ -479,6 +479,8 @@ public function sideload_item( WP_REST_Request $request ) { // Acquire a per-attachment advisory lock to prevent concurrent sideloads // from overwriting each other's metadata changes (read-modify-write race). if ( ! $this->acquire_metadata_lock( $attachment_id ) ) { + // Clean up the uploaded file since we cannot update metadata. + wp_delete_file( $path ); return new WP_Error( 'rest_upload_lock_timeout', __( 'Could not acquire metadata lock for this attachment. Please try again.', 'gutenberg' ), From 8f9b47b7cef49f9c73820a9aef46ebc37dcaaed5 Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 12:27:56 -0700 Subject: [PATCH 05/13] Switch from MySQL advisory locks to transient-based locks Replace GET_LOCK/RELEASE_LOCK with WordPress transient locks, matching the pattern used by wp_maybe_generate_attachment_metadata() in core. This aligns with established WordPress conventions and avoids introducing an unprecedented locking mechanism. The transient lock spins with 50ms pauses until available or a 5-second timeout is reached. The transient auto-expires as a safety net against stale locks from crashed processes. --- ...-gutenberg-rest-attachments-controller.php | 125 ++++++++---------- 1 file changed, 54 insertions(+), 71 deletions(-) diff --git a/lib/media/class-gutenberg-rest-attachments-controller.php b/lib/media/class-gutenberg-rest-attachments-controller.php index a39b7b03c7acc9..4dd43e95bfcd90 100644 --- a/lib/media/class-gutenberg-rest-attachments-controller.php +++ b/lib/media/class-gutenberg-rest-attachments-controller.php @@ -476,9 +476,12 @@ public function sideload_item( WP_REST_Request $request ) { $image_size = $request['image_size']; - // Acquire a per-attachment advisory lock to prevent concurrent sideloads + // Acquire a per-attachment transient lock to prevent concurrent sideloads // from overwriting each other's metadata changes (read-modify-write race). - if ( ! $this->acquire_metadata_lock( $attachment_id ) ) { + // Uses the same transient lock pattern as wp_maybe_generate_attachment_metadata(). + $lock_key = 'wp_sideload_att_' . $attachment_id; + + if ( ! $this->acquire_metadata_lock( $lock_key ) ) { // Clean up the uploaded file since we cannot update metadata. wp_delete_file( $path ); return new WP_Error( @@ -488,51 +491,49 @@ public function sideload_item( WP_REST_Request $request ) { ); } - try { - // Invalidate the object cache to read the latest metadata from the database. - wp_cache_delete( $attachment_id, 'post_meta' ); + // Invalidate the object cache to read the latest metadata from the database. + wp_cache_delete( $attachment_id, 'post_meta' ); - $metadata = wp_get_attachment_metadata( $attachment_id, true ); + $metadata = wp_get_attachment_metadata( $attachment_id, true ); - if ( ! $metadata ) { - $metadata = array(); - } + if ( ! $metadata ) { + $metadata = array(); + } - if ( 'original' === $image_size ) { - $metadata['original_image'] = wp_basename( $path ); - } elseif ( 'scaled' === $image_size ) { - // The current attached file is the original; record it as original_image. - $current_file = get_attached_file( $attachment_id, true ); - $metadata['original_image'] = wp_basename( $current_file ); + if ( 'original' === $image_size ) { + $metadata['original_image'] = wp_basename( $path ); + } elseif ( 'scaled' === $image_size ) { + // The current attached file is the original; record it as original_image. + $current_file = get_attached_file( $attachment_id, true ); + $metadata['original_image'] = wp_basename( $current_file ); - // Update the attached file to point to the scaled version. - update_attached_file( $attachment_id, $path ); + // Update the attached file to point to the scaled version. + update_attached_file( $attachment_id, $path ); - $size = wp_getimagesize( $path ); + $size = wp_getimagesize( $path ); - $metadata['width'] = $size ? $size[0] : 0; - $metadata['height'] = $size ? $size[1] : 0; - $metadata['filesize'] = wp_filesize( $path ); - $metadata['file'] = _wp_relative_upload_path( $path ); - } else { - $metadata['sizes'] = $metadata['sizes'] ?? array(); + $metadata['width'] = $size ? $size[0] : 0; + $metadata['height'] = $size ? $size[1] : 0; + $metadata['filesize'] = wp_filesize( $path ); + $metadata['file'] = _wp_relative_upload_path( $path ); + } else { + $metadata['sizes'] = $metadata['sizes'] ?? array(); - $size = wp_getimagesize( $path ); + $size = wp_getimagesize( $path ); - $metadata['sizes'][ $image_size ] = array( - 'width' => $size ? $size[0] : 0, - 'height' => $size ? $size[1] : 0, - 'file' => wp_basename( $path ), - 'mime-type' => $type, - 'filesize' => wp_filesize( $path ), - ); - } - - wp_update_attachment_metadata( $attachment_id, $metadata ); - } finally { - $this->release_metadata_lock( $attachment_id ); + $metadata['sizes'][ $image_size ] = array( + 'width' => $size ? $size[0] : 0, + 'height' => $size ? $size[1] : 0, + 'file' => wp_basename( $path ), + 'mime-type' => $type, + 'filesize' => wp_filesize( $path ), + ); } + wp_update_attachment_metadata( $attachment_id, $metadata ); + + delete_transient( $lock_key ); + $response_request = new WP_REST_Request( WP_REST_Server::READABLE, rest_get_route_for_post( $attachment_id ) @@ -552,45 +553,27 @@ public function sideload_item( WP_REST_Request $request ) { } /** - * Acquires a MySQL advisory lock for attachment metadata updates. + * Acquires a transient-based lock for attachment metadata updates. * - * Prevents concurrent sideload requests from overwriting each other's - * metadata changes by serializing the read-modify-write cycle. + * Uses the same transient lock pattern as wp_maybe_generate_attachment_metadata() + * in WordPress core. Spins with brief pauses until the lock is available or + * the timeout is reached. * - * @param int $attachment_id Attachment ID. - * @param int $timeout Lock timeout in seconds. Default 5. + * @param string $lock_key Transient key for the lock. + * @param int $timeout Maximum wait time in seconds. Default 5. * @return bool True if the lock was acquired, false on timeout. */ - private function acquire_metadata_lock( int $attachment_id, int $timeout = 5 ): bool { - global $wpdb; + private function acquire_metadata_lock( string $lock_key, int $timeout = 5 ): bool { + $start = time(); - $lock_name = $wpdb->prefix . 'attachment_meta_' . $attachment_id; - $lock_name = substr( $lock_name, 0, 64 ); - - // GET_LOCK returns 1 on success, 0 on timeout, NULL on error. - $result = $wpdb->get_var( - $wpdb->prepare( 'SELECT GET_LOCK(%s, %d)', $lock_name, $timeout ) - ); - - return '1' === $result; - } - - /** - * Releases a MySQL advisory lock for attachment metadata updates. - * - * @param int $attachment_id Attachment ID. - * @return bool True if the lock was released, false otherwise. - */ - private function release_metadata_lock( int $attachment_id ): bool { - global $wpdb; - - $lock_name = $wpdb->prefix . 'attachment_meta_' . $attachment_id; - $lock_name = substr( $lock_name, 0, 64 ); - - $result = $wpdb->get_var( - $wpdb->prepare( 'SELECT RELEASE_LOCK(%s)', $lock_name ) - ); + while ( get_transient( $lock_key ) ) { + if ( ( time() - $start ) >= $timeout ) { + return false; + } + usleep( 50000 ); // 50ms. + } - return '1' === $result; + set_transient( $lock_key, time(), $timeout ); + return true; } } From 8b831282e6cae36b0f11623dbecd04965799c3e9 Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 17:24:48 -0700 Subject: [PATCH 06/13] Move metadata saving from sideload to finalize endpoint Eliminate the race condition in concurrent sideloads by no longer writing attachment metadata in the sideload endpoint. Instead, sideload returns lightweight sub-size data (dimensions, filename, filesize) and the client accumulates it. The finalize endpoint receives all sub-size metadata and writes it in a single operation. This removes the transient-based lock (acquire_metadata_lock) which had TOCTOU issues, and aligns more closely with how WordPress core handles sub-size generation (single metadata write after all sizes are created). --- ...-gutenberg-rest-attachments-controller.php | 168 +++++++++--------- 1 file changed, 82 insertions(+), 86 deletions(-) diff --git a/lib/media/class-gutenberg-rest-attachments-controller.php b/lib/media/class-gutenberg-rest-attachments-controller.php index 4dd43e95bfcd90..86d2b0aed638b0 100644 --- a/lib/media/class-gutenberg-rest-attachments-controller.php +++ b/lib/media/class-gutenberg-rest-attachments-controller.php @@ -68,10 +68,42 @@ public function register_routes(): void { 'callback' => array( $this, 'finalize_item' ), 'permission_callback' => array( $this, 'edit_media_item_permissions_check' ), 'args' => array( - 'id' => array( + 'id' => array( 'description' => __( 'Unique identifier for the attachment.', 'gutenberg' ), 'type' => 'integer', ), + 'sub_sizes' => array( + 'description' => __( 'Array of sub-size metadata collected from sideload responses.', 'gutenberg' ), + 'type' => 'array', + 'default' => array(), + 'items' => array( + 'type' => 'object', + 'properties' => array( + 'image_size' => array( + 'type' => 'string', + 'required' => true, + ), + 'width' => array( + 'type' => 'integer', + ), + 'height' => array( + 'type' => 'integer', + ), + 'file' => array( + 'type' => 'string', + ), + 'mime_type' => array( + 'type' => 'string', + ), + 'filesize' => array( + 'type' => 'integer', + ), + 'original_image' => array( + 'type' => 'string', + ), + ), + ), + ), ), ), 'allow_batch' => $this->allow_batch, @@ -300,6 +332,35 @@ public function finalize_item( WP_REST_Request $request ) { $metadata = array(); } + // Apply all sub-size metadata collected from sideload responses. + $sub_sizes = $request['sub_sizes'] ?? array(); + + foreach ( $sub_sizes as $sub_size ) { + $image_size = $sub_size['image_size']; + + if ( 'original' === $image_size ) { + $metadata['original_image'] = $sub_size['file']; + } elseif ( 'scaled' === $image_size ) { + if ( ! empty( $sub_size['original_image'] ) ) { + $metadata['original_image'] = $sub_size['original_image']; + } + $metadata['width'] = $sub_size['width'] ?? 0; + $metadata['height'] = $sub_size['height'] ?? 0; + $metadata['filesize'] = $sub_size['filesize'] ?? 0; + $metadata['file'] = $sub_size['file'] ?? ''; + } else { + $metadata['sizes'] = $metadata['sizes'] ?? array(); + + $metadata['sizes'][ $image_size ] = array( + 'width' => $sub_size['width'] ?? 0, + 'height' => $sub_size['height'] ?? 0, + 'file' => $sub_size['file'] ?? '', + 'mime-type' => $sub_size['mime_type'] ?? '', + 'filesize' => $sub_size['filesize'] ?? 0, + ); + } + } + /** * Filters the attachment metadata after client-side processing. * @@ -476,104 +537,39 @@ public function sideload_item( WP_REST_Request $request ) { $image_size = $request['image_size']; - // Acquire a per-attachment transient lock to prevent concurrent sideloads - // from overwriting each other's metadata changes (read-modify-write race). - // Uses the same transient lock pattern as wp_maybe_generate_attachment_metadata(). - $lock_key = 'wp_sideload_att_' . $attachment_id; - - if ( ! $this->acquire_metadata_lock( $lock_key ) ) { - // Clean up the uploaded file since we cannot update metadata. - wp_delete_file( $path ); - return new WP_Error( - 'rest_upload_lock_timeout', - __( 'Could not acquire metadata lock for this attachment. Please try again.', 'gutenberg' ), - array( 'status' => 503 ) - ); - } - - // Invalidate the object cache to read the latest metadata from the database. - wp_cache_delete( $attachment_id, 'post_meta' ); - - $metadata = wp_get_attachment_metadata( $attachment_id, true ); - - if ( ! $metadata ) { - $metadata = array(); - } + // Build sub-size data to return to the client. + // The client accumulates these and sends them all to the finalize endpoint. + $sub_size_data = array( + 'image_size' => $image_size, + ); if ( 'original' === $image_size ) { - $metadata['original_image'] = wp_basename( $path ); + $sub_size_data['file'] = wp_basename( $path ); } elseif ( 'scaled' === $image_size ) { - // The current attached file is the original; record it as original_image. - $current_file = get_attached_file( $attachment_id, true ); - $metadata['original_image'] = wp_basename( $current_file ); + // Record the current attached file as the original. + $current_file = get_attached_file( $attachment_id, true ); + $sub_size_data['original_image'] = wp_basename( $current_file ); // Update the attached file to point to the scaled version. + // This writes to _wp_attached_file meta, not _wp_attachment_metadata. update_attached_file( $attachment_id, $path ); $size = wp_getimagesize( $path ); - $metadata['width'] = $size ? $size[0] : 0; - $metadata['height'] = $size ? $size[1] : 0; - $metadata['filesize'] = wp_filesize( $path ); - $metadata['file'] = _wp_relative_upload_path( $path ); + $sub_size_data['width'] = $size ? $size[0] : 0; + $sub_size_data['height'] = $size ? $size[1] : 0; + $sub_size_data['filesize'] = wp_filesize( $path ); + $sub_size_data['file'] = _wp_relative_upload_path( $path ); } else { - $metadata['sizes'] = $metadata['sizes'] ?? array(); - $size = wp_getimagesize( $path ); - $metadata['sizes'][ $image_size ] = array( - 'width' => $size ? $size[0] : 0, - 'height' => $size ? $size[1] : 0, - 'file' => wp_basename( $path ), - 'mime-type' => $type, - 'filesize' => wp_filesize( $path ), - ); - } - - wp_update_attachment_metadata( $attachment_id, $metadata ); - - delete_transient( $lock_key ); - - $response_request = new WP_REST_Request( - WP_REST_Server::READABLE, - rest_get_route_for_post( $attachment_id ) - ); - - $response_request['context'] = 'edit'; - - if ( isset( $request['_fields'] ) ) { - $response_request['_fields'] = $request['_fields']; - } - - $response = $this->prepare_item_for_response( get_post( $attachment_id ), $response_request ); - - $response->header( 'Location', rest_url( rest_get_route_for_post( $attachment_id ) ) ); - - return $response; - } - - /** - * Acquires a transient-based lock for attachment metadata updates. - * - * Uses the same transient lock pattern as wp_maybe_generate_attachment_metadata() - * in WordPress core. Spins with brief pauses until the lock is available or - * the timeout is reached. - * - * @param string $lock_key Transient key for the lock. - * @param int $timeout Maximum wait time in seconds. Default 5. - * @return bool True if the lock was acquired, false on timeout. - */ - private function acquire_metadata_lock( string $lock_key, int $timeout = 5 ): bool { - $start = time(); - - while ( get_transient( $lock_key ) ) { - if ( ( time() - $start ) >= $timeout ) { - return false; - } - usleep( 50000 ); // 50ms. + $sub_size_data['width'] = $size ? $size[0] : 0; + $sub_size_data['height'] = $size ? $size[1] : 0; + $sub_size_data['file'] = wp_basename( $path ); + $sub_size_data['mime_type'] = $type; + $sub_size_data['filesize'] = wp_filesize( $path ); } - set_transient( $lock_key, time(), $timeout ); - return true; + return rest_ensure_response( $sub_size_data ); } } From 29ecd015641defc39d2392f9604da500909cbf6b Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 17:26:38 -0700 Subject: [PATCH 07/13] Update media-utils sideload to return SubSizeData instead of Attachment The sideload endpoint now returns lightweight sub-size data instead of a full attachment response. Update sideloadToServer to return SubSizeData directly and change sideloadMedia to use an onSuccess callback with SubSizeData instead of onFileChange with Attachment. --- packages/media-utils/README.md | 4 +++ packages/media-utils/src/index.ts | 2 +- .../media-utils/src/utils/sideload-media.ts | 19 ++++++++------ .../src/utils/sideload-to-server.ts | 25 ++++++++++--------- packages/media-utils/src/utils/types.ts | 16 ++++++++++++ 5 files changed, 46 insertions(+), 20 deletions(-) diff --git a/packages/media-utils/README.md b/packages/media-utils/README.md index a87aaf89618d9a..7b3fc814169bdd 100644 --- a/packages/media-utils/README.md +++ b/packages/media-utils/README.md @@ -33,6 +33,10 @@ Private @wordpress/media-utils APIs. Undocumented declaration. +### SubSizeData + +Undocumented declaration. + ### transformAttachment Transforms an attachment object from the REST API shape into the shape expected by the block editor and other consumers. diff --git a/packages/media-utils/src/index.ts b/packages/media-utils/src/index.ts index e90b12ce8d6a63..0e6ef85198b39a 100644 --- a/packages/media-utils/src/index.ts +++ b/packages/media-utils/src/index.ts @@ -6,6 +6,6 @@ export { validateFileSize } from './utils/validate-file-size'; export { validateMimeType } from './utils/validate-mime-type'; export { validateMimeTypeForUser } from './utils/validate-mime-type-for-user'; -export type { Attachment, RestAttachment } from './utils/types'; +export type { Attachment, RestAttachment, SubSizeData } from './utils/types'; export { privateApis } from './private-apis'; diff --git a/packages/media-utils/src/utils/sideload-media.ts b/packages/media-utils/src/utils/sideload-media.ts index cff3003c7466ce..abc59ad1a6a8a9 100644 --- a/packages/media-utils/src/utils/sideload-media.ts +++ b/packages/media-utils/src/utils/sideload-media.ts @@ -7,16 +7,18 @@ import { __, sprintf } from '@wordpress/i18n'; * Internal dependencies */ import type { - OnChangeHandler, OnErrorHandler, CreateSideloadFile, RestAttachment, + SubSizeData, } from './types'; import { sideloadToServer } from './sideload-to-server'; import { UploadError } from './upload-error'; const noop = () => {}; +type OnSubSizeHandler = ( subSize: SubSizeData ) => void; + interface SideloadMediaArgs { // Additional data to include in the request. additionalData?: CreateSideloadFile; @@ -26,8 +28,8 @@ interface SideloadMediaArgs { attachmentId: RestAttachment[ 'id' ]; // Function called when an error happens. onError?: OnErrorHandler; - // Function called each time a file or a temporary representation of the file is available. - onFileChange?: OnChangeHandler; + // Function called when the sideload completes with sub-size data. + onSuccess?: OnSubSizeHandler; // Abort signal. signal?: AbortSignal; } @@ -35,12 +37,15 @@ interface SideloadMediaArgs { /** * Uploads a file to the server without creating an attachment. * + * Returns sub-size data instead of a full attachment. The client + * accumulates this data and sends it to the finalize endpoint. + * * @param $0 Parameters object passed to the function. * @param $0.file Media File to Save. * @param $0.attachmentId Parent attachment ID. * @param $0.additionalData Additional data to include in the request. * @param $0.signal Abort signal. - * @param $0.onFileChange Function called each time a file or a temporary representation of the file is available. + * @param $0.onSuccess Function called when the sideload completes with sub-size data. * @param $0.onError Function called when an error happens. */ export async function sideloadMedia( { @@ -48,17 +53,17 @@ export async function sideloadMedia( { attachmentId, additionalData = {}, signal, - onFileChange, + onSuccess, onError = noop, }: SideloadMediaArgs ) { try { - const attachment = await sideloadToServer( + const subSizeData = await sideloadToServer( file, attachmentId, additionalData, signal ); - onFileChange?.( [ attachment ] ); + onSuccess?.( subSizeData ); } catch ( error ) { let message; if ( error instanceof Error ) { diff --git a/packages/media-utils/src/utils/sideload-to-server.ts b/packages/media-utils/src/utils/sideload-to-server.ts index 941baa769fba8d..6baffde280b1d2 100644 --- a/packages/media-utils/src/utils/sideload-to-server.ts +++ b/packages/media-utils/src/utils/sideload-to-server.ts @@ -6,26 +6,29 @@ import apiFetch from '@wordpress/api-fetch'; /** * Internal dependencies */ -import type { CreateSideloadFile, RestAttachment } from './types'; +import type { CreateSideloadFile, RestAttachment, SubSizeData } from './types'; import { flattenFormData } from './flatten-form-data'; -import { transformAttachment } from './transform-attachment'; /** * Uploads a file to the server without creating an attachment. * + * Returns lightweight sub-size data instead of a full attachment. + * The client accumulates these responses and sends them to the + * finalize endpoint. + * * @param file Media File to Save. * @param attachmentId Parent attachment ID. * @param additionalData Additional data to include in the request. * @param signal Abort signal. * - * @return The saved attachment. + * @return Sub-size data for the uploaded file. */ export async function sideloadToServer( file: File, attachmentId: RestAttachment[ 'id' ], additionalData: CreateSideloadFile = {}, signal?: AbortSignal -) { +): Promise< SubSizeData > { // Create upload payload. const data = new FormData(); data.append( 'file', file, file.name || file.type.replace( '/', '.' ) ); @@ -37,12 +40,10 @@ export async function sideloadToServer( ); } - return transformAttachment( - await apiFetch< RestAttachment >( { - path: `/wp/v2/media/${ attachmentId }/sideload`, - body: data, - method: 'POST', - signal, - } ) - ); + return apiFetch< SubSizeData >( { + path: `/wp/v2/media/${ attachmentId }/sideload`, + body: data, + method: 'POST', + signal, + } ); } diff --git a/packages/media-utils/src/utils/types.ts b/packages/media-utils/src/utils/types.ts index c4c6882ea2532e..2be7ed5f17e0ef 100644 --- a/packages/media-utils/src/utils/types.ts +++ b/packages/media-utils/src/utils/types.ts @@ -214,3 +214,19 @@ export interface SideloadAdditionalData { post: RestAttachment[ 'id' ]; image_size?: string; } + +/** + * Sub-size data returned by the sideload endpoint. + * + * Each sideload returns this lightweight object instead of a full attachment. + * The client accumulates these and sends them all to the finalize endpoint. + */ +export interface SubSizeData { + image_size: string; + width?: number; + height?: number; + file: string; + mime_type?: string; + filesize?: number; + original_image?: string; +} From fcc11f1500cb3c5462605279dce0332dd877087e Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 17:29:21 -0700 Subject: [PATCH 08/13] Accumulate sub-size data client-side and pass to finalize Sideload responses now return SubSizeData instead of full attachments. The client accumulates these on the parent queue item via a new AccumulateSubSize reducer action, then passes all collected sub-sizes to the finalize endpoint which writes metadata in a single operation. Remove onChange callbacks from sideload items since they no longer return attachment data for intermediate editor updates. --- .../editor/src/utils/media-finalize/index.js | 3 +- .../upload-media/src/store/private-actions.ts | 34 +++++++------------ packages/upload-media/src/store/reducer.ts | 19 +++++++++++ packages/upload-media/src/store/types.ts | 28 +++++++++++++-- 4 files changed, 59 insertions(+), 25 deletions(-) diff --git a/packages/editor/src/utils/media-finalize/index.js b/packages/editor/src/utils/media-finalize/index.js index d7459f1e8512be..1133cecc7dd7ed 100644 --- a/packages/editor/src/utils/media-finalize/index.js +++ b/packages/editor/src/utils/media-finalize/index.js @@ -3,9 +3,10 @@ */ import apiFetch from '@wordpress/api-fetch'; -export default async function mediaFinalize( id ) { +export default async function mediaFinalize( id, subSizes = [] ) { await apiFetch( { path: `/wp/v2/media/${ id }/finalize`, method: 'POST', + data: { sub_sizes: subSizes }, } ); } diff --git a/packages/upload-media/src/store/private-actions.ts b/packages/upload-media/src/store/private-actions.ts index 039cfa312648f0..a43c5e7acd6b29 100644 --- a/packages/upload-media/src/store/private-actions.ts +++ b/packages/upload-media/src/store/private-actions.ts @@ -25,6 +25,7 @@ import { terminateVipsWorker, } from './utils'; import type { + AccumulateSubSizeAction, AddAction, AdditionalData, AddOperationsAction, @@ -48,6 +49,7 @@ import type { SideloadAdditionalData, Settings, State, + SubSizeData, UpdateProgressAction, UpdateSettingsAction, } from './types'; @@ -787,8 +789,16 @@ export function sideloadItem( id: QueueItemId ) { attachmentId: post as number, additionalData, signal: item.abortController?.signal, - onFileChange: ( [ attachment ] ) => { - dispatch.finishOperation( id, { attachment } ); + onSuccess: ( subSize: SubSizeData ) => { + // Accumulate sub-size data on the parent item for finalize. + if ( item.parentId ) { + dispatch< AccumulateSubSizeAction >( { + type: Type.AccumulateSubSize, + id: item.parentId, + subSize, + } ); + } + dispatch.finishOperation( id, {} ); }, onError: ( error ) => { dispatch.cancelItem( id, error ); @@ -1129,18 +1139,6 @@ export function generateThumbnails( id: QueueItemId ) { dispatch.addSideloadItem( { file, - onChange: ( [ updatedAttachment ] ) => { - // If the sub-size is still being generated, there is no need - // to invoke the callback below. It would just override - // the main image in the editor with the sub-size. - if ( isBlobURL( updatedAttachment.url ) ) { - return; - } - - // This might be confusing, but the idea is to update the original - // image item in the editor with the new one with the added sub-size. - item.onChange?.( [ updatedAttachment ] ); - }, batchId, parentId: item.id, additionalData: { @@ -1195,12 +1193,6 @@ export function generateThumbnails( id: QueueItemId ) { dispatch.addSideloadItem( { file: sourceForScaled, - onChange: ( [ updatedAttachment ] ) => { - if ( isBlobURL( updatedAttachment.url ) ) { - return; - } - item.onChange?.( [ updatedAttachment ] ); - }, batchId, parentId: item.id, additionalData: { @@ -1240,7 +1232,7 @@ export function finalizeItem( id: QueueItemId ) { // Only finalize if we have an attachment ID and a mediaFinalize callback. if ( attachment?.id && mediaFinalize ) { try { - await mediaFinalize( attachment.id ); + await mediaFinalize( attachment.id, item.subSizes || [] ); } catch ( error ) { // Log but don't fail the upload if finalization fails. // eslint-disable-next-line no-console diff --git a/packages/upload-media/src/store/reducer.ts b/packages/upload-media/src/store/reducer.ts index b51759282d6ea5..a9783d2ccdd307 100644 --- a/packages/upload-media/src/store/reducer.ts +++ b/packages/upload-media/src/store/reducer.ts @@ -2,6 +2,7 @@ * Internal dependencies */ import { + type AccumulateSubSizeAction, type AddAction, type AddOperationsAction, type CacheBlobUrlAction, @@ -42,6 +43,7 @@ const DEFAULT_STATE: State = { }; type Action = + | AccumulateSubSizeAction | AddAction | RemoveAction | CancelAction @@ -251,6 +253,23 @@ function reducer( ), }; + case Type.AccumulateSubSize: + return { + ...state, + queue: state.queue.map( + ( item ): QueueItem => + item.id === action.id + ? { + ...item, + subSizes: [ + ...( item.subSizes || [] ), + action.subSize, + ], + } + : item + ), + }; + case Type.UpdateSettings: { return { ...state, diff --git a/packages/upload-media/src/store/types.ts b/packages/upload-media/src/store/types.ts index da9c1d11661f8c..3c977e13a9ba93 100644 --- a/packages/upload-media/src/store/types.ts +++ b/packages/upload-media/src/store/types.ts @@ -1,3 +1,19 @@ +/** + * Sub-size data returned by the sideload endpoint. + * + * Each sideload returns this lightweight object instead of a full attachment. + * The client accumulates these and sends them all to the finalize endpoint. + */ +export interface SubSizeData { + image_size: string; + width?: number; + height?: number; + file: string; + mime_type?: string; + filesize?: number; + original_image?: string; +} + export type QueueItemId = string; export type QueueStatus = 'active' | 'paused'; @@ -26,6 +42,7 @@ export interface QueueItem { sourceAttachmentId?: number; abortController?: AbortController; parentId?: QueueItemId; + subSizes?: SubSizeData[]; } export interface State { @@ -52,6 +69,7 @@ export enum Type { CacheBlobUrl = 'CACHE_BLOB_URL', RevokeBlobUrls = 'REVOKE_BLOB_URLS', UpdateProgress = 'UPDATE_PROGRESS', + AccumulateSubSize = 'ACCUMULATE_SUB_SIZE', UpdateSettings = 'UPDATE_SETTINGS', } @@ -104,6 +122,10 @@ export type UpdateProgressAction = Action< Type.UpdateProgress, { id: QueueItemId; progress: number } >; +export type AccumulateSubSizeAction = Action< + Type.AccumulateSubSize, + { id: QueueItemId; subSize: SubSizeData } +>; export type UpdateSettingsAction = Action< Type.UpdateSettings, { settings: Partial< Settings > } @@ -145,8 +167,8 @@ export interface SideloadMediaArgs { additionalData?: AdditionalData; /** Function called when an error happens. */ onError?: OnErrorHandler; - /** Function called when the file or a temporary representation is available. */ - onFileChange?: OnChangeHandler; + /** Function called when the sideload completes with sub-size data. */ + onSuccess?: ( subSize: SubSizeData ) => void; /** Abort signal to cancel the sideload operation. */ signal?: AbortSignal; } @@ -182,7 +204,7 @@ export interface Settings { // Default is 0.82 if not set. imageQuality?: number; // Function for finalizing an upload after all client-side processing is complete. - mediaFinalize?: ( id: number ) => Promise< void >; + mediaFinalize?: ( id: number, subSizes: SubSizeData[] ) => Promise< void >; } // Matches the Attachment type from the media-utils package. From 62a9ac13043beff8f3dfa8e6ea6ba9fe9d6c355b Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 17:30:58 -0700 Subject: [PATCH 09/13] Update tests for sub-size data accumulation in finalize Update mediaFinalize tests to verify sub_sizes are sent in the request body. Update finalizeItem tests to verify sub-sizes are passed from the queue item. Update sideload tests to use onSuccess callback with SubSizeData instead of onFileChange with Attachment. --- .../src/utils/media-finalize/test/index.js | 26 ++++++++++- .../src/utils/test/sideload-media.ts | 18 ++++++-- .../upload-media/src/store/test/actions.ts | 23 +++++----- .../src/store/test/private-actions.js | 45 +++++++++++++++++-- 4 files changed, 94 insertions(+), 18 deletions(-) diff --git a/packages/editor/src/utils/media-finalize/test/index.js b/packages/editor/src/utils/media-finalize/test/index.js index eeba1c0a9893f3..a13625ec13f618 100644 --- a/packages/editor/src/utils/media-finalize/test/index.js +++ b/packages/editor/src/utils/media-finalize/test/index.js @@ -15,7 +15,30 @@ describe( 'mediaFinalize', () => { jest.clearAllMocks(); } ); - it( 'should call the finalize endpoint with the correct path and method', async () => { + it( 'should call the finalize endpoint with the correct path, method, and sub_sizes', async () => { + apiFetch.mockResolvedValue( {} ); + + const subSizes = [ + { + image_size: 'thumbnail', + width: 150, + height: 150, + file: 'image-150x150.jpg', + mime_type: 'image/jpeg', + filesize: 5000, + }, + ]; + + await mediaFinalize( 123, subSizes ); + + expect( apiFetch ).toHaveBeenCalledWith( { + path: '/wp/v2/media/123/finalize', + method: 'POST', + data: { sub_sizes: subSizes }, + } ); + } ); + + it( 'should send empty sub_sizes array by default', async () => { apiFetch.mockResolvedValue( {} ); await mediaFinalize( 123 ); @@ -23,6 +46,7 @@ describe( 'mediaFinalize', () => { expect( apiFetch ).toHaveBeenCalledWith( { path: '/wp/v2/media/123/finalize', method: 'POST', + data: { sub_sizes: [] }, } ); } ); diff --git a/packages/media-utils/src/utils/test/sideload-media.ts b/packages/media-utils/src/utils/test/sideload-media.ts index 818a09e8fc05f6..662156ba1c4698 100644 --- a/packages/media-utils/src/utils/test/sideload-media.ts +++ b/packages/media-utils/src/utils/test/sideload-media.ts @@ -17,17 +17,27 @@ describe( 'sideloadMedia', () => { jest.clearAllMocks(); } ); - it( 'should sideload to server', async () => { + it( 'should sideload to server and call onSuccess with sub-size data', async () => { + const mockSubSizeData = { + image_size: 'thumbnail', + width: 150, + height: 150, + file: 'test-150x150.jpeg', + mime_type: 'image/jpeg', + filesize: 5000, + }; + ( sideloadToServer as jest.Mock ).mockResolvedValue( mockSubSizeData ); + const onError = jest.fn(); - const onFileChange = jest.fn(); + const onSuccess = jest.fn(); await sideloadMedia( { file: imageFile, attachmentId: 1, onError, - onFileChange, + onSuccess, } ); expect( sideloadToServer ).toHaveBeenCalled(); - expect( onFileChange ).toHaveBeenCalled(); + expect( onSuccess ).toHaveBeenCalledWith( mockSubSizeData ); } ); } ); diff --git a/packages/upload-media/src/store/test/actions.ts b/packages/upload-media/src/store/test/actions.ts index 5eced721412fd9..e9f566b70166a2 100644 --- a/packages/upload-media/src/store/test/actions.ts +++ b/packages/upload-media/src/store/test/actions.ts @@ -444,15 +444,13 @@ describe( 'actions', () => { } ); it( 'starts pending sideloads after one finishes', async () => { - let onFileChangeCallback: - | ( ( - attachments: Array< Record< string, unknown > > - ) => void ) + let onSuccessCallback: + | ( ( subSize: Record< string, unknown > ) => void ) | undefined; - const mediaSideload = jest.fn( ( { onFileChange } ) => { + const mediaSideload = jest.fn( ( { onSuccess } ) => { // Capture the first callback to simulate completion later. - if ( ! onFileChangeCallback ) { - onFileChangeCallback = onFileChange; + if ( ! onSuccessCallback ) { + onSuccessCallback = onSuccess; } } ); @@ -486,9 +484,14 @@ describe( 'actions', () => { expect( mediaSideload ).toHaveBeenCalledTimes( 1 ); // Complete the first upload to trigger the pending one. - onFileChangeCallback?.( [ - { id: 400, url: 'https://example.com/img.jpg' }, - ] ); + onSuccessCallback?.( { + image_size: 'thumbnail', + width: 150, + height: 150, + file: 'image-150x150.jpg', + mime_type: 'image/jpeg', + filesize: 5000, + } ); // Allow async dispatch to propagate. await new Promise( ( resolve ) => setTimeout( resolve, 0 ) ); diff --git a/packages/upload-media/src/store/test/private-actions.js b/packages/upload-media/src/store/test/private-actions.js index 5da1dace106aea..936ec5919728fd 100644 --- a/packages/upload-media/src/store/test/private-actions.js +++ b/packages/upload-media/src/store/test/private-actions.js @@ -253,12 +253,32 @@ describe( 'private actions', () => { } ); describe( 'finalizeItem', () => { - it( 'should call mediaFinalize with the attachment ID', async () => { + const mockSubSizes = [ + { + image_size: 'thumbnail', + width: 150, + height: 150, + file: 'image-150x150.jpg', + mime_type: 'image/jpeg', + filesize: 5000, + }, + { + image_size: 'medium', + width: 300, + height: 200, + file: 'image-300x200.jpg', + mime_type: 'image/jpeg', + filesize: 15000, + }, + ]; + + it( 'should call mediaFinalize with the attachment ID and sub-sizes', async () => { const mediaFinalize = jest.fn().mockResolvedValue( undefined ); const finishOperation = jest.fn(); const select = { getItem: () => ( { attachment: { id: 42 }, + subSizes: mockSubSizes, } ), getSettings: () => ( { mediaFinalize } ), }; @@ -267,7 +287,25 @@ describe( 'private actions', () => { const thunk = finalizeItem( 'test-id' ); await thunk( { select, dispatch } ); - expect( mediaFinalize ).toHaveBeenCalledWith( 42 ); + expect( mediaFinalize ).toHaveBeenCalledWith( 42, mockSubSizes ); + expect( finishOperation ).toHaveBeenCalledWith( 'test-id', {} ); + } ); + + it( 'should pass empty array when no sub-sizes accumulated', async () => { + const mediaFinalize = jest.fn().mockResolvedValue( undefined ); + const finishOperation = jest.fn(); + const select = { + getItem: () => ( { + attachment: { id: 42 }, + } ), + getSettings: () => ( { mediaFinalize } ), + }; + const dispatch = { finishOperation }; + + const thunk = finalizeItem( 'test-id' ); + await thunk( { select, dispatch } ); + + expect( mediaFinalize ).toHaveBeenCalledWith( 42, [] ); expect( finishOperation ).toHaveBeenCalledWith( 'test-id', {} ); } ); @@ -316,6 +354,7 @@ describe( 'private actions', () => { const select = { getItem: () => ( { attachment: { id: 42 }, + subSizes: mockSubSizes, } ), getSettings: () => ( { mediaFinalize } ), }; @@ -324,7 +363,7 @@ describe( 'private actions', () => { const thunk = finalizeItem( 'test-id' ); await thunk( { select, dispatch } ); - expect( mediaFinalize ).toHaveBeenCalledWith( 42 ); + expect( mediaFinalize ).toHaveBeenCalledWith( 42, mockSubSizes ); expect( warnSpy ).toHaveBeenCalledWith( 'Media finalization failed:', expect.any( Error ) From 4d014f233c174590f245ae4063ad75de458d8939 Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 17:37:23 -0700 Subject: [PATCH 10/13] Update PHP tests for finalize metadata writing Update existing sideload tests to match the new response shape (sub-size data instead of full attachment). Add new tests for the finalize endpoint: - test_finalize_writes_regular_sub_sizes: verifies thumbnail and medium metadata are written correctly - test_finalize_writes_scaled_metadata: verifies scaled image metadata including original_image, dimensions, and file path - test_finalize_writes_original_metadata: verifies original_image is set for rotated image sideloads - test_finalize_with_empty_sub_sizes: verifies the wp_generate_attachment_metadata filter still fires - test_finalize_preserves_image_meta: verifies EXIF data is not overwritten when adding sub-sizes Update the full flow integration test to sideload then finalize, verifying the complete upload pipeline works end-to-end. --- ...nberg-rest-attachments-controller-test.php | 416 ++++++++++++++---- 1 file changed, 326 insertions(+), 90 deletions(-) diff --git a/phpunit/media/class-gutenberg-rest-attachments-controller-test.php b/phpunit/media/class-gutenberg-rest-attachments-controller-test.php index 0286ffb310cae9..203f8b36352570 100644 --- a/phpunit/media/class-gutenberg-rest-attachments-controller-test.php +++ b/phpunit/media/class-gutenberg-rest-attachments-controller-test.php @@ -267,14 +267,21 @@ public function test_sideload_item() { $data = $response->get_data(); $this->assertSame( 200, $response->get_status() ); - $this->assertSame( 'image', $data['media_type'] ); - $this->assertArrayHasKey( 'missing_image_sizes', $data ); - $this->assertEmpty( $data['missing_image_sizes'] ); - $this->assertArrayHasKey( 'media_details', $data ); - $this->assertArrayHasKey( 'sizes', $data['media_details'] ); - $this->assertArrayHasKey( 'medium', $data['media_details']['sizes'] ); - $this->assertArrayHasKey( 'file', $data['media_details']['sizes']['medium'] ); - $this->assertSame( 'canola-777x777.jpg', $data['media_details']['sizes']['medium']['file'] ); + + // Sideload now returns sub-size data instead of a full attachment. + $this->assertSame( 'medium', $data['image_size'] ); + $this->assertSame( 'canola-777x777.jpg', $data['file'] ); + $this->assertSame( 'image/jpeg', $data['mime_type'] ); + $this->assertArrayHasKey( 'width', $data ); + $this->assertArrayHasKey( 'height', $data ); + $this->assertArrayHasKey( 'filesize', $data ); + $this->assertGreaterThan( 0, $data['width'] ); + $this->assertGreaterThan( 0, $data['height'] ); + $this->assertGreaterThan( 0, $data['filesize'] ); + + // Sideload should NOT have written metadata — that happens in finalize. + $metadata = wp_get_attachment_metadata( $attachment_id, true ); + $this->assertArrayNotHasKey( 'medium', $metadata['sizes'] ?? array(), 'Sideload should not write metadata; finalize does.' ); } /** @@ -323,12 +330,15 @@ public function test_sideload_item_year_month_based_folders() { $this->assertSame( 200, $response->get_status() ); - $attachment = get_post( $data['id'] ); + // Sideload returns sub-size data; verify the file was saved correctly. + $this->assertSame( 'medium', $data['image_size'] ); + $this->assertSame( 'canola-year-month-777x777.jpg', $data['file'] ); - $this->assertSame( $attachment->post_parent, $data['post'] ); + // Verify the sideloaded file was placed in the parent post's year/month folder. + $attachment = get_post( $attachment_id ); + $attachment_url = wp_get_attachment_url( $attachment->ID ); $this->assertSame( $attachment->post_parent, $published_post ); - $this->assertSame( wp_get_attachment_url( $attachment->ID ), $data['source_url'] ); - $this->assertStringContainsString( '2017/02', $data['source_url'] ); + $this->assertStringContainsString( '2017/02', $attachment_url ); } /** @@ -383,13 +393,15 @@ public function test_sideload_item_year_month_based_folders_page_post_type() { $this->assertSame( 200, $response->get_status() ); - $attachment = get_post( $data['id'] ); + // Sideload returns sub-size data. + $this->assertSame( 'medium', $data['image_size'] ); - $this->assertSame( $attachment->post_parent, $data['post'] ); + // Verify the file is in the current year/month folder (not the page's post date). + $attachment = get_post( $attachment_id ); + $attachment_url = wp_get_attachment_url( $attachment->ID ); $this->assertSame( $attachment->post_parent, $published_post ); - $this->assertSame( wp_get_attachment_url( $attachment->ID ), $data['source_url'] ); - $this->assertStringNotContainsString( '2017/02', $data['source_url'] ); - $this->assertStringContainsString( $subdir, $data['source_url'] ); + $this->assertStringNotContainsString( '2017/02', $attachment_url ); + $this->assertStringContainsString( $subdir, $attachment_url ); } /** @@ -554,7 +566,10 @@ public function test_exif_and_iptc_metadata_extracted() { } /** - * Verifies that sideloading sub-sizes preserves the original image_meta. + * Verifies that sideloading sub-sizes does not modify existing image_meta. + * + * Since sideload no longer writes metadata, the image_meta should remain + * untouched in the database after a sideload. * * @covers ::sideload_item * @requires extension exif @@ -573,8 +588,8 @@ public function test_sideload_preserves_image_meta() { $data = $response->get_data(); $attachment_id = $data['id']; - // Record the original image_meta. - $original_image_meta = $data['media_details']['image_meta']; + // Record the original metadata. + $original_metadata = wp_get_attachment_metadata( $attachment_id, true ); // Now sideload a sub-size. $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/sideload" ); @@ -582,28 +597,19 @@ public function test_sideload_preserves_image_meta() { $request->set_header( 'Content-Disposition', 'attachment; filename=2004-07-22-DSC_0008-150x150.jpg' ); $request->set_param( 'image_size', 'thumbnail' ); - // Use a smaller image for the sub-size (dimensions don't matter for this test). $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); $response = rest_get_server()->dispatch( $request ); - $data = $response->get_data(); $this->assertSame( 200, $response->get_status() ); - // Verify the image_meta is preserved after sideloading. - $this->assertArrayHasKey( 'image_meta', $data['media_details'] ); - $sideloaded_image_meta = $data['media_details']['image_meta']; - - // The EXIF data should be unchanged. - $this->assertSame( $original_image_meta['aperture'], $sideloaded_image_meta['aperture'] ); - $this->assertSame( $original_image_meta['camera'], $sideloaded_image_meta['camera'] ); - $this->assertSame( $original_image_meta['focal_length'], $sideloaded_image_meta['focal_length'] ); - $this->assertSame( $original_image_meta['iso'], $sideloaded_image_meta['iso'] ); + // Verify the metadata is untouched — sideload should not write metadata. + $metadata_after = wp_get_attachment_metadata( $attachment_id, true ); + $this->assertSame( $original_metadata['image_meta'], $metadata_after['image_meta'] ); + $this->assertArrayNotHasKey( 'thumbnail', $metadata_after['sizes'] ?? array() ); } /** - * Verifies that sideloaded sub-sizes include expected metadata fields. - * - * Sub-sizes should have file, width, height, mime_type, and filesize in their metadata. + * Verifies that sideload response includes all expected sub-size data fields. * * @covers ::sideload_item */ @@ -633,21 +639,19 @@ public function test_sideloaded_subsize_has_complete_metadata() { $data = $response->get_data(); $this->assertSame( 200, $response->get_status() ); - $this->assertArrayHasKey( 'sizes', $data['media_details'] ); - $this->assertArrayHasKey( 'medium', $data['media_details']['sizes'] ); - - $medium_size = $data['media_details']['sizes']['medium']; - // Verify all expected metadata fields are present for the sub-size. - $this->assertArrayHasKey( 'file', $medium_size ); - $this->assertArrayHasKey( 'width', $medium_size ); - $this->assertArrayHasKey( 'height', $medium_size ); - $this->assertArrayHasKey( 'mime_type', $medium_size ); - $this->assertArrayHasKey( 'filesize', $medium_size ); + // Verify all expected sub-size data fields are present. + $this->assertArrayHasKey( 'image_size', $data ); + $this->assertArrayHasKey( 'file', $data ); + $this->assertArrayHasKey( 'width', $data ); + $this->assertArrayHasKey( 'height', $data ); + $this->assertArrayHasKey( 'mime_type', $data ); + $this->assertArrayHasKey( 'filesize', $data ); - $this->assertSame( 'canola-300x200.jpg', $medium_size['file'] ); - $this->assertSame( 'image/jpeg', $medium_size['mime_type'] ); - $this->assertGreaterThan( 0, $medium_size['filesize'] ); + $this->assertSame( 'medium', $data['image_size'] ); + $this->assertSame( 'canola-300x200.jpg', $data['file'] ); + $this->assertSame( 'image/jpeg', $data['mime_type'] ); + $this->assertGreaterThan( 0, $data['filesize'] ); } /** @@ -666,15 +670,15 @@ public function test_exif_orientation_in_schema() { } /** - * Verifies that sideloading a scaled image sets original_image metadata - * and updates the attached file to the scaled version, matching core behavior. + * Verifies that sideloading a scaled image updates the attached file + * and returns the correct sub-size data including original_image. * * @see https://github.com/WordPress/wordpress-develop/blob/trunk/tests/phpunit/tests/media.php * For similar core media tests that verify equivalent server-side behavior. * * @covers ::sideload_item */ - public function test_sideload_scaled_sets_original_image_metadata() { + public function test_sideload_scaled_updates_attached_file() { wp_set_current_user( self::$admin_id ); // Upload the original image with client-side processing. @@ -690,9 +694,6 @@ public function test_sideload_scaled_sets_original_image_metadata() { $this->assertSame( 201, $response->get_status() ); $attachment_id = $data['id']; - // Record the original attached file path before sideloading scaled. - $original_attached_file = get_attached_file( $attachment_id, true ); - // Sideload the -scaled version (simulating client-side big image threshold resize). $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/sideload" ); $request->set_header( 'Content-Type', 'image/jpeg' ); @@ -705,37 +706,29 @@ public function test_sideload_scaled_sets_original_image_metadata() { $this->assertSame( 200, $response->get_status() ); - // Verify original_image metadata points to the original filename. - $metadata = wp_get_attachment_metadata( $attachment_id, true ); - $this->assertArrayHasKey( 'original_image', $metadata, 'original_image metadata should be set after scaled sideload.' ); - $this->assertSame( wp_basename( $original_attached_file ), $metadata['original_image'], 'original_image should be the original filename before scaling.' ); + // Verify sub-size data includes original_image and dimensions. + $this->assertSame( 'scaled', $data['image_size'] ); + $this->assertArrayHasKey( 'original_image', $data ); + $this->assertSame( 'my-photo.jpg', $data['original_image'] ); + $this->assertArrayHasKey( 'width', $data ); + $this->assertArrayHasKey( 'height', $data ); + $this->assertArrayHasKey( 'filesize', $data ); + $this->assertGreaterThan( 0, $data['width'] ); + $this->assertGreaterThan( 0, $data['height'] ); + $this->assertGreaterThan( 0, $data['filesize'] ); // Verify the attached file now points to the scaled version. $new_attached_file = get_attached_file( $attachment_id, true ); - $this->assertStringContainsString( '-scaled', wp_basename( $new_attached_file ), 'Attached file should now be the -scaled version.' ); $this->assertSame( 'my-photo-scaled.jpg', wp_basename( $new_attached_file ) ); - - // Verify metadata dimensions and file are updated. - $this->assertArrayHasKey( 'width', $metadata ); - $this->assertArrayHasKey( 'height', $metadata ); - $this->assertArrayHasKey( 'filesize', $metadata ); - $this->assertArrayHasKey( 'file', $metadata ); - $this->assertGreaterThan( 0, $metadata['width'] ); - $this->assertGreaterThan( 0, $metadata['height'] ); - $this->assertGreaterThan( 0, $metadata['filesize'] ); - - // Verify wp_get_original_image_path returns the original file. - $original_path = wp_get_original_image_path( $attachment_id ); - $this->assertSame( 'my-photo.jpg', wp_basename( $original_path ), 'wp_get_original_image_path() should return the original file.' ); } /** - * Verifies that sideloading with image_size=original sets original_image metadata - * without changing the attached file. + * Verifies that sideloading with image_size=original returns the file basename + * and does not change the attached file. * * @covers ::sideload_item */ - public function test_sideload_original_sets_original_image_metadata() { + public function test_sideload_original_returns_file_data() { wp_set_current_user( self::$admin_id ); // Upload via REST so the file is in the uploads directory. @@ -766,10 +759,13 @@ public function test_sideload_original_sets_original_image_metadata() { $this->assertSame( 200, $response->get_status() ); - // Verify original_image metadata is set to the sideloaded file. + // Verify sub-size data returns the file basename. + $this->assertSame( 'original', $data['image_size'] ); + $this->assertSame( 'canola-original.jpg', $data['file'] ); + + // Sideload should NOT have written metadata — that happens in finalize. $metadata = wp_get_attachment_metadata( $attachment_id, true ); - $this->assertArrayHasKey( 'original_image', $metadata ); - $this->assertSame( 'canola-original.jpg', $metadata['original_image'] ); + $this->assertArrayNotHasKey( 'original_image', $metadata, 'Sideload should not write metadata.' ); // Verify the attached file was NOT changed (only scaled changes it). $attached_file_after = get_attached_file( $attachment_id, true ); @@ -778,15 +774,16 @@ public function test_sideload_original_sets_original_image_metadata() { /** * Verifies the full client-side upload flow with scaled image: - * upload original, sideload sub-sizes, sideload scaled version. + * upload original, sideload sub-sizes, sideload scaled version, then finalize. * - * After the full flow, metadata should match core's server-side behavior: + * After finalize, metadata should match core's server-side behavior: * - original_image points to the unscaled original * - attached file points to -scaled version * - sub-sizes are present in metadata * * @covers ::create_item * @covers ::sideload_item + * @covers ::finalize_item */ public function test_full_client_side_upload_flow_with_scaled_image() { wp_set_current_user( self::$admin_id ); @@ -805,21 +802,17 @@ public function test_full_client_side_upload_flow_with_scaled_image() { $attachment_id = $data['id']; $this->assertNotEmpty( $data['missing_image_sizes'], 'Should have missing image sizes after client-side upload.' ); - // Step 2: Sideload a thumbnail sub-size. + // Step 2: Sideload a thumbnail sub-size (file saved, no metadata written). $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/sideload" ); $request->set_header( 'Content-Type', 'image/jpeg' ); $request->set_header( 'Content-Disposition', 'attachment; filename=landscape-150x150.jpg' ); $request->set_param( 'image_size', 'thumbnail' ); $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); - $response = rest_get_server()->dispatch( $request ); + $response = rest_get_server()->dispatch( $request ); + $thumbnail_data = $response->get_data(); $this->assertSame( 200, $response->get_status() ); - // Verify sub-size was added to metadata. - $metadata = wp_get_attachment_metadata( $attachment_id, true ); - $this->assertArrayHasKey( 'thumbnail', $metadata['sizes'] ); - $this->assertSame( 'landscape-150x150.jpg', $metadata['sizes']['thumbnail']['file'] ); - // Step 3: Sideload the scaled version (big image threshold). $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/sideload" ); $request->set_header( 'Content-Type', 'image/jpeg' ); @@ -827,9 +820,19 @@ public function test_full_client_side_upload_flow_with_scaled_image() { $request->set_param( 'image_size', 'scaled' ); $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); - $response = rest_get_server()->dispatch( $request ); - $data = $response->get_data(); + $response = rest_get_server()->dispatch( $request ); + $scaled_data = $response->get_data(); + $this->assertSame( 200, $response->get_status() ); + + // Before finalize: metadata should NOT have the sub-sizes. + $metadata_before = wp_get_attachment_metadata( $attachment_id, true ); + $this->assertArrayNotHasKey( 'thumbnail', $metadata_before['sizes'] ?? array() ); + + // Step 4: Finalize with all collected sub-size data. + $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/finalize" ); + $request->set_param( 'sub_sizes', array( $thumbnail_data, $scaled_data ) ); + $response = rest_get_server()->dispatch( $request ); $this->assertSame( 200, $response->get_status() ); // Verify final metadata matches expected state. @@ -846,7 +849,7 @@ public function test_full_client_side_upload_flow_with_scaled_image() { // The metadata file should reflect the scaled version. $this->assertStringContainsString( 'landscape-scaled.jpg', $metadata['file'] ); - // Sub-sizes should still be present. + // Sub-sizes should be present after finalize. $this->assertArrayHasKey( 'thumbnail', $metadata['sizes'] ); $this->assertSame( 'landscape-150x150.jpg', $metadata['sizes']['thumbnail']['file'] ); @@ -884,7 +887,6 @@ public function test_sideload_scaled_filename_not_suffixed() { $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); $response = rest_get_server()->dispatch( $request ); - $data = $response->get_data(); $this->assertSame( 200, $response->get_status() ); @@ -894,6 +896,240 @@ public function test_sideload_scaled_filename_not_suffixed() { $this->assertStringNotContainsString( '-scaled-1', wp_basename( $attached_file ) ); } + /** + * Verifies that finalize writes sub-size metadata from the sub_sizes parameter. + * + * @covers ::finalize_item + */ + public function test_finalize_writes_regular_sub_sizes() { + wp_set_current_user( self::$admin_id ); + + $request = new WP_REST_Request( 'POST', '/wp/v2/media' ); + $request->set_header( 'Content-Type', 'image/jpeg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=finalize-test.jpg' ); + $request->set_param( 'generate_sub_sizes', false ); + + $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); + $response = rest_get_server()->dispatch( $request ); + $data = $response->get_data(); + $attachment_id = $data['id']; + + // Call finalize with sub_sizes. + $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/finalize" ); + $request->set_param( + 'sub_sizes', + array( + array( + 'image_size' => 'thumbnail', + 'width' => 150, + 'height' => 150, + 'file' => 'finalize-test-150x150.jpg', + 'mime_type' => 'image/jpeg', + 'filesize' => 5000, + ), + array( + 'image_size' => 'medium', + 'width' => 300, + 'height' => 200, + 'file' => 'finalize-test-300x200.jpg', + 'mime_type' => 'image/jpeg', + 'filesize' => 15000, + ), + ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertSame( 200, $response->get_status() ); + + $metadata = wp_get_attachment_metadata( $attachment_id, true ); + + // Verify both sub-sizes were written. + $this->assertArrayHasKey( 'thumbnail', $metadata['sizes'] ); + $this->assertArrayHasKey( 'medium', $metadata['sizes'] ); + + // Verify thumbnail metadata. + $this->assertSame( 150, $metadata['sizes']['thumbnail']['width'] ); + $this->assertSame( 150, $metadata['sizes']['thumbnail']['height'] ); + $this->assertSame( 'finalize-test-150x150.jpg', $metadata['sizes']['thumbnail']['file'] ); + $this->assertSame( 'image/jpeg', $metadata['sizes']['thumbnail']['mime-type'] ); + $this->assertSame( 5000, $metadata['sizes']['thumbnail']['filesize'] ); + + // Verify medium metadata. + $this->assertSame( 300, $metadata['sizes']['medium']['width'] ); + $this->assertSame( 200, $metadata['sizes']['medium']['height'] ); + $this->assertSame( 'finalize-test-300x200.jpg', $metadata['sizes']['medium']['file'] ); + $this->assertSame( 'image/jpeg', $metadata['sizes']['medium']['mime-type'] ); + $this->assertSame( 15000, $metadata['sizes']['medium']['filesize'] ); + } + + /** + * Verifies that finalize writes scaled sub-size metadata correctly. + * + * @covers ::finalize_item + */ + public function test_finalize_writes_scaled_metadata() { + wp_set_current_user( self::$admin_id ); + + $request = new WP_REST_Request( 'POST', '/wp/v2/media' ); + $request->set_header( 'Content-Type', 'image/jpeg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=big-photo.jpg' ); + $request->set_param( 'generate_sub_sizes', false ); + + $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); + $response = rest_get_server()->dispatch( $request ); + $data = $response->get_data(); + $attachment_id = $data['id']; + + $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/finalize" ); + $request->set_param( + 'sub_sizes', + array( + array( + 'image_size' => 'scaled', + 'width' => 2560, + 'height' => 1920, + 'file' => '2026/04/big-photo-scaled.jpg', + 'filesize' => 500000, + 'original_image' => 'big-photo.jpg', + ), + ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertSame( 200, $response->get_status() ); + + $metadata = wp_get_attachment_metadata( $attachment_id, true ); + + $this->assertSame( 'big-photo.jpg', $metadata['original_image'] ); + $this->assertSame( 2560, $metadata['width'] ); + $this->assertSame( 1920, $metadata['height'] ); + $this->assertSame( 500000, $metadata['filesize'] ); + $this->assertSame( '2026/04/big-photo-scaled.jpg', $metadata['file'] ); + } + + /** + * Verifies that finalize writes original sub-size metadata correctly. + * + * @covers ::finalize_item + */ + public function test_finalize_writes_original_metadata() { + wp_set_current_user( self::$admin_id ); + + $request = new WP_REST_Request( 'POST', '/wp/v2/media' ); + $request->set_header( 'Content-Type', 'image/jpeg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=rotated-photo.jpg' ); + $request->set_param( 'generate_sub_sizes', false ); + + $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); + $response = rest_get_server()->dispatch( $request ); + $data = $response->get_data(); + $attachment_id = $data['id']; + + $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/finalize" ); + $request->set_param( + 'sub_sizes', + array( + array( + 'image_size' => 'original', + 'file' => 'rotated-photo-original.jpg', + ), + ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertSame( 200, $response->get_status() ); + + $metadata = wp_get_attachment_metadata( $attachment_id, true ); + $this->assertSame( 'rotated-photo-original.jpg', $metadata['original_image'] ); + } + + /** + * Verifies that finalize with empty sub_sizes still triggers the + * wp_generate_attachment_metadata filter. + * + * @covers ::finalize_item + */ + public function test_finalize_with_empty_sub_sizes() { + wp_set_current_user( self::$admin_id ); + + $request = new WP_REST_Request( 'POST', '/wp/v2/media' ); + $request->set_header( 'Content-Type', 'image/jpeg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=simple.jpg' ); + $request->set_param( 'generate_sub_sizes', false ); + + $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); + $response = rest_get_server()->dispatch( $request ); + $data = $response->get_data(); + $attachment_id = $data['id']; + + $filter_called = false; + add_filter( + 'wp_generate_attachment_metadata', + function ( $metadata ) use ( &$filter_called ) { + $filter_called = true; + return $metadata; + } + ); + + $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/finalize" ); + $response = rest_get_server()->dispatch( $request ); + + $this->assertSame( 200, $response->get_status() ); + $this->assertTrue( $filter_called, 'wp_generate_attachment_metadata filter should be triggered.' ); + } + + /** + * Verifies that finalize preserves existing image_meta when adding sub-sizes. + * + * @covers ::finalize_item + * @requires extension exif + */ + public function test_finalize_preserves_image_meta() { + wp_set_current_user( self::$admin_id ); + + $request = new WP_REST_Request( 'POST', '/wp/v2/media' ); + $request->set_header( 'Content-Type', 'image/jpeg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=2004-07-22-DSC_0008.jpg' ); + $request->set_param( 'generate_sub_sizes', false ); + + $request->set_body( file_get_contents( DIR_TESTDATA . '/images/2004-07-22-DSC_0008.jpg' ) ); + $response = rest_get_server()->dispatch( $request ); + $data = $response->get_data(); + $attachment_id = $data['id']; + + $original_image_meta = wp_get_attachment_metadata( $attachment_id, true )['image_meta']; + + // Finalize with sub-sizes. + $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/finalize" ); + $request->set_param( + 'sub_sizes', + array( + array( + 'image_size' => 'thumbnail', + 'width' => 150, + 'height' => 150, + 'file' => '2004-07-22-DSC_0008-150x150.jpg', + 'mime_type' => 'image/jpeg', + 'filesize' => 5000, + ), + ) + ); + + $response = rest_get_server()->dispatch( $request ); + $this->assertSame( 200, $response->get_status() ); + + $metadata = wp_get_attachment_metadata( $attachment_id, true ); + + // Sub-size should be present. + $this->assertArrayHasKey( 'thumbnail', $metadata['sizes'] ); + + // EXIF data should be preserved. + $this->assertSame( $original_image_meta['aperture'], $metadata['image_meta']['aperture'] ); + $this->assertSame( $original_image_meta['camera'], $metadata['image_meta']['camera'] ); + $this->assertSame( $original_image_meta['focal_length'], $metadata['image_meta']['focal_length'] ); + $this->assertSame( $original_image_meta['iso'], $metadata['image_meta']['iso'] ); + } + /** * Verifies metadata consistency between server-side and client-side upload flows. * From 3f08e6cb319d5a3f9530243c881941612acd65ee Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 18:08:16 -0700 Subject: [PATCH 11/13] Fix PHPCS alignment warnings and PHP test failures Fix equals sign alignment in controller and test file. Fix test_sideload_item to use generate_sub_sizes=false upload so the attachment starts without pre-existing sub-sizes. Fix test_finalize_writes_original_metadata to actually sideload the file before finalizing so it exists on disk. --- ...-gutenberg-rest-attachments-controller.php | 4 +- ...nberg-rest-attachments-controller-test.php | 47 ++++++++++--------- 2 files changed, 26 insertions(+), 25 deletions(-) diff --git a/lib/media/class-gutenberg-rest-attachments-controller.php b/lib/media/class-gutenberg-rest-attachments-controller.php index 86d2b0aed638b0..d348c3800858ca 100644 --- a/lib/media/class-gutenberg-rest-attachments-controller.php +++ b/lib/media/class-gutenberg-rest-attachments-controller.php @@ -547,8 +547,8 @@ public function sideload_item( WP_REST_Request $request ) { $sub_size_data['file'] = wp_basename( $path ); } elseif ( 'scaled' === $image_size ) { // Record the current attached file as the original. - $current_file = get_attached_file( $attachment_id, true ); - $sub_size_data['original_image'] = wp_basename( $current_file ); + $current_file = get_attached_file( $attachment_id, true ); + $sub_size_data['original_image'] = wp_basename( $current_file ); // Update the attached file to point to the scaled version. // This writes to _wp_attached_file meta, not _wp_attachment_metadata. diff --git a/phpunit/media/class-gutenberg-rest-attachments-controller-test.php b/phpunit/media/class-gutenberg-rest-attachments-controller-test.php index 203f8b36352570..1aab6295e14bc4 100644 --- a/phpunit/media/class-gutenberg-rest-attachments-controller-test.php +++ b/phpunit/media/class-gutenberg-rest-attachments-controller-test.php @@ -243,19 +243,15 @@ public function test_prepare_item_lists_missing_image_sizes_for_pdfs() { public function test_sideload_item() { wp_set_current_user( self::$admin_id ); - $attachment_id = self::factory()->attachment->create_object( - DIR_TESTDATA . '/images/canola.jpg', - 0, - array( - 'post_mime_type' => 'image/jpeg', - 'post_excerpt' => 'A sample caption', - ) - ); + // Upload with client-side processing (no server-generated sub-sizes). + $request = new WP_REST_Request( 'POST', '/wp/v2/media' ); + $request->set_header( 'Content-Type', 'image/jpeg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=canola.jpg' ); + $request->set_param( 'generate_sub_sizes', false ); - wp_update_attachment_metadata( - $attachment_id, - wp_generate_attachment_metadata( $attachment_id, DIR_TESTDATA . '/images/canola.jpg' ) - ); + $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); + $response = rest_get_server()->dispatch( $request ); + $attachment_id = $response->get_data()['id']; $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/sideload" ); $request->set_header( 'Content-Type', 'image/jpeg' ); @@ -335,8 +331,8 @@ public function test_sideload_item_year_month_based_folders() { $this->assertSame( 'canola-year-month-777x777.jpg', $data['file'] ); // Verify the sideloaded file was placed in the parent post's year/month folder. - $attachment = get_post( $attachment_id ); - $attachment_url = wp_get_attachment_url( $attachment->ID ); + $attachment = get_post( $attachment_id ); + $attachment_url = wp_get_attachment_url( $attachment->ID ); $this->assertSame( $attachment->post_parent, $published_post ); $this->assertStringContainsString( '2017/02', $attachment_url ); } @@ -1025,16 +1021,21 @@ public function test_finalize_writes_original_metadata() { $data = $response->get_data(); $attachment_id = $data['id']; + // Sideload the "original" version (simulating a rotated image). + $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/sideload" ); + $request->set_header( 'Content-Type', 'image/jpeg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=rotated-photo-original.jpg' ); + $request->set_param( 'image_size', 'original' ); + + $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); + $response = rest_get_server()->dispatch( $request ); + $original_data = $response->get_data(); + + $this->assertSame( 200, $response->get_status() ); + + // Finalize with the original sub-size data. $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/finalize" ); - $request->set_param( - 'sub_sizes', - array( - array( - 'image_size' => 'original', - 'file' => 'rotated-photo-original.jpg', - ), - ) - ); + $request->set_param( 'sub_sizes', array( $original_data ) ); $response = rest_get_server()->dispatch( $request ); $this->assertSame( 200, $response->get_status() ); From 33579aab568624eacf2b623ba2cde3bb7049b759 Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Thu, 2 Apr 2026 19:16:24 -0700 Subject: [PATCH 12/13] Fix test_sideload_item filename collision in CI Use a unique filename (sideload-test.jpg) to avoid collision with canola.jpg that may exist in uploads from other tests in the same CI run. --- .../class-gutenberg-rest-attachments-controller-test.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/phpunit/media/class-gutenberg-rest-attachments-controller-test.php b/phpunit/media/class-gutenberg-rest-attachments-controller-test.php index 1aab6295e14bc4..e8d99b8e7d4536 100644 --- a/phpunit/media/class-gutenberg-rest-attachments-controller-test.php +++ b/phpunit/media/class-gutenberg-rest-attachments-controller-test.php @@ -246,7 +246,7 @@ public function test_sideload_item() { // Upload with client-side processing (no server-generated sub-sizes). $request = new WP_REST_Request( 'POST', '/wp/v2/media' ); $request->set_header( 'Content-Type', 'image/jpeg' ); - $request->set_header( 'Content-Disposition', 'attachment; filename=canola.jpg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=sideload-test.jpg' ); $request->set_param( 'generate_sub_sizes', false ); $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); @@ -255,7 +255,7 @@ public function test_sideload_item() { $request = new WP_REST_Request( 'POST', "/wp/v2/media/$attachment_id/sideload" ); $request->set_header( 'Content-Type', 'image/jpeg' ); - $request->set_header( 'Content-Disposition', 'attachment; filename=canola-777x777.jpg' ); + $request->set_header( 'Content-Disposition', 'attachment; filename=sideload-test-777x777.jpg' ); $request->set_param( 'image_size', 'medium' ); $request->set_body( file_get_contents( DIR_TESTDATA . '/images/canola.jpg' ) ); @@ -266,7 +266,7 @@ public function test_sideload_item() { // Sideload now returns sub-size data instead of a full attachment. $this->assertSame( 'medium', $data['image_size'] ); - $this->assertSame( 'canola-777x777.jpg', $data['file'] ); + $this->assertSame( 'sideload-test-777x777.jpg', $data['file'] ); $this->assertSame( 'image/jpeg', $data['mime_type'] ); $this->assertArrayHasKey( 'width', $data ); $this->assertArrayHasKey( 'height', $data ); From be32a496c86083810ad67e55475cd58e667d4b02 Mon Sep 17 00:00:00 2001 From: adamsilverstein Date: Fri, 3 Apr 2026 15:10:07 -0700 Subject: [PATCH 13/13] Add schema validation for finalize endpoint sub-size fields Add minimum constraints for width, height, and filesize (must be >= 1) and a pattern constraint for mime_type (must start with 'image/'). Props westonruter. --- .../class-gutenberg-rest-attachments-controller.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/media/class-gutenberg-rest-attachments-controller.php b/lib/media/class-gutenberg-rest-attachments-controller.php index d348c3800858ca..2b0576dc19b7fd 100644 --- a/lib/media/class-gutenberg-rest-attachments-controller.php +++ b/lib/media/class-gutenberg-rest-attachments-controller.php @@ -84,19 +84,23 @@ public function register_routes(): void { 'required' => true, ), 'width' => array( - 'type' => 'integer', + 'type' => 'integer', + 'minimum' => 1, ), 'height' => array( - 'type' => 'integer', + 'type' => 'integer', + 'minimum' => 1, ), 'file' => array( 'type' => 'string', ), 'mime_type' => array( - 'type' => 'string', + 'type' => 'string', + 'pattern' => '^image/.*', ), 'filesize' => array( - 'type' => 'integer', + 'type' => 'integer', + 'minimum' => 1, ), 'original_image' => array( 'type' => 'string',