UploadedFileFactory - Failing to create a readable UploadedFile from string

[dakujem]

While I was updating Mika Tuupola's tuupola/http-factory to support this package, I stumbled upon this bug. The tests won't pass for slim/psr7 as can be seen here tuupola/http-factory#10, while they do pass for the other PSR-7 implementations.

I was trying to solve it for a couple of hours, but failed, so for now I'm just providing the failing test and a temporary comparison to nyholm/psr7 that does not fail.

The code of the test is taken from Interop\Http\Factory\UploadedFileFactoryTestCase (http-interop/http-factory-tests package).

I will try a bit later again, but I suspect the authors will have much better insight into the matter.

[adriansuter]

The problem might be, that the temp stream created, fails to be readable in \Slim\Psr7\Factory\UploadedFileFactory::createUploadedFile():

Slim-Psr7/src/Factory/UploadedFileFactory.php

Line 32 in 6e32929

if (!is_string($file) || !is_readable($file)) {

A var_dump() of the $stream variable right before that line mentioned above, gave the following:

class Slim\Psr7\Stream#8517 (7) {
  protected $stream =>
  resource(1089) of type (stream)
  protected $meta =>
  array(6) {
    'wrapper_type' =>
    string(3) "PHP"
    'stream_type' =>
    string(4) "TEMP"
    'mode' =>
    string(3) "w+b"
    'unread_bytes' =>
    int(0)
    'seekable' =>
    bool(true)
    'uri' =>
    string(10) "php://temp"
  }
  protected $readable =>
  NULL
  protected $writable =>
  NULL
  protected $seekable =>
  NULL
  protected $size =>
  NULL
  protected $isPipe =>
  NULL
}

While is_readable($file) returns false, $stream->isReadable() returns true.

@l0gicgate Do you have any idea why this happens?

[adriansuter]

I just changed

Slim-Psr7/src/Factory/UploadedFileFactory.php

Lines 32 to 34 in 6e32929

	if (!is_string($file) || !is_readable($file)) {
	throw new InvalidArgumentException('File is not readable.');
	}

into

       if (!is_string($file)) {
            throw new InvalidArgumentException('File is not readable.');
        }

        if ($stream instanceof Stream) {
            if (!$stream->isReadable()) {
                throw new InvalidArgumentException('File is not readable.');
            }
        } elseif (!is_readable($file)) {
            throw new InvalidArgumentException('File is not readable.');
        }

See adriansuter@9c7f060

Now, the contents of the uploaded file seems to be empty.

[adriansuter]

In

Slim-Psr7/src/Factory/UploadedFileFactory.php

Line 40 in 6e32929

return new UploadedFile($file, $clientFilename, $clientMediaType, $size, $error);

we would actually pass php://temp as $file to the constructor.

But later on, in \Slim\Psr7\UploadedFile::getStream(), the stream would be recreated from the given file, which is php://temp:

Slim-Psr7/src/UploadedFile.php

Line 106 in 6e32929

$this->stream = (new StreamFactory())->createStreamFromFile($this->file);

Maybe that's the issue?

[dakujem]

Sorry, I forgot to provide those details: Yes, the file is not readable at the moment of the check, but even if you fix the check the stream content will be empty, that is what I found out. Anyway the same happens in the nyholm implementation (file is not readable in the constructor), but the stream content returns as expected. Tough I have not investigated further.

[dakujem]

So I managed to fix this particular issue by allowing to create an instance of UploadedFile from a stream directly (no recreation of the stream). But this breaks the other tests, obviously, and is most probably a BC break.

The issue is this:
UploadedFileFactory will extract the filename of the stream, pass it down to the UploadedFile and then the UploadedFile recreates the stream when getStream method is invoked. This IMHO fails for temporary streams though:

php://memory and php://temp are not reusable, i.e. after the streams have been closed there is no way to refer to them again.

PHP php:// documentation, scroll to the bottom of the page

[adriansuter]

Exactly.

Maybe wee need to add the stream as optional param to the uploaded file creator. What are your opinions?

Or as a new setter method to avoid BC.

[dakujem]

My opinion? I'd prefer to modify the UploadedFile::__construct as it is in this draft PR (that is, remove the type constraint from the $file argument) and possibly rename it to $fileOrStream, to keep the interface simple. This is IMHO much better than adding an optional $stream parameter, because then there would be confusion as to whether the filename comes from $file parameter or $stream->getMetadata('uri)... Or how to solve case where these two would not match.

P.S.: I was wrong with the BC break - the classes that extend UploadedFile and overload the constructor should keep working without problems even if the superclass's type constraint is removed (I tried with PHP 7.2 and 7.3).

The tests that this modification breaks are in fact only those that expect an exception being thrown:

1) Slim\Tests\Psr7\Factory\UploadedFileFactoryTest::testCreateUploadedFileWithInvalidUri
Failed asserting that exception of type "InvalidArgumentException" is thrown.
2) Slim\Tests\Psr7\Factory\UploadedFileFactoryTest::testCreateUploadedFileWithNonReadableFile
Failed asserting that exception of type "InvalidArgumentException" is thrown.
3) Slim\Tests\Psr7\Integration\UploadedFileTest::testGetSize
Failed asserting that 0 is null.

(except for the last one with file size, i'll investigate that one)

So if you're asking me, I'd go down this way and finish this PR, possibly altering the tests that expect the exception and/or move the "is readable" check elsewhere (e.g. to the getStream method).

[adriansuter]

Hi @dakujem

I agree. Probably we should use $fileOrStream or even $fileNameOrStream as variable name in the constructor.

Regarding the "get size" test case: It seems that the slim integration of \Http\Psr7Test\Tests\Slim\UploadedFileTest::createSubject() does not write the created temporary file to disk.

Slim-Psr7/tests/Integration/UploadedFileTest.php

Lines 23 to 28 in 6e32929

	public function createSubject()
	{
	$file = tempnam(sys_get_temp_dir(), 'Slim_Http_UploadedFileTest_');
	return new UploadedFile($file);
	}

I changed that in my branch addressing this subject adriansuter@c40aebf

[dakujem]

@adriansuter Shoul I continue to solve this issue? I'd most probably need to update the tests so that they do not expect exception at the time of calling UploadedFileFactory::createUploadedFile (because of the postponed check).

[adriansuter]

@dakujem
You can if you like and if you have time.

Why do you postpone those checks? Couldn't we leave them in \Slim\Psr7\Factory\UploadedFileFactory::createUploadedFile()?

If you don't like to continue to work on this issue, I could PR my branch https://github.com/adriansuter/Slim-Psr7/tree/patch-uploaded-file-temp-stream instead

[dakujem]

I can continue, I just was not sure you liked the changes... would not want to waste time.

For reason I have yet to discover, the "is readable" check will always fail for php://temp streams that have just been created.

This makes the test sequence used in the http-interop tests fail. I have compared this to the nyholm implementation and they do not check for file being readable and thus make the factory work as expected.

This is why I tried to postpone the test to the moment the stream is actually used.

[adriansuter]

See #100 (comment) for infos about the "is readable" check.

In the end, the composer.json should be removed from the PR (set back to what it was before).

[dakujem]

Okay, I will try to work it out with your version of the factory. Of course I plan to squash and polish everything before marking the PR ready, obviously no need for nyholm nor the related test case :-)

[dakujem]

So I prepared this, but I have one question before I mark this ready:
in the UploadedFileFactory as you have written above, the code goes like this:

        if ($stream instanceof Stream) {
            if (!$stream->isReadable()) {
                throw new InvalidArgumentException('File is not readable.');
            }
        } elseif (!is_readable($file)) {
            throw new InvalidArgumentException('File is not readable.');
        }

... well should we care about the implementation? I believe passing StreamInterface instance as the $stream argument is enough, as the isReadable method is declared by the interface. I believe the above code can be reduced into:

        if (!$stream->isReadable()) {
            throw new InvalidArgumentException('File is not readable.');
        }

However, with this one test (UploadedFileFactoryTest::testCreateUploadedFileWithNonReadableFile) fails, due to the mock object not implementing the isReadable method. I will play with this, though I am not familiar with the prophecy package used for mocking.

[dakujem]

I managed to update the test "prophecy" so the test does not fail. IMHO this is the way the factory should be implemented (independent of the particular implementation of the StreamInterface being passed in).

EDIT: cleaned and updated the code so the CS fixer stays cool.

@adriansuter any thoughts?

[coveralls]

Coverage increased (+0.003%) to 99.733% when pulling 1a624ff on dakujem:patch-uploaded-file-temp-stream into 6e32929 on slimphp:master.

[dakujem]

This may be due to loosening the UploadedFile parameter type restriction. How can I/we improve the coverage? I believe both cases when a stream and a filename is used are tested. Maybe the case where the exception is thrown is not. Let me see...

[adriansuter]

@dakujem Yes, we should add a test case for when the $filenameOrStream is neither a stream nor a file name (string). Then we should expect an exception to be thrown.

[dakujem]

Done.

[adriansuter]

42 - nerd 😄.

At a first glance, your PR looks goot to me. Although I have to check it in more detail. Thanks already for your work.

[dakujem]

Got a towel with me all the time...

You're welcome. Glad to help.

[adriansuter]

We should make sure that the returned value of getMetadata() is a string and if not, throw an exception.

[adriansuter]

I think that the docblock can be removed.

[dakujem]

I may have messed up, I'm not sure how to mark the changes "done".