Enabling files upload #834

Lustmored · 2023-05-02T12:26:30Z

Q	A
Bug fix?	no
New feature?	yes
Tickets
License	MIT

Getting back on the file uploading front I've decided to go slowly this time, as in the end it should allow us to get basic support faster and then build up on it, without determining all the little details of both backend and frontend implementations upfront.

This PR aims at enabling frontend tools to send files back to the server. It does not add file upload functions to the live controller or handle the files on the backend. Instead it focuses only on changes that are essential to get any kind of file upload in the future.

What exactly is done here:

RequestBuilder gets an additional parameter (files) that takes a map of FileList objects that should be uploaded with the request (and the keys by which they should be available). This additional parameter is added as an empty map everywhere right now.
To make file upload possible the content type of a request is moved from JSON to the default (form type) and all the component related data is sent under data key.
RequestBuilder tests are adapted to the changes and tests for appending files to the request are added.
On the backend site LiveComponentSubscriber is adapted to read data from JSON-encoded data parameter instead of request content (in case of a POST request), without any further functional changes.
All tests are adapted to send data in the new format.

Two tests are failing for me locally (and in 2 seemingly random CI configurations), while logic tells me they shouldn't and I'm unable to find the cause right now. Help would be appreciated.

I would love to get that first stepping stone finalized and merged before further work. It is a simple change, but has strong implications, therefore getting those out of the way will make further work much more comfortable. Also - splitting the work will hopefully make it easier for you to review.

weaverryan

Hey @Lustmored!

Nice to see this PR :). I'm following your logic. Changing the POST situation to send normal "form" data where one key is JSON (to make room for the files)... seems to make sense to me.

So, I'm a 👍 for this. However, while I think the "small steps" approach is the way to go, I'm not sure it makes sense to merge this in without having some fully-functional version of it (i.e. these are building blocks, but not useable in any way yet). Perhaps we do need a bigger PR, but building it piece-by-piece is a nice method (this WAS relatively easy to review).

weaverryan · 2023-05-03T18:48:35Z

src/LiveComponent/assets/src/Backend/RequestBuilder.ts

@@ -26,9 +27,15 @@ export default class {
            Accept: 'application/vnd.live-component+html',
        };

+        const totalFiles = Object.entries(files).reduce(


Could just be?

const totalFiles = Object.keys(files).length

I think it's not - that would count FileList objects passed, but they can be empty (like inputs with no files selected). In that case we should not count them.

Actually maybe what would be more fitting is to first filter files to remove empty FileLists and then just use what you propose.

weaverryan · 2023-05-03T18:55:53Z

src/LiveComponent/tests/Functional/Controller/BatchActionControllerTest.php

@@ -31,17 +31,19 @@ public function testCanBatchActions(): void

        $this->browser()
            ->throwExceptions()
-            ->get('/_components/with_actions', ['json' => ['props' => $dehydrated->getProps()]])
+            ->get('/_components/with_actions', ['query' => ['props' => json_encode($dehydrated->getProps())]])


This is an unrelated fix, right? I mean, it was "wrong" before and was just working by some chance, right?

Right - it was simulating GET request, but with data sent in body request as a JSON. I think it shouldn't be possible, but it was for some reason. Now that POST is handled differently it needed a proper fix :)

On the other hand - in GET we use props as a key to send all data, while for the POST method I used data. I only notices this inconsistency at the very end. But on the other hand it looks weird to me to have props key that contains an array with props method in it. Do you think it's worth to change data key to props to match query parameter name?

weaverryan · 2023-05-03T18:57:30Z

Two tests are failing for me locally (and in 2 seemingly random CI configurations), while logic tells me they shouldn't and I'm unable to find the cause right now. Help would be appreciated

Loos like they're related to batch controller? I don't see anything obvious, unfortunately. It must be related to the new data format, but you've updated things like I would expect.

kbond · 2023-05-03T19:14:08Z

To clarify, on the back-end, the uploaded files will be available via $request->files?

Lustmored · 2023-05-04T12:16:04Z

So, I'm a +1 for this. However, while I think the "small steps" approach is the way to go, I'm not sure it makes sense to merge this in without having some fully-functional version of it (i.e. these are building blocks, but not useable in any way yet). Perhaps we do need a bigger PR, but building it piece-by-piece is a nice method (this WAS relatively easy to review).

Sure thing, I will push it a bit until we can actually upload files this way (so that they are available via $request->files) 👍

To clarify, on the back-end, the uploaded files will be available via $request->files?

With this PR - not yet, but this is the initial goal right now, and more sophisticated backend stuff will follow 👍

kbond · 2023-05-04T14:02:40Z

I think if we can access $request->files within a component action, that would be enough for a first step. @weaverryan, do you agree?

weaverryan · 2023-05-06T00:14:48Z

That would probably be enough. We’ll need some frontend implementation, and some thought should be given to when these are sent. Like, it probably doesn’t make sense to upload a file during a normal re-render / model send, as you’d upload the file… but your component wouldn’t be saving yet… so there is some complexity with that. Sending on an action makes more sense. But my point is, we need to think through a minimally viable, real-world workflow :). But I think that would be a great first place to land.

Lustmored · 2023-05-09T11:00:42Z

I'm back with more goodies :) I have taken inspiration from previous PR (#299). It was so easy to implement that I was surprised - current project structure is really pleasant to work with 👍

How it works?

Whenever any file input changes, underlying files are being stored in live component (in pendingFiles) for upload later. They take the key by default from data-model, with fallback to name HTML attribute as usually.

A new action modifier is introduced - files. When used without parameter it will send all pending files with an action. When used with parameter it will only send files associated with passed key (can be chained for multiple keys).

I have added a very simple (and ugly, but that's what happens when I touch Twig files) demo to ux.symfony.com just to showcase this. It would probably need some love if we want to show it publicly. Or you can check it out and experiment with it to see how things work and remove it.

The bad news here is that I have no idea how to approach functional testing this (in JS, there is nothing to test in PHP as this is regular file upload with nothing added on top of it). As far as I gone digging I believe current test suite doesn't support adding files to mocked requests.

Regarding failing tests in PHP - I have switched them from asserting status and context to expecting exceptions. For some reason underlying library in some cases unconditionally handles exceptions even when they are expected. And it fails only in some versions probably because the logic looks for exception page by class names that could've changed between versions (?). Seems weird to me, but the change makes them work and pass.

I have also added a simple docs section. English is not my native language, so it surely needs a bit of love too.

kbond

Looking good!

src/LiveComponent/doc/index.rst

Lustmored · 2023-05-10T12:55:01Z

@kbond Thanks a lot for your help. I have adapted the docs to your suggestions and removed the data-model fallback. It probably made sense in the previous PR, but this time we are not binding files to model data right now, so the usage was purely theoretical.

weaverryan · 2023-05-16T10:54:49Z

@Lustmored Sorry for the delay - and thank you for the demo - SUPER useful. And yea, we'll be able to think of a nice example for a final demo and make it pretty - nothing to worry about now.

You're right that it works really smoothly! I have two questions:

A) How are/should the files be "cleared? For example, in your demo, imagine that we DO process the file uploads in the LiveAction and store them somewhere. Right now, on the page, the file upload fields always remain "attached":

And so also, every time you trigger the LiveAction, the file is sent again. I would imagine that, once a file is sent on an action, we would need to find the corresponding file input and "clear" that upload? Or were you thinking of something different?

B) I added a normal string LiveProp(writable: true) to the demo component and I was happy to see that when the component re-renders for some "normal" reason, any files that I have attached to "file inputs" are not lost during the re-render. I don't see any code for this related to morphdom... so my assumption is that the re-rendering system naturally does not touch the file inputs and "undo" the attaching. That... likely makes sense, as I think when you select a file, no HTML on the page changes (and morphdom's job is to modify HTML), so it "just works". I guess this is more of a statement to make sure you agree than a question ;).

Thanks!

Lustmored · 2023-05-19T10:03:50Z

Regarding your questions:

A) The file should not be sent again and again - changing the field triggers queuing it for upload and when it is being sent - it's removed from pendingFiles. So as far as I do nothing for the input, the file should be sent again only after the input changes.
I'm not sure if/when we should clear the input element - with a tiny rewiring we could do that whenever the files from input are sent to an action. Should I proceed this way?
B) Yes, I agree - selecting files only make changes to the underlying FileList object under input.files, so there is no HTML change at all.

weaverryan · 2023-05-20T12:34:46Z

A) The file should not be sent again and again - changing the field triggers queuing it for upload and when it is being sent - it's removed from pendingFiles. So as far as I do nothing for the input, the file should be sent again only after the input changes.

that’s very nice!

I'm not sure if/when we should clear the input element - with a tiny rewiring we could do that whenever the files from input are sent to an action. Should I proceed this way?

I think so, yes. Let’s think about the use-cases. I MAY have a situation where, after I process the upload, I don’t even render the file field anymore. That use-case, I think, already works wonderfully. The other case is that I DO keep the file field… in case you want to upload again. My thinking is that if we don’t clear that field for them, then they’ll be stuck with this filled-in field with no way to clear it. So yes, unless I’m not thinking of some angle or use-case, let’s proceed with clearing when they are sent :)

thank you!

kbond · 2023-05-20T13:23:44Z

I agree with clearing the field.

Lustmored · 2023-05-25T13:40:06Z

So with a bit of rewiring the inputs are now cleared and I made sure the same files aren't sent over again (they were actually as I missed clearing one queue) 👍

…der 'data' key

…t afterwards

weaverryan · 2023-06-26T21:20:42Z

@Lustmored I just rebased and made some tweaks to the demo. But I think this is ready to go as a V1 - it works great!

We DO need some documentation - do you want to try that? Also, here are the rough spots / questions I'm thinking about. These aren't blockers, but these are not as smooth as I'd like:

A) How would it look to use this in a Symfony form? Perhaps we should create a 2nd demo that submits to a LiveAction (and has some other, non-file fields in the form) and show how everything could be done.

B) I added validation, but it looks terrible (not your fault). Previously, you were storing the UploadedFile on a property. I removed that because I think it may be confusing: users may expect this to persist between re-renders (thinking out loud... with @kbond's filesystem work... I wonder if we COULD have LiveProps that are File objects and which have a custom hydrator so they are dehydrated to a string then re-hydrated back to the File object later). Anyway, having the UploadedFile on a property makes validation easier: you can use ValidatableComponentTrait. But I think this isn't super useful, at least not yet because, in a real situation, you would probably have one/some "file" fields in your form and a few other normal fields. If you have the UploadedFile on a property and you call $this->validate(), it will validate all the fields on the form, which doesn't seem ideal. So, I'm validating manually, and it's not great.

Cheers!

Lustmored · 2023-06-30T10:30:44Z

@weaverryan Regarding the documentation - I think I'm not the best person for the job, but given some guide points about what to document and how to approach it, I can surely give it a try 👍

Regarding your points - both are excellent and valid points. And also a reason I wanted to start small, as both the topics can be approached separately (maybe even others will hop in to help) once we have any kind of working upload support. But I have some thought, so here they are:

A) Regarding forms - I believe one change that could be useful would be to automatically submit all form files on submit event and only clean them from upload queue if response is not 422 (or otherwise detected to be valid), so that subsequent submits resubmit the files, as one would expect. I think that resolves most of the use cases, but does so without validation on the fly. That would probably still require some manual work on component. I'm not sure if there's a good default for when we should submit files for validation.
It could also play nicely with filesystem library, but I'd have to play with it a bit to see where it goes.

B) For that problem I believe @kbond filesystem approach could be the best way to go. It solves many (if not all) problems regarding hydration/temporary storage/validation and so on. Before working on this PR I was working with him on bringing functions useful for live components into the library, so that there is a chance it will be very pleasant combo out of the box. That's what I intent to experiment with after merging this PR and see where it goes. To be honest I have no vision of scope required to achieve easy work with files without that library.

weaverryan · 2023-06-30T18:47:22Z

I believe one change that could be useful would be to automatically submit all form files on submit event and only clean them from upload queue if response is not 422 (or otherwise detected to be valid), so that subsequent submits resubmit the files, as one would expect

That's a really interesting idea. Thinking out loud, we would need to:

Add the files modifier to the <form tag that ComponentWithFormTrait uses (or document to do this... or something) so that the files submit with the form.
Have some new syntax (?) on the files modifier that says "don't clear me on 422".

B) For that problem I believe @kbond filesystem approach could be the best way to go.

Fair enough. Validation is ugly, but not impossible. So let's move forward.

I'm going to merge this and write some docs. My hope is that we (I'll definitely need your lead on this) can continue to push this forward as this v1 is functional, but not what either of us want in the end!

Thanks!

kbond

I'm all for merging this and building on it!

weaverryan · 2023-07-05T15:33:47Z

Thank you @Lustmored!

lukepass · 2023-08-31T17:22:17Z

Hello and thanks for the work! I tried to upload file with version 2.9.1 (it should support files) but an error appears in the console saying Unknown modifier files in action "files|prevent|save". Available modifiers are: prevent, stop, self, debounce.. Thanks.

jmsche · 2023-09-01T08:48:15Z

The version number in the docs is wrong, it should be 2.11 instead.

I guess it was forgotten as the PR was likely written when 2.9 was not released yet.

lukepass · 2023-09-01T09:01:03Z

Thanks, so there is way for this change to work on PHP 8.0?

jmsche · 2023-09-01T09:06:16Z

No, as from v2.10 all UX packages require PHP 8.1.

lukepass · 2023-09-01T09:27:29Z

Would it be really possible to backport just the files upload to the 2.9x branch? Thanks again.

Lustmored · 2023-09-01T09:29:17Z

Would it be really possible to backport just the files upload to the 2.9x branch? Thanks again.

It would not. It was rather big effort with breaking changes involved to achieve the goal, so backporting is unfortunately not an option here.

lukepass · 2023-09-01T09:43:10Z

Ok, thanks! Are there any workarounds or different strategies for uploading files along with a form in a live component?

Lustmored · 2023-09-01T09:46:51Z

I don't think there is something simple. You would have to manage file upload by another stimulus controller within a live component and update some hidden field with it's value so that component handles just raw form data. With 2.9 data transfer method live components simply aren't capable of even receiving files alongside other data.

lukepass · 2023-09-01T09:57:52Z

Maybe I could create a hidden field with a base64 version of the file taken from a dummy input (so it can be uploaded). Then I would need to manually validate the form and the file, convert and save it.

…support (jmsche) This PR was merged into the 2.x branch. Discussion ---------- [LiveComponent][Docs] Fix version added for file uploads support | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Tickets | N/A | License | MIT File uploads support for Live Components was introduced with v2.11, however the docs mention v2.9 instead. See #834 (comment) Commits ------- 16b83ec [LiveComponent][Docs] Fix version added for file uploads support

…support (jmsche) This PR was merged into the 2.x branch. Discussion ---------- [LiveComponent][Docs] Fix version added for file uploads support | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | Tickets | N/A | License | MIT File uploads support for Live Components was introduced with v2.11, however the docs mention v2.9 instead. See symfony/ux#834 (comment) Commits ------- 16b83ec0 [LiveComponent][Docs] Fix version added for file uploads support

weaverryan reviewed May 3, 2023

View reviewed changes

Lustmored force-pushed the upload-again branch from a8b4547 to 18c9dcc Compare May 9, 2023 08:02

Lustmored changed the title ~~Enabling files upload on a RequestBuilder level~~ Enabling files upload May 9, 2023

This was referenced May 9, 2023

[Live][POC] Defining interface for files uploader #404

Closed

[LiveComponent][RFC] Simple file upload handling #299

Closed

Lustmored requested a review from weaverryan May 9, 2023 11:00

kbond reviewed May 9, 2023

View reviewed changes

src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved

src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved

src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved

kbond reviewed May 9, 2023

View reviewed changes

src/LiveComponent/doc/index.rst Outdated Show resolved Hide resolved

kbond mentioned this pull request May 10, 2023

[Live][POC] alternate upload system #395

Closed

12 tasks

weaverryan mentioned this pull request May 16, 2023

[LiveComponent] - Live component and Input File #877

Closed

Lustmored force-pushed the upload-again branch 2 times, most recently from 7be946d to 8cc63ec Compare May 25, 2023 13:30

kbond mentioned this pull request Jun 22, 2023

[Live Component] Are file uploads supported? #486

Open

Lustmored added 4 commits June 26, 2023 16:19

Allow files to be passed to RequestBuilder and move request body un…

2d13c27

…der 'data' key

Make files mandatory in Backend.ts and pass empty map from controller

14a640b

Add tests for passing files to RequestBuilder and make them pass

b5ff362

Read POST payload from data key instead of a request body

fe69154

Lustmored and others added 7 commits June 26, 2023 16:19

Rebuild dist assets

c74f689

Stop using data-model on file inputs as an alternative. It's pointless

2155865

Pump up docs

0e1013e

Clear pendingFiles after being sent

1f74e6d

Rewire files handling to actually send files only once and clear inpu…

65e2794

…t afterwards

Adapt tests again

1a147a2

[Upload] Updating demo

cc41980

weaverryan force-pushed the upload-again branch from b1bc1ca to cc41980 Compare June 26, 2023 21:12

phpcs

380b492

Fixing test

d29df00

weaverryan mentioned this pull request Jun 30, 2023

[Live] Upload Documentation #979

Closed

kbond approved these changes Jul 3, 2023

View reviewed changes

weaverryan merged commit 2dd64b9 into symfony:2.x Jul 5, 2023

jmsche mentioned this pull request Sep 1, 2023

[LiveComponent][Docs] Fix version added for file uploads support #1087

Merged

smnandre mentioned this pull request Mar 9, 2025

[Live] Preserve file input values after render requests #2628

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling files upload #834

Enabling files upload #834

Lustmored commented May 2, 2023

weaverryan left a comment

weaverryan May 3, 2023

Lustmored May 4, 2023

weaverryan May 3, 2023

Lustmored May 4, 2023

weaverryan commented May 3, 2023

kbond commented May 3, 2023

Lustmored commented May 4, 2023

kbond commented May 4, 2023

weaverryan commented May 6, 2023

Lustmored commented May 9, 2023 •

edited

Loading

kbond left a comment

Lustmored commented May 10, 2023

weaverryan commented May 16, 2023

Lustmored commented May 19, 2023

weaverryan commented May 20, 2023

kbond commented May 20, 2023 •

edited

Loading

Lustmored commented May 25, 2023

weaverryan commented Jun 26, 2023

Lustmored commented Jun 30, 2023

weaverryan commented Jun 30, 2023

kbond left a comment

weaverryan commented Jul 5, 2023

lukepass commented Aug 31, 2023

jmsche commented Sep 1, 2023

lukepass commented Sep 1, 2023

jmsche commented Sep 1, 2023

lukepass commented Sep 1, 2023

Lustmored commented Sep 1, 2023

lukepass commented Sep 1, 2023

Lustmored commented Sep 1, 2023

lukepass commented Sep 1, 2023

Enabling files upload #834

Enabling files upload #834

Conversation

Lustmored commented May 2, 2023

weaverryan left a comment

Choose a reason for hiding this comment

weaverryan May 3, 2023

Choose a reason for hiding this comment

Lustmored May 4, 2023

Choose a reason for hiding this comment

weaverryan May 3, 2023

Choose a reason for hiding this comment

Lustmored May 4, 2023

Choose a reason for hiding this comment

weaverryan commented May 3, 2023

kbond commented May 3, 2023

Lustmored commented May 4, 2023

kbond commented May 4, 2023

weaverryan commented May 6, 2023

Lustmored commented May 9, 2023 • edited Loading

kbond left a comment

Choose a reason for hiding this comment

Lustmored commented May 10, 2023

weaverryan commented May 16, 2023

Lustmored commented May 19, 2023

weaverryan commented May 20, 2023

kbond commented May 20, 2023 • edited Loading

Lustmored commented May 25, 2023

weaverryan commented Jun 26, 2023

Lustmored commented Jun 30, 2023

weaverryan commented Jun 30, 2023

kbond left a comment

Choose a reason for hiding this comment

weaverryan commented Jul 5, 2023

lukepass commented Aug 31, 2023

jmsche commented Sep 1, 2023

lukepass commented Sep 1, 2023

jmsche commented Sep 1, 2023

lukepass commented Sep 1, 2023

Lustmored commented Sep 1, 2023

lukepass commented Sep 1, 2023

Lustmored commented Sep 1, 2023

lukepass commented Sep 1, 2023

Lustmored commented May 9, 2023 •

edited

Loading

kbond commented May 20, 2023 •

edited

Loading