Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

$_FILES missing size and type after flow.js upload #155

Closed
arkuuu opened this issue Jan 14, 2016 · 16 comments
Closed

$_FILES missing size and type after flow.js upload #155

arkuuu opened this issue Jan 14, 2016 · 16 comments

Comments

@arkuuu
Copy link

arkuuu commented Jan 14, 2016

Hello,

I'm trying to get flow.js ready. This is how I've set up flow.js:

$(document).ready(function(){

            var flow = new Flow({
                target: '{$uploadUrl}',
                singleFile: true,
                maxChunkRetries: 0,
                testChunks: false
            });

            flow.assignBrowse($('#{$addButtonId}').get());

            flow.on('fileAdded', function(file, event){

                console.log(file, event);
                console.log('added');
            });
            flow.on('fileSuccess', function(file,message){
                console.log(file,message);
                console.log('succes');
            });
            flow.on('fileError', function(file, message){
                console.log(file, message);
                console.log('error');
            });
            flow.on('filesSubmitted', function (file) {
                console.log(file);
                file[0].flowObj.upload();
            });
 });

Everything works fine, except that the PHP script at $uploadUrl does only receive the following in $_FILES (var_dump()'d at the first line, so nothing can interfere):

array (
  'file' => 
  array (
    'name' => 'select-css.txt,
    'type' => '0',
    'tmp_name' => '/tmp/php99aZVY',
    'error' => 0,
    'size' => 0,
  ),
)

So, $_FILES['size'] and $_FILES['type'] is missing, which makes it an invalid upload. I'm using flowjs/flow-php-server on the server side. Note that $_FILES['error']is 0, so this shouldn't be an error due to php upload_max_size etc

The request send to the server has the following payload (using Chrome):

------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="flowChunkNumber"

1
------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="flowChunkSize"

2097152
------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="flowCurrentChunkSize"

1561
------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="flowTotalSize"

1561
------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="flowIdentifier"

1561-select-csstxt
------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="flowFilename"

select-css.txt
------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="flowRelativePath"

select-css.txt
------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="flowTotalChunks"

1
------WebKitFormBoundaryJNrM1tKMEhvD5dag
Content-Disposition: form-data; name="file"; filename="select-css.txt"
Content-Type: 0


------WebKitFormBoundaryJNrM1tKMEhvD5dag--

Another jquery file upload librariy works well and the standard POST upload, too.
What am I doing wrong that prevents flow.js from sending the correct size and type?

@arkuuu
Copy link
Author

arkuuu commented Jan 18, 2016

I think I found it.

flow.js:1052

function webAPIFileRead(fileObj, fileType, startByte, endByte, chunk)

flow.js:1331

read(this.fileObj, this.startByte, this.endByte, this.fileType, this);

Parameters are switched here.

Change line 1052 to

function webAPIFileRead(fileObj, startByte, endByte, fileType, chunk)

and it works.

@evilaliv3
Copy link
Member

nice catch @arkuuu ! i'm already retesting your fix and i will be back to you shortly :)

@evilaliv3
Copy link
Member

@arkuuu you just found a bug in my patch: 88c9aa7

i'm going to first write a unit test that proves the malfunction and then fix it.

anyway this patch has still not been released; @arkuuu do you confirm that you have this bug while testing using manually the master branch?

\cc @AidasK

@arkuuu
Copy link
Author

arkuuu commented Jan 18, 2016

@evilaliv3 yes, I was using flow.js from the master branch, not the released version.

@evilaliv3
Copy link
Member

okay! i'm taking care of this now implementing all the unit tests that would have helped finding this bug.

@AidasK: are you planning to release the master branch than?

a lot of bugfixing and improvements have been done since 2.9.0

@AidasK
Copy link
Member

AidasK commented Jan 18, 2016

Thanks @arkuuu, nice catch. Maybe node.js sample will work after this. If it will, we can make a 2.10.0 release

evilaliv3 added a commit that referenced this issue Jan 18, 2016
@evilaliv3
Copy link
Member

@AidasK exactly! just retested the nodejs sample and IT WORKS!

all ready for a super 2.10.0 👍

@AidasK
Copy link
Member

AidasK commented Jan 18, 2016

@evilaliv3 I have tried node.js sample, and it seems uploaded file is missing last chunk. I have tried files bigger than a chunks size (> 1mb).
In the output I am getting "invalid_flow_request4".

@evilaliv3
Copy link
Member

this is probably showing that we would require to improve the unit tests as they are not covering the situation where the file is bigger than the chunksize?

@evilaliv3
Copy link
Member

i'm going to investigate this issue now.

@AidasK
Copy link
Member

AidasK commented Jan 18, 2016

If I set forceChunkSize: true option, file size matches. But downloaded file has different md5 checksum.

@AidasK
Copy link
Member

AidasK commented Jan 18, 2016

Everything works if I remove:
https://github.com/flowjs/flow.js/blob/master/samples/Node.js/flow-node.js#L50-L53
And
https://github.com/flowjs/flow.js/blob/master/samples/Node.js/flow-node.js#L35-L38
And
forceChunkSize: true is set to True.

So everything should be fine if we could fix forceChunkSize option.

@evilaliv3
Copy link
Member

i'm investigating also; since now i discovered that the issue is for sure in the last chunk.

right now the differences i see between 2.9.0 and master is that in 2.9.0 (where all works) the flowCurrentChunkSize of the last chunk size is less that chunk size, while in the master the last chunk has the size of a full chunk.

@evilaliv3
Copy link
Member

okay, bug found, i'm preparing the patch :)

when we implemented the read hook we moved the following code inside the preprocessFinished:

      // Compute the endByte after the preprocess function to allow an
      // implementer of preprocess to set the fileObj size
      this.endByte = Math.min(this.fileObj.size, (this.offset + 1) * this.chunkSize);
      if (this.fileObj.size - this.endByte < this.chunkSize &&
          !this.flowObj.opts.forceChunkSize) {
        // The last chunk will be bigger than the chunk size,
        // but less than 2*this.chunkSize
        this.endByte = this.fileObj.size;
      }

but this would not work when the preprocess function is not defined, so i'm thinking a little on what can be the best solution.

and i will also implement a test for the streaming capability that has not been unit tested so far.

evilaliv3 added a commit that referenced this issue Jan 18, 2016
@evilaliv3
Copy link
Member

Ok @AidasK even if not part of this ticket i've referred this issue in the commit given the fact that we discussed it here.

I will now proceed implementing the full example the testing for the streaming functionality.

@AidasK
Copy link
Member

AidasK commented Jan 19, 2016

Thanks for fixing this! You can see released version here: https://github.com/flowjs/flow.js/releases/tag/v2.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants