Nyholm / psr7-server

Helper classes to use any PSR7 implementation as your main request and response
MIT License
90 stars 21 forks source link

Failed to create UploadedFile when an upload error occurred #20

Closed emonkak closed 6 years ago

emonkak commented 6 years ago

The following warning is occurred because trying to create a stream from an empty filename.

Warning: fopen(): Filename cannot be empty

The problem is in createUploadedFileFromSpec() function. I think that the function should check for upload errors.

private function createUploadedFileFromSpec(array $value)
{
    if (\is_array($value['tmp_name'])) {
        return $this->normalizeNestedFileSpec($value);
    }

    return $this->uploadedFileFactory->createUploadedFile(
        $this->streamFactory->createStreamFromFile($value['tmp_name']),
        (int) $value['size'],
        (int) $value['error'],
        $value['name'],
        $value['type']
    );
}
Zegnat commented 6 years ago

I really dislike problems that are multi-library problems. There are two things in play here:

  1. ServerRequestCreator::createUploadedFileFromSpec returns an instance (or (nested) array) of PSR-7 UploadedFileInterface from whichever factory you have provided. These instances include the error code (which is being passed along), so theoretically we shouldn’t have to do any other upload error processing.

    PSR-17 UploadedFileFactoryInterface::createUploadedFile requires a valid PSR-7 StreamInterface. The code as it is currently assumes that StreamFactoryInterface::createStreamFromFile is just always going to work. Which is not a good idea. In case it goes wrong I think we should be creating an empty instance instead.

  2. The factory you are using (is it nyholm/psr7?) doesn’t seem to throw a \RuntimeException when it fails to create the stream instance. (Or it did, but you didn’t see it?) This goes against PSR-17, so the fix I described above wouldn’t solve your issue!

    The factory library you are using will need to change to correctly handle \fopen failures. Which is outside of this package’s control.

Zegnat commented 6 years ago

@emonkak I would appreciate it if you could have a look at my PR and let me know if this solves the issue for you: Nyholm/psr7-server#21.