Split from #2587755: AJAX error when using progress bar on file field widget
When uploadprogress is available, Drupal.file.progressBar temporarily changes the name of the hidden field that contains the key for pecl (from filename[0][UPLOAD_IDENTIFIER] to UPLOAD_IDENTIFIER) or apc uploadprogress (from filename[0][APC_UPLOAD_PROGRESS] to APC_UPLOAD_PROGRESS). However, the jQuery.form plugin starts submitting the form before the field name is changed. This prevents PHP from tracking upload progress.
We need to ensure that the name change is done before the jQuery.form plugin submits the form. The patch for this is ready to be reviewed. A full explanation of all the approaches we tried is in comment #70.
D7 post data
------WebKitFormBoundary4d4zgU831PUSggVA
Content-Disposition: form-data; name="UPLOAD_IDENTIFIER"
123487692
with
D8 post data
------WebKitFormBoundaryctCyV2fneyPxBbKl
Content-Disposition: form-data; name="field_file[0][UPLOAD_IDENTIFIER]"
426806279
Comment | File | Size | Author |
---|---|---|---|
#74 | interdiff-2662932-73-74.txt | 2 KB | Darren Oh |
#74 | upload-progress-bar-2662932-74.patch | 4.58 KB | Darren Oh |
Comments
Comment #2
imre.horjanHere's a patch.
The problem is that the file upload starts asynchronously before file module's event handlers are called.
The code also prevents calling these event handlers twice.
Comment #3
imre.horjanComment #4
imre.horjanHere's a fix for multiple event handler function calls at once.
Comment #5
imre.horjanHere's an other small fix.
Comment #6
imre.horjanThe progressbar should work using the latest patch, however some problems still may exist:
The disableFields() and progressBar() methods are called multiple times when the user selects a file to upload and the upload progress starts automatically, that's why I tried to prevent them to be called multiple times.
When the user clicks a submit button, these methods are called only once, but I'm not sure if they're executed in time (before the upload is actually started).
I've tested with and without Javascipt, with throbber and progressmeter, and even with large files.
Some feedbacks would be appreciated when used on forms having multiple file fields, multi-value fields, and using APCu instead of upload-progress.
Comment #7
imre.horjanComment #8
swentel CreditAttribution: swentel commented@imre.horjan
Nicely done! The progress bar now works indeed. Underneath is a nitpick review for now, all of them trailing spaces.
I will try to ping nod_ and Cottser to look at the actual code itself since I'm less proficient in javascript :)
trailing spaces
same here
same here
same here
same here
and here as well
Comment #9
imre.horjanYou are right, those spaces shouldn't be there.
Even if it works, I had the impression I'm on a wrong track.
I mean maybe someone else can solve it an other way to ensure the form is always submitted later than our handlers are executed.
Comment #10
imre.horjanFinally I could fix this issue in a more clean and nice way without modifying actual code of event handlers using Drupal.debounce() based on the suggestion of nod_ in #2664216: Drupal.debounce() can not access function arguments within closure.. :)
Comment #11
swentel CreditAttribution: swentel commentedVery sweet, works nicely!
The only nitpick is that there's no space between the upload message (Uploading... (@current of @total)) and the 'One file' text. Now that's not really the fault of this patch I think, or shouldn't even be fixed here.
Comment #12
nod_I can't see the progressbar so it's sort of a blind patch. Went ahead and simplified a bit the code, a couple of temporary variables were not needed and placed debounce so that we don't need a separate objects for the callbacks.
Anyway, the only thing I'm not sure will work as expected is what I did in triggerUploadButton, which is sort of the whole point of the patch, In theory it works, but you never know… If I'm wrong I'll eat some turnips for punishement :D
No interdiff since the updated version is very simple compared to existing code.
Comment #13
imre.horjanI think it's as simple as it should be now. :) It works for me.
Comment #14
swentel CreditAttribution: swentel commentedYep, works great, wonderful! :)
Comment #15
droplet CreditAttribution: droplet commentedNeeded docs if there's intended to do so.
I'm interested why it chosen `100ms`
added namespace, it can't land on D8.0 :(
Comment #16
imre.horjan1. Comments are added.
2. 100 is less than 1000 (until the form's file elements are disabled and the form can not be used normally), it's more than 16.667 (if we assume 60 fps), and is an acceptable number since I don't know any human who can attach 10 files to a form in a second clicking on multiple file upload elements. 100 is even acceptable on slow machines.
3. No regressions. That's an event namespace, not an API namespace. A red apple is still an apple. :)
https://api.jquery.com/event.namespace/
http://www.w3schools.com/jquery/event_namespace.asp
Bug fixes should go to 8.0 as stated here: https://www.drupal.org/node/2494073#comment-10833258
Comment #17
droplet CreditAttribution: droplet commentedahh. I may understand what we're trying to do now. We (I) fooled by the event name. @see attached patch. I've tested on my local and working.
Comment #18
nod_Yep, that's clearer like that. New patch with trivial doc fixes.
If we use a custom event I'm not sure we need the event namespace.
Can this patch get any smaller? :)
Comment #19
droplet CreditAttribution: droplet commentedI just realized that NON-AUTO-UPLOAD BUTTON do not working on both patches in #16 & #17. But this is working on D7 for some reason.
Should we setup Drupal.file.progressBar() on mousedown ?
Comment #20
imre.horjanDoes the remove / re-upload functionality work in latest?
Comment #21
swentel CreditAttribution: swentel commentedre #20 - works fine here.
Comment #22
imre.horjanNice. I'll get back and test regressions, because ('mousedown') is removed in #17.
Until that I share my opinions:
#12 & #16 is RTBC for me, and can go to 8.0 because original handlers are not removed, functionality is injected between attach & event handlers. It's also tested.
#17 & #18 Much cleaner. :)
('mousedown') is removed, so we need to test edge cases.
Even if these buttons are hidden it can only go to 8.1 because theoretically someone has a custom JS which triggers mousedown on these buttons, and will break after update, what means API change. (Whoever triggers this button if upload starts automatically? :D Tests or bots maybe? )
If we agree we don't need mousedown handlers any more it's a nice solution. ;)
Comment #23
droplet CreditAttribution: droplet commentedI minimized the patch for auto upload features.
I think we better kick it in first and split non-auto upload to another issue. (no regressions)
Also, I've confirmed that non-auto upload problem caused by:
* jQuery Form Plugin
* version: 3.51.0-2014.06.20
I also uploaded a patch to demo non-auto buttons fixing. IMO, this is the best for both cases, but it may not able to kick in D8.0.x ?
(I commented out some code for quick testing)
**Sidenotes: we also have to address the `fileAutoUpload` on slow networks. Now, the upload button is hidden by default (HTML). Since all behaviors run on Dom Ready, onChange won't be triggered.
Comment #24
imre.horjanAnd finally you got it even shorter. :)
Patch #22 works here. RTBC?
Do you think the other (NON-AUTO) can be solved by updating/downgrading or patching JQuery Form?
Comment #25
droplet CreditAttribution: droplet commentedNON-AUTO is not patching jQuery Form, it sorted by Drupal internal. So that, it triggered Drupal.ajax with custom event `fileUpload`:
It's also standardized all UPLOAD handlers to triggerUploadButton only.
** Even said jQuery Form fixing the problem in next version, non-auto.patch still good to go IMO.
Comment #26
droplet CreditAttribution: droplet commented@nod_ #18,
IMO, we should namespaced all events. eg. mousedown.drupal.fileUpload. (event.drupal.moduleNameOrFunctionName).
I will open an issue to discuss it soon. If someone can do it, please do so.
Comment #27
imre.horjanWe have multiple different solutions for the same problem here, so I decided to test all of them using multiple fields attached to a node.
I've added file 1 & file 2 fields to a content type, and started two simultanous upload testing how they work in this case.
I've applied the patch, clear caches, and refreshed the page in all cases, and executed the test multiple times for each not to get any false results by mistake.
Of course I skipped testing trivial fixes.
I'm working on a system where we need to handle large files that's why I'm really interested in fixing this issue.
Let's see the results from the latest to the first:
#10 gave the best results, but it's not a nice solution because a new helper class is introduced.
I suggest review why #18 and #16 are failing or how can we remove utility class from #10.
IMO #16 is the less obstrusive solution but I'm not sure why it fails in this case, maybe we don't need debounce at all?
Actually I wouldn't go with #18 because if someone needs a custom widget, triggering mousedown on submit button is the most straightforward way to implement.
Comment #28
droplet CreditAttribution: droplet commented#23 back & fore, we need to take the mousedown event down.
---
It seems a bug of simultanous uploading. Each upload posting FULL FORM data to backend. therefore,
First Upload is sending FIRST UPLOAD
Second Upload is sending SECOND UPLOAD, also the FIRST UPLOAD AGAIN.
Comment #29
droplet CreditAttribution: droplet commented#2666746: Fix simultaneous file uploads re-posting data
Comment #30
swentel CreditAttribution: swentel commentedDo we also need a test scenario where 1 filefield is set to multiple and you upload multiple files at once ?
Comment #31
droplet CreditAttribution: droplet commentedThis isn't the final workaround but can you help to test. Thanks. ( It's worked on my side )
(wait 2 sec between 2 uploads, less side effects )
Comment #32
imre.horjan@swentel
I've tried this case, and worked normally (I think there's no difference between patches).
Comment #33
imre.horjan@droplet
Could you please provide interdiff and state which patch did you start from?
I'm afraid of quick & untested patches, since I've spent a lot of time with testing.
Comment #34
droplet CreditAttribution: droplet commented@imre.horjan #33,
Only 2 lines diff
Comment #35
imre.horjan@swentel
I'll test this case also, maybe there are differences, I'm not sure now.
@droplet
If you don't provide information what and why and in which patch did you change that is not so useful and lead to having more different patches instead of having only one which works and tested.
Comment #36
droplet CreditAttribution: droplet commented@imre.horjan,
the patch is pretty simple, it's actually trying to test the simultaneous uploading bugs. I have no more additional info to my patch at the moment (As you can see, I didn't set the status to `Needs Review`. It's not ready as I told.). If you have no time or didn't understand the patch, just skip it. I'm pretty sure we have to test it at least 3 ~ 50 more times.
As I said before in #28, it worked on my side. The reason I asked you to test is I wonder if there's anymore scenario I didn't catch on my side.
We needed issue summary update to help maintainers to reproduce different bugs.
Also, we may have to sort following issue first:
#2666746: Fix simultaneous file uploads re-posting data
Comment #37
imre.horjanWell, I got some time to address this issue in my worktime, and finally finished in my free time.
Testing this issue is time-consuming since we need to upload large files all the time. We need to ensure multi-value fields, multiple fields for single entities, while keeping in mind non-JS submissions, not to break up throbber, custom widgets or any other part of any D8 site ever been installed, and without adding new side effects.
My current solutions:
#6 Solves the problem using helper variables like Drupal.file.preventProgressBarCall.
#10 Using helper class (Drupal.fileHandlerDebounce) to debounce event handlers.
#16 debounced event handlers in Drupal.file.
Comment #38
droplet CreditAttribution: droplet commentedThat's sad. I'm doing it 100% in my free time. I should have a rest now. I will drop out awhile. But I must jump out for final reviewing since @nod_ said he can't test it in #12.
I don't care which solution kick into CORE also. My goal is move this forward WITHOUT errors & Redundant code.
According to #27 & #37, the problem doesn't address yet.
Thanks ALL.
Comment #39
droplet CreditAttribution: droplet commentedComment #40
imre.horjanSorry about your feelings...
I'll get back with other test scenarios.
Comment #41
star-szrInterdiffs save lives.
Comment #42
imre.horjanFinally it turned out I have no time for other test scenarios, so I put it back to needs review since we have some working solutions. Actually, I'm using patch #10 on my project.
Progressbar works until ~2GB file size is reached (higher than I expected). Progress meter starts to show negative values when it reaches 2GB, however that's the limitation of Upload progress. (Regarding to specification it won't work for 1GB+ files)
Comment #44
Peter Swietoslawski CreditAttribution: Peter Swietoslawski commentedCan anybody explain to me why do we even do all this JavaScript dynamic name changing of upload progress hidden field in
Drupal::file::progressBar()
?Looks to me like all we're trying to achieve is to temporarily adjust name of this element from something like
name="field_attachment[0][UPLOAD_IDENTIFIER]"
toname="UPLOAD_IDENTIFIER"
which is required by Uploadprogress extension to work correctly.Why not just get to the root cause of the issue and tell
ManagedFile::processManagedFile()
method responsible for generating file upload form to set the name of the element correctly in the first place'#name' => 'UPLOAD_IDENTIFIER',
?I've tried it and progress bar with Uploadprogress worked fine.
Comment #45
Peter Swietoslawski CreditAttribution: Peter Swietoslawski commentedAdding patch for
ManagedFile::processManagedFile()
in case somebody wants to try it. Of course if we go this route then we can remove a bunch of unnecessary JavaScript code fromDrupal::file
that deals with the dynamic name replacement.Comment #46
droplet CreditAttribution: droplet commented@Peter,
For parallel uploads?
Comment #47
Peter Swietoslawski CreditAttribution: Peter Swietoslawski commented@droplet
I'm not entirely sure what you mean by parallel uploads?
In first case the upload doesn't work in parallel and files being uploaded sequentially.
In second case the upload works in parallel but the information about the uploaded files is incorrect as Uploadprogress saves information about each upload to the same file in /tmp/upt_xxxx reporting info about first file then overriding it with the info about second file etc.
I believe solution to the problem here might be PHP's native Session Upload https://www.drupal.org/node/1561866#comment-11546855
Comment #51
anavarreComment #52
Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commentedComment #54
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #55
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedPatch for 8.6.x , please review it :)
Comment #56
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #57
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #58
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedComment #59
Darren Oh#45 and #55 are incorrect because, when there are multiple file upload elements, each one must have a unique key.
Comment #60
Darren OhComment #61
Darren OhRerolled patches from droplet that need testing and added interdiff.
Comment #62
Darren OhRerolled patch from nod_.
Comment #64
Darren OhRerolled patches from imre.horjan and added interdiff.
Comment #66
Darren OhRemoved patch from droplet that failed tests.
Comment #67
Darren OhAdded more interdiffs.
Comment #68
Darren OhComment #69
Darren OhComment #70
Darren OhI have reviewed all the patches. There were three different successful approaches:
I tried three other approaches:
In the end, I updated droplet's patch to support auto upload. This version of the patch passed all my tests.
Comment #71
Darren OhComment #72
imre.horjanHi Darren Oh,
I appreciate you finally took time to progress on this issue.
#70 seems to be an elegant solution.
(I haven't tested though)
Comment #73
Darren OhI accidentally changed the event on both the upload button and the remove button. Updated patch attached.
Comment #74
Darren OhFixed an undefined variable error.
Comment #75
mohit1604 CreditAttribution: mohit1604 at Google Summer of Code commentedPatch #74 looks good to me. Marking RTBC.
Comment #76
Darren OhComment #77
lauriiiThank you all for your work. The changes look good. I tried to test all possible use cases for this: auto file upload, file upload with upload button on both single and multi field configuration including without JavaScript.
Given that this bug fix is still potentially disruptive, I will only commit this for 8.6.x so we have time to fix any regressions.
Committed 8b236d9 and pushed to 8.6.x. Thanks!
Comment #79
lauriii