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, for some reason, this doesn't end up in the post data send back to the server, so further requests to file/progress/key will not report the progress since it can't find it.

The code in file.js is more or less the same. The only difference is that we now autotrigger a mousedown on the upload button, however, disabling that doesn't fix this problem, so this behavior doesn't seem to be the main problem here.

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

Comments

swentel created an issue. See original summary.

imre.horjan’s picture

Here'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.

imre.horjan’s picture

Status: Active » Needs review
imre.horjan’s picture

imre.horjan’s picture

imre.horjan’s picture

The 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.

imre.horjan’s picture

swentel’s picture

@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 :)

  1. +++ b/core/modules/file/file.js
    @@ -170,9 +172,24 @@
    +    },    ¶
    +    ¶
    

    trailing spaces

  2. +++ b/core/modules/file/file.js
    @@ -170,9 +172,24 @@
    +    ¶
    

    same here

  3. +++ b/core/modules/file/file.js
    @@ -182,7 +199,18 @@
    +      ¶
    

    same here

  4. +++ b/core/modules/file/file.js
    @@ -208,9 +236,15 @@
    +    ¶
    

    same here

  5. +++ b/core/modules/file/file.js
    @@ -220,7 +254,19 @@
    +      ¶
    

    same here

  6. +++ b/core/modules/file/file.js
    @@ -220,7 +254,19 @@
    +      ¶
    

    and here as well

imre.horjan’s picture

You 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.

imre.horjan’s picture

Finally 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.. :)

swentel’s picture

Very 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.

nod_’s picture

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.

imre.horjan’s picture

I think it's as simple as it should be now. :) It works for me.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Yep, works great, wonderful! :)

droplet’s picture

  1. +++ b/core/modules/file/file.js
    @@ -170,7 +170,9 @@
    +        .trigger('mousedown.fileButtons')
    +        .trigger('mousedown');
    

    Needed docs if there's intended to do so.

  2. +++ b/core/modules/file/file.js
    @@ -209,7 +211,7 @@
    +    }, 100, true),
    

    I'm interested why it chosen `100ms`

  3. +++ b/core/modules/file/file.js
    @@ -88,13 +88,13 @@
    +      $context.find('.js-form-submit').on('mousedown.fileButtons', Drupal.file.disableFields);
    

    added namespace, it can't land on D8.0 :(

imre.horjan’s picture

1. 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

droplet’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.63 KB

ahh. 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.

nod_’s picture

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? :)

droplet’s picture

I 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 ?

imre.horjan’s picture

Does the remove / re-upload functionality work in latest?

swentel’s picture

re #20 - works fine here.

imre.horjan’s picture

Nice. 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. ;)

droplet’s picture

I 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.

imre.horjan’s picture

And 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?

droplet’s picture

NON-AUTO is not patching jQuery Form, it sorted by Drupal internal. So that, it triggered Drupal.ajax with custom event `fileUpload`:

Drupal.Ajax.prototype.eventResponse
...
..
 ajax.$form.ajaxSubmit(ajax.options);

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.

droplet’s picture

@nod_ #18,

If we use a custom event I'm not sure we need the event namespace.

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.

imre.horjan’s picture

We 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:

  • #22: Can't handle this case, first progress bar is jumping back & forward.
  • #18: First progressbar works, second doesn't seem to be initialized.
  • #16: Same as #18.
  • #10: First progressbar works, second is initialized, but doesn't show progress until first is finished.
  • #6: Same as #10. (Not a nice solution, I was interested if it works or not)

#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.

droplet’s picture

Status: Needs review » Needs work

#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.

droplet’s picture

swentel’s picture

Do we also need a test scenario where 1 filefield is set to multiple and you upload multiple files at once ?

droplet’s picture

This 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 )

imre.horjan’s picture

@swentel
I've tried this case, and worked normally (I think there's no difference between patches).

imre.horjan’s picture

@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.

droplet’s picture

@imre.horjan #33,

+++ b/core/modules/file/file.js
@@ -89,7 +89,7 @@
+      // $context.find('.js-form-managed-file .js-form-submit').on('mousedown', Drupal.file.progressBar);

@@ -230,7 +233,10 @@
+          $clickedButton.closest('div.js-form-managed-file').find('input.form-file').remove();

Only 2 lines diff

imre.horjan’s picture

@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.

droplet’s picture

Status: Needs work » Postponed (maintainer needs more info)
Issue tags: +Needs manual testing, +Needs issue summary update

@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: [PERFORMANCE] Simultaneous file uploads re-posting data

imre.horjan’s picture

Well, I got some time to address this issue in my worktime, and finally finished in my free time.

I provided 2 different solutions, tested and collected all pros and cons of all the available solutions (incl. yours).

If someone who arrives to this issue and needs this patch for their project I think their want to know which is the latest working solution.

It's not about understanding your code or not...I think if you add '.remove()' into your code, you should provide information why you added that, and what patch did you continue work on.

Actually I don't care if yours or mine is going to be committed, it's not a competition. I'm looking for the least obstrusive, most working solution with the least side effects, regressions, just like anyone else.

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.

I can sum up 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.

(I think you knows your solutions better, so please sum up how you tried to solve the problem and please hide or mark patches you know are not stable or superseeded by an other patch not to waste others' time)

droplet’s picture

That'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.

imre.horjan’s picture

Sorry about your feelings...
I'll get back with other test scenarios.

Cottser’s picture

Interdiffs save lives.

imre.horjan’s picture

Status: Postponed (maintainer needs more info) » Needs review

Finally 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)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Peter Swietoslawski’s picture

Can 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]" to name="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.

Peter Swietoslawski’s picture

Adding 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 from Drupal::file that deals with the dynamic name replacement.

droplet’s picture

@Peter,
For parallel uploads?

Peter Swietoslawski’s picture

@droplet

I'm not entirely sure what you mean by parallel uploads?

  1. Do you mean uploading multiple files from one File upload field by just selecting more then one file in your Finder/File Explorer?
  2. or do you mean there is multiple upload forms on the page with File upload fields on them that would allow user to click on Choose file on each of the form and start uploading files multiple files at the same time?

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

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.