WP-NOW: fix file uploads using multipart form data#203
Conversation
| * Encodes the Express request with files into multipart/form-data request body. | ||
| * Adaptation of https://github.com/WordPress/wordpress-playground/blob/45bf16970867cc6f23738224dacf201905adcab6/packages/php-wasm/universal/src/lib/encode-as-multipart.ts | ||
| */ | ||
| export async function encodeAsMultipart(req: Express.Request) { |
There was a problem hiding this comment.
Could you just passthrough the raw request bytes to PHP without re-encoding them as multipart?
There was a problem hiding this comment.
Good question. I presume we could pass the data using an object to let Playground encode it as multipart (reference). The only tricky part would convert the data from files property of the request to File class so it can be processed properly.
There was a problem hiding this comment.
@fluiddot the browser has already encoded the request – it should be possible to read the raw request body bytes and pass them, unchanged, to Playground. There's no need to create File instances or multipart-encode anything in the JavaScript code.
There was a problem hiding this comment.
I spent time reviewing this logic while addressing a Studio app error, as similar logic is applied there.
From what I gather, by the time we reach the point to pass the request values onto PHP, the express-fileupload package has relocated any file data from req.body to req.files.
The file uploading logic was added in WordPress/wordpress-playground#188. My perception is that using a package like express-fileupload or multer is typical and required, as Express does not provide default support for multipart/form-data. @sejas does this align with your understanding as well?
I attempted to merely pass long both req.body and req.files to PHP as the body attribute, but that ended with "out of bounds" errors for the Uint8Array, as the files are not in a format that Playground's multipart encoding would recognize currently (instance of File).
I imagine we'd either need to retain this File creation/mutlipart-encode logic or expand Playground's multipart-encode logic to accept different file data structures.
There was a problem hiding this comment.
@dcalhoun here's how @wp-playground/cli supports uploads without any multipart parsing:
There was a problem hiding this comment.
A multipart-related bug that was submitted recently: #259
There was a problem hiding this comment.
I've tested the approach you shared @adamziel and seems to work if I disable the Express fileUpload middleware. I presume the middleware is modifying the request in a way that produces the out of bounds error that @dcalhoun described here. I'll create a PR to update this.
There was a problem hiding this comment.
Here's the PR with the suggestion applied. Let me know if someone could take a look, thanks 🙇 !
|
Niiiiiiiiice 
|
<!-- Thanks for contributing to WordPress Playground Tools! --> ## What? <!-- In a few words, what is the PR actually doing? Include screenshots or screencasts if applicable --> Drop manually encoding multipart requests. This PR is a follow-up of #203. ## Why? <!-- Why is this PR necessary? What problem is it solving? Reference any existing previous issue(s) or PR(s), but please add a short summary here, too --> This PR follows [this discussion](#203 (comment)) regarding the necessity of manually encoding multipart requests. Additionally, it addresses #259. ## How? <!-- How is your PR addressing the issue at hand? What are the implementation details? --> - Remove Express `fileUpload` middleware to avoid modifying the request object. - Remove `encodeAsMultipart` helper, since we don't need to process them differently ([Playground CLI example](https://github.com/WordPress/wordpress-playground/blob/946dd7225cec1b5bebbe33072b246609d9e4524a/packages/playground/cli/src/server.ts#L28-L41)). ## Testing Instructions <!-- Please include step by step instructions on how to test this PR. --> <!-- 1. Check out the branch. --> <!-- 2. Run a command. --> <!-- 3. etc. --> 1. Run the command: `nvm use && npm install`. 2. Run the command `nx preview wp-now start`. 3. Open the WP-admin page (e.g. `http://localhost:8881/wp-admin`). 4. Navigate to the Media page. 5. Upload a media file. 6. Observe the media has been uploaded correctly to the Media Library.
<!-- Thanks for contributing to WordPress Playground Tools! --> ## What? <!-- In a few words, what is the PR actually doing? Include screenshots or screencasts if applicable --> Drop manually encoding multipart requests. This PR is a follow-up of WordPress/playground-tools#203. ## Why? <!-- Why is this PR necessary? What problem is it solving? Reference any existing previous issue(s) or PR(s), but please add a short summary here, too --> This PR follows [this discussion](WordPress/playground-tools#203 (comment)) regarding the necessity of manually encoding multipart requests. Additionally, it addresses WordPress/playground-tools#259. ## How? <!-- How is your PR addressing the issue at hand? What are the implementation details? --> - Remove Express `fileUpload` middleware to avoid modifying the request object. - Remove `encodeAsMultipart` helper, since we don't need to process them differently ([Playground CLI example](https://github.com/WordPress/wordpress-playground/blob/946dd7225cec1b5bebbe33072b246609d9e4524a/packages/playground/cli/src/server.ts#L28-L41)). ## Testing Instructions <!-- Please include step by step instructions on how to test this PR. --> <!-- 1. Check out the branch. --> <!-- 2. Run a command. --> <!-- 3. etc. --> 1. Run the command: `nvm use && npm install`. 2. Run the command `nx preview wp-now start`. 3. Open the WP-admin page (e.g. `http://localhost:8881/wp-admin`). 4. Navigate to the Media page. 5. Upload a media file. 6. Observe the media has been uploaded correctly to the Media Library.
6 participants
What?
This PR fixes the ability to upload Images and any file in general using wp-now.
Why?
filesproperty from the request handler.How?
encodeAsMultipartwhich was used as a base to fix the issue in this PR.Testing Instructions
nvm use && npm install && npm startnpx nx build wp-now && node dist/packages/wp-now/cli.js startAdd New Media File.wp-now-upload-files.mp4