October: File upload widget handler is running too many times

[tobias-kuendig]

Using the test plugin:

1. Navigate to Playground -> Posts
2. Click on a post to edit it
3. In the Galleries tab, create a new gallery
4. Upload a single image
5. Hit save

Expected: The image is uploaded once.

Effective result: The image is uploaded three times.

The checkUploadPostback method is run for every file upload instance of the form but the uniqueId check in the beginning of the method somehow allows multiple file uploads to go through for a single request.

[mjauvin]

Ok, was able to reproduce this with latest october develop...

[mjauvin]

Just tested the same with octobercms/october MASTER branch and the bug does not happen.

[mjauvin]

I identified commit 5190c8177bbb6531836ad8fa8458111361a35608 to be causing the problem.

need to investigate further.

[mjauvin]

@bennothommo @w20k do you see anything in that commit that would cause this issue?

[LukeTowers]

@mjauvin it's the removal of https://github.com/octobercms/october/commit/5190c8177bbb6531836ad8fa8458111361a35608#diff-1ed71bcf1b00cec1209c03574555ff54L432.

We need to add better filtering to the checkUploadPostback to fix the check done here: https://github.com/octobercms/october/commit/5190c8177bbb6531836ad8fa8458111361a35608#diff-1ed71bcf1b00cec1209c03574555ff54R365 so that it truly is unique per widget instance. Usually that would be the alias, I'm not sure what the getId() is.

When the fix is identified, this will probably need to be applied to the MediaManager widget too

[mjauvin]

@LukeTowers Any idea why/how checkUploadPostback() is being called 3 times for one upload action in the first place?

[LukeTowers]

@mjauvin because it's called from the init() method of the widget, and every widget for a request is initialized on that request.

[mjauvin]

The bug only shows up if the attachMany field is in a popUp form (so AJAX handlers).

[mjauvin]

I really don't get where those 2 extra init() calls are coming from... there is only one FileUpload widget on that form

[LukeTowers]

Hmm, maybe the widget is getting initialized in multiple places

[tobias-kuendig]

When I was looking into the issue I found that the widget somehow also is initialized via the relationRender calls.

[bennothommo]

A potential fix may be to remove the temporary file after it is converted into a file model, and adding a check in subsequent calls to see if that temporary file exists, and skip processing the upload request if it does not.

[mjauvin]

@bennothommo I tried your suggestion even though I know this would not get accepted as a solution (I think we need to find out why the formwidget gets inititialized 3 times)... it solves the problem of the file being added 3 times to the Model, but there is a second problem happening when UPDATING an existing model record which this "fix" does not solve:

Upload error
"get_class() expects parameter 1 to be object, null given" on line 838 of modules/backend/behaviors/RelationController.php

[mjauvin]

Anyone has an idea on why this breaks when handled from a Popup form and not when handled by a regular create view with the same model/file upload widget!?

[mjauvin]

You can see the [unbroken] bahavior when using the "Users" menu of the same test demo and uploading images from the Single and Multiple Image tabs

[bennothommo]

@mjauvin I'd say the relation form has its own initialisation process on top of the main form initialisation. AJAX calls also tend to refresh the widgets and forms they are related to, which usually involves running initialisation again.

[mjauvin]

This is way over my head, I'll leave this to someone more familiar with all
this initialization mess... ;)

On Wed, May 1, 2019 at 10:59 AM Ben Thomson [e-mail]
wrote:

@mjauvin https://github.com/mjauvin I'd say the relation form has its
own initialisation process on top of the main form initialisation. AJAX
calls also tend to refresh the widgets and forms they are related to, which
usually involves running initialisation again.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/octobercms/october/issues/4300#issuecomment-488306156,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPLTPQYBMADXWSDS2LVHITPTGV4VANCNFSM4HJBC5HQ
.

--
Marc

[LukeTowers]

@bennothommo do you feel up to the task?

[bennothommo]

@LukeTowers nope :P. Jokes, I'll add it to the list.

[alxy]

I have found an alternative fix, and that is to rework the FileUpload widget to use an ajax event handler for the uploading, see https://github.com/octobercms/october/pull/4311

[bennothommo]

@tobias-kuendig Could you give @alxy 's PR #4311 a test when you get a chance?

[tobias-kuendig]

I can confirm PR #4311 solves the issues I was experiencing. Great work, @alxy :+1:!

[daftspunk]

This should be available in Build 455.

Thanks and nice solution @alxy!