Make WordPress Core

Opened 5 weeks ago

Closed 4 weeks ago

#65662 closed defect (bug) (worksforme)

Media: Client-side uploads for the media-new.php uploader and a guard for in-flight uploads

Reported by: adamsilverstein Owned by:
Priority: normal Milestone:
Component: Media Version:
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

#65661 extends WordPress 7.1's client-side media processing pipeline (wasm-vips) to the Media Library grid at upload.php. Two follow-ups were explicitly left out of scope there:

  1. The "Add New Media File" screen (media-new.php). This screen uses a separate uploader path: plupload-handlers creates a raw plupload.Uploader (not wp.Uploader), with its own progress list UI (#media-items) and its own error surfaces. Uploads go to async-upload.php with all image processing server-side. It is the last remaining admin upload surface without pipeline integration.
  1. No warning when leaving during an in-flight upload. With the client-side pipeline, closing the tab mid-upload is worse than the classic flow: thumbnails that have not been sideloaded yet are lost and the attachment is left unfinalized (no finalize request), whereas classic per-file uploads complete server-side once the bytes arrive. A beforeunload guard should warn while pipeline uploads are in flight.

Proposed change

media-new.php pipeline integration:

  • Extend cross-origin isolation to media-new.php by hooking the existing Document-Isolation-Policy output buffer (wp_start_cross_origin_isolation_output_buffer()) on load-media-new.php, gated to users with upload_files (the screen already dies without that capability). No mode gating applies here.
  • Add a media-new-upload admin script that binds a higher-priority FilesAdded handler on the plupload-handlers uploader instance and routes files through the @wordpress/upload-media store (REST upload of the original, client-side thumbnails, sideload, finalize). The existing screen UI is reused rather than replicated: the script calls the global fileQueued(), uploadSuccess(), itemAjaxError(), and uploadComplete() helpers from plupload-handlers, so progress items, the rendered attachment row (via the existing async-upload.php?fetch=3 markup endpoint), and the error styling all work unchanged.
  • Settings are shared with the grid integration via wp_get_media_library_upload_settings() (introduced in #65661).
  • Graceful degradation, as on the grid: when the browser is not cross-origin isolated or lacks client-side support, the script no-ops and the classic plupload flow (and the browser-uploader HTML fallback form) keep working unchanged.

beforeunload guard:

  • Both integration scripts (grid, from #65661, and the new media-new.php one) register a beforeunload handler that triggers the browser's leave-confirmation while pipeline uploads are in flight, and unregisters when the queue drains. Scope is intentionally limited to the client-side pipeline; the classic plupload flow is unchanged.

Testing instructions

  1. Use Chrome 137+ on a secure origin (HTTPS or localhost).
  2. Go to Media > Add New Media File (media-new.php) and upload a JPEG via drag-and-drop or "Select Files".
  3. In DevTools > Network, observe a POST to /wp/v2/media, one or more POSTs to /wp/v2/media/<id>/sideload, one POST to /wp/v2/media/<id>/finalize, and no upload POST to async-upload.php (a GET with fetch=3 for the item markup is expected).
  4. Confirm the progress bar item resolves to the normal attachment row with the Edit and Copy URL links.
  5. While a large upload is in flight (either on the grid or on media-new.php), try closing the tab: the browser should ask for confirmation. After the upload completes, closing should be silent.
  6. In Firefox/Safari, or with ?browser-uploader on media-new.php, confirm uploads still work via the classic path.

Depends on / builds on #65661 (the PR is stacked on its branch).

Change History (3)

This ticket was mentioned in PR #12586 on WordPress/wordpress-develop by @adamsilverstein.


5 weeks ago
#1

  • Keywords has-patch has-unit-tests added

[!NOTE]
Stacked on #12585 (Trac #65661): the branch builds on add/media-library-client-side-uploads, so only the last 3 commits are new here.

## What

Two follow-ups that #12585 explicitly left out of scope:

  1. The "Add New Media File" screen (media-new.php). It uses a separate uploader path: plupload-handlers creates a raw plupload.Uploader (wp.Uploader never loads there) posting to async-upload.php with all image processing server-side. It was the last remaining admin upload surface without client-side pipeline integration.
  2. A beforeunload guard while pipeline uploads are in flight. Interrupting a pipeline upload is worse than interrupting a classic one: classic uploads complete server-side once the bytes arrive, but an interrupted pipeline upload loses browser-generated thumbnails that were not sideloaded yet and leaves the attachment unfinalized.

## How

Commit 1 - beforeunload guard for the grid. media-library-upload.js triggers the browser's leave confirmation while its progress map is non-empty (models stay there from interception until success or error). Scoped to the pipeline; classic uploads behave exactly as before.

Commit 2 - media-new.php pipeline routing. A new wp_set_up_media_new_cross_origin_isolation() on load-media-new.php extends core's Document-Isolation-Policy output buffer to the screen, gated on client-side processing being enabled and upload_files (the screen itself already requires it; no mode gating applies). A new media-new-upload admin script binds a higher-priority FilesAdded handler on the plupload-handlers uploader instance and routes files through the @wordpress/upload-media store, sharing settings with the grid integration via wp_get_media_library_upload_settings().

The screen's existing UI helpers are reused rather than replicated: fileQueued() builds the progress item, uploadSuccess() renders the finished attachment row through the existing async-upload.php?fetch=3 markup endpoint, itemAjaxError() surfaces per-file errors, and uploadComplete() runs when the queue drains - so the screen looks and behaves unchanged. Store progress is mirrored onto the screen's progress bars, and the same beforeunload guard applies. When the browser is not cross-origin isolated or lacks client-side support, the script no-ops and the classic plupload flow (and the ?browser-uploader HTML fallback form) keep working unchanged - degradation, never data loss.

The mediaSideload/mediaFinalize apiFetch wrappers are intentionally duplicated from the grid script rather than extracted into a shared handle: they are thin, and the two screens share no other loadable script (the grid script requires wp.Uploader/media-views, which media-new.php never loads).

Commit 3 - E2E coverage. New spec asserting the DIP header on media-new.php, the happy-path upload (create + sideload + finalize via REST, no file upload through async-upload.php; the fetch=3 markup POST is expected and excluded), and the disallowed-file-type error path. As in #12585, Playwright's Chromium lacks Document-Isolation-Policy support, so upload assertions skip when the context is not isolated; the header assertion always runs.

One deliberate scope choice: the built-in FilesAdded handler blocks WebP/AVIF (and warns for HEIC) when the server cannot edit them; the pipeline handler bypasses those checks because conversion happens client-side - the same behavior the grid integration has.

## Testing

  • 9 new PHPUnit tests (wpMediaNewCrossOriginIsolation.php, wpEnqueueMediaNewUpload.php) pass locally, as do the two suites from #12585.
  • PHPCS: no new issues on the touched files. JSHint passes on the new and modified scripts.
  • Manual flow to verify (Chrome 137+, secure origin): upload on Media > Add New Media File and observe REST create/sideload/finalize in DevTools with no upload POST to async-upload.php; the progress item resolves to the normal row with Edit and Copy URL links; closing the tab mid-upload prompts for confirmation; Firefox/Safari and ?browser-uploader keep the classic path.

@adamsilverstein commented on PR #12586:


4 weeks ago
#2

Consolidated into #12585. This branch was stacked on that one and the two changes share their PHP infrastructure and settings plumbing (and this PR's diff already showed the full combined change since its base was trunk), so a single PR is simpler to review and maintain. All five commits from this branch are now part of #12585 unchanged; Trac #65662 is being closed as a duplicate of #65661.

#3 @adamsilverstein
4 weeks ago

  • Milestone Awaiting Review
  • Resolutionworksforme
  • Status newclosed

I wound up folding this into https://core.trac.wordpress.org/ticket/65661

Note: See TracTickets for help on using tickets.