PHP Warning: Creating default object from empty value

[Indigo744]

Hello,

First of all, thank you for this library. It works really well.

However, I have sometimes these kind of errors: Severity: Warning --> Creating default object from empty value /.../vendor/gargron/fileupload/src/FileUpload/FileUpload.php 263

Looking at the code:

            } else {
                if ($upload && $upload['error'] != 0) {
                    $file = $this->getFileContainer();
                    $file->error = $this->getMessage($upload['error']); // <--- warning line is here
                    $file->errorCode = $upload['error'];
                    $this->files[] = $file;
                }

It seems that $file is not set properly (i.e. empty) hence the error when setting $file->error.

I'm not really sure what you're supposed to do in this branch, hence why I don't try to fix it and push a PR...

I hope you will find the time to look at it :)

Cheers,

[adelowo]

Hi, how can i reproduce this locally ?

[Indigo744]

I'm not really sure... I happens quite frequently when leaving the page (or doing a refresh) while uploading a big file in chunks.

However, just by looking at the code I can see that, in this branch, the $this->getFileContainer() will always return something empty because the $this->fileContainer will not be set. It is set by the method process() which is not called in this branch.

[adelowo]

The fileContainer is supposed to be set in the constructor... Not sure why it is isn't

[adelowo]

I would take a look at this tomorrow but if you can fix this, please do :+1:

[Indigo744]

In the constructor ? I don't see it...

    public function __construct($upload, $server, FileNameGenerator $generator = null)
    {
        $this->upload = isset($upload) ? $upload : null;
        $this->server = $server;
        $this->fileNameGenerator = $generator ?: new Simple();
        $this->prepareMessages();
    }

[Indigo744]

For me, this attribute is set in the process()method :

    protected function process($tmp_name, $name, $size, $type, $error, $index = 0, $content_range = null)
    {
        $this->fileContainer = $file = new File($tmp_name);
        ...

[adelowo]

I meant it should have been initialized in the constructor but it wasn't.. I have added this to my todo list though

[Indigo744]

Oh sorry, I've read your answer too quickly!

I'm not really sure actually how to fix this. Maybe use a default standard class like $this->fileContainer = new \stdClass(); ?

[adelowo]

That would be off, it is supossed to be an instance of File specific to the current uploaded file (being processed)

[Indigo744]

Yeah, you're right... But we don't have the name in the constructor yet...

Maybe in the offending branch, check for the return value of getFileContainer() and if null create a standard class with only error and errorCode attribute set? Can't do better, since there is an upload error, but not an upload tmp_name value...

[Indigo744]

I just made some more tests when aborting an upload.

In this particular branch

$upload = Array
(
    [name] => someFileName.ext
    [type] => 
    [tmp_name] => 
    [error] => 3
    [size] => 0
)

The code 3 corresponds to UPLOAD_ERR_PARTIAL, which seems logical.

[Indigo744]

So, i would suggest something along the lines of:

                if ($upload && $upload['error'] != 0) {
                    // $this->fileContainer is empty at this point
                    // $upload['tmp_name'] is also empty
                    // So we create a File instance from $upload['name']
                    $file = new File($upload['name']);
                    $file->error = $this->getMessage($upload['error']);
                    $file->errorCode = $upload['error'];
                    $this->files[] = $file;
                }

[adelowo]

LGTM... can you make a PR ?

[Indigo744]

However, I wonder what the setMimeType() method of the File class will do with an unknown file... I will perform some test on my server and get back to you.

[Indigo744]

Well, as I supposed, it doesn't like it: Severity: Warning --> finfo_file(filename.ext): failed to open stream: No such file or directory We are trading an error with another...

[adelowo]

Oops... I would need time to go over this properly

[Indigo744]

Actually, the PHPDoc for the SplFileInfo constructor says (http://php.net/manual/en/splfileinfo.construct.php):

Creates a new SplFileInfo object for the file_name specified. The file does not need to exist, or be readable.

Hence, we could also modify your File class to accomodate a non-existing file. For instance, simply checking for file existence before checking the mime-type:

    protected function setMimeType($fileName)
    {
        if (file_exists($fileName)) {
            $this->mimeType = finfo_file(finfo_open(FILEINFO_MIME_TYPE), $fileName);
        }
    }

[adelowo]

Oh, nice catch..

[Indigo744]

Another error ambushed... Severity: Notice --> Undefined property: FileUpload\File::$size /gargron/fileupload/src/FileUpload/FileUpload.php 586

Related to this:

    protected function getNewHeaders(array $files, $content_range)
    {
        $headers = array(
            'pragma' => 'no-cache',
            'cache-control' => 'no-store, no-cache, must-revalidate',
            'content-disposition' => 'inline; filename="files.json"',
            'x-content-type-options' => 'nosniff'
        );

        if ($content_range && is_object($files[0]) && $files[0]->size) {       // <-- here
            $headers['range'] = '0-' . ($this->fixIntegerOverflow($files[0]->size) - 1);
        }

        return $headers;
    }

I have corrected using a check for this property :

    protected function getNewHeaders(array $files, $content_range)
    {
        $headers = array(
            'pragma' => 'no-cache',
            'cache-control' => 'no-store, no-cache, must-revalidate',
            'content-disposition' => 'inline; filename="files.json"',
            'x-content-type-options' => 'nosniff'
        );

        if ($content_range && is_object($files[0]) && isset($files[0]->size) && $files[0]->size) {
            $headers['range'] = '0-' . ($this->fixIntegerOverflow($files[0]->size) - 1);
        }

        return $headers;
    }

All error seems to have gone now...

If it's ok for you, I will try to make a PR tomorrow.

[Indigo744]

PR is live :)