search

Gargron / fileupload

PHP FileUpload library that supports chunked uploads
MIT License
460 stars 87 forks source link

Undefined behavior when validation fails - missing else section? #37

Closed radmax closed 7 years ago

radmax commented 7 years ago

In case of failed validation L362 of FileUpload.php has undefined state.

I patched it by adding else section and throwing exception, that i handle in my app. What should be proper behavior?

adelowo commented 7 years ago

Hi @radmax , here (https://github.com/Gargron/fileupload/blob/master/src/FileUpload/FileUpload.php#L362), i think the undefined behaviour is as a result of the $file_path variable that does not exist if the validation fails. Am i right ? would like more info on this issue though.

Another thing is i do not like the fact that an Exception is introduced since it is kind of different from the default behaviour - which is to still return a File object that has it's completed property as false.

Could you try moving $file_path (https://github.com/Gargron/fileupload/blob/master/src/FileUpload/FileUpload.php#L316) to maybe 5 lines above and see if this undefined behaviour still exists.

radmax commented 7 years ago

Ok, i agree that just throwing naked Exception is not best solution.

I think problem is bigger than just undefined variable. If i move statement in front of validation condition, then next point of failure is File.php constructor calling method setMimeType. Filename passed is path of file for final destination directory. If validation had passed it would be copied there, but that file doesnt exist.

adelowo commented 7 years ago

Oh that's true.. The constructor would "error out"..

So an exception seems the only way to go ? Do you have any other idea as to how to handle this. ?

Currently not with my laptop

adelowo commented 7 years ago

If not, I'd merge #38 later

radmax commented 7 years ago

I tried #38 only as hotfix. Throwing exception should be implemented in better way (include specific message what failed or even custom exception class). I don't think my PR should be merged as it is right now. My intention was to start discussion and provide starting solution for this problem.

adelowo commented 7 years ago

Thanks a lot.. I would look into this tomorrow and try to find a balanced ground

adelowo commented 7 years ago

HI @radmax, I tried something on the hot-fix branch that doesn't involve an Exception. Can you take a look at it and try that out if it solves your particular issue ?

Here is the commit https://github.com/Gargron/fileupload/commit/5595d8b8ecbf43bc0efab7423e26bece0b7f81d9 .

adelowo commented 7 years ago

Ping @radmax

radmax commented 7 years ago

I tested it and it works. Thanks.

adelowo commented 7 years ago

Thanks a lot, i should tag a release with this fix as soon as possible. #44 is holding me back

adelowo commented 7 years ago

I just tagged a new release.. If there are issues, you can always reopen this issue.

BTW, thanks